update dolt blame to use sql backend and accept specific revision #6065
Conversation
| @@ -108,3 +108,29 @@ teardown() { | |||
| [[ "$output" =~ "defaultDB does not exist" ]] || false | |||
| } | |||
|
|
|||
| @test "sql-local-remote: verify dolt blame behavior is identical in switch between server/no server" { | |||
| cd defaultDB | |||
There was a problem hiding this comment.
There is a bug here where we are out of sync with other command behavior. You shouldn't need to cd defaultDB - and in fact I tested this and you get an error stating that you need to dolt init first. This is incorrect behavior.
There was a problem hiding this comment.
I'm not sure I understand why I don't need to do cd defaultDB first. I would need to be in a dolt repo to do dolt add and dolt commit right?
| [ "$status" -eq 0 ] | ||
| [[ "$output" =~ "1".*"insert initial value into test" ]] || false | ||
| [[ "$output" =~ "2".*"insert more values into test" ]] || false | ||
| [[ "$output" =~ "3".*"insert more values into test" ]] || false |
There was a problem hiding this comment.
I think a simpler check would be to compare the total output of both blame calls. The line by line information will be the same if that checks out, and the tests as it stands now would allow for one implementation to drop the commit date and we'd never know it.
| if apr.NArg() == 1 { | ||
| schema, ri, err = queryist.Query(sqlCtx, fmt.Sprintf(blameQueryTemplate, apr.Arg(0), "HEAD")) | ||
| } else { | ||
| schema, ri, err = queryist.Query(sqlCtx, fmt.Sprintf(blameQueryTemplate, apr.Arg(1), apr.Arg(0))) |
There was a problem hiding this comment.
We need to validate the input or we'll have a SQL injection bug here. There is hopefully code we can find around here which validates a revision value. Example of exploit:
$ dolt blame 'HEAD;insert into tbl values (....);' tbl
At the moment, this doesn't have any real impact because blame doesn't have a write transaction
There was a problem hiding this comment.
I think when we parse the value given after AS OF it will catch invalid revision values right? I'm not too clear on what your given example shows.
go/cmd/dolt/commands/blame.go
Outdated
| schema, ri, err = queryist.Query(sqlCtx, fmt.Sprintf(blameQueryTemplate, apr.Arg(0), "HEAD")) | ||
| } else { | ||
| // validate input | ||
| _, err = ResolveCommitWithVErr(dEnv, apr.Arg(0)) |
There was a problem hiding this comment.
See chat discussion. This is a very reliable way to validate the input - but it used dEnv which is off limits.
|
|
||
| run dolt blame test | ||
| [ "$status" -eq 0 ] | ||
| [[ "$output" = $out ]] || false |
There was a problem hiding this comment.
Noice. I like this test a lot more now :-)
|
@coffeegoddd DOLT
|
|
@coffeegoddd DOLT
|
This change updates
dolt blameto use the appropriate sql engine to generate results. This change also allows users to specify a specific revision to annotate from.Related: #3922