| Message ID | 0d0696f310a6f8e13ed480b1a1e91cdc2debaa20.1573152599.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | t5520: various test cleanup | expand |
Denton Liu <liu.denton@gmail.com> writes: > Currently, in the case where we are using test_cmp_rev() to report > not-equals, we write `! test_cmp_rev`. However, since test_cmp_rev() > contains > > r1=$(git rev-parse --verify "$1") && > r2=$(git rev-parse --verify "$2") && > > In the case where `git rev-parse` segfaults and dies unexpectedly, the > failure will be ignored. Good justification. The last two lines are continuation of the sentence that begins the proposed log message, so downcase "In" at the beginning of the line. Also, when we present the problem to be solved at the beginning, it is customary to describe the status quo, and "Currently, " is a noiseword that does not add much information, so drop it. > Rewrite test_cmp_rev() to optionally accept "!" as the first argument to > do a not-equals comparison. Rewrite `! test_cmp_rev` to `test_cmp_rev !` > in all tests to take advantage of this new functionality. Good. > -# Tests that its two parameters refer to the same revision > +# Tests that its two parameters refer to the same revision, or if '!' is > +# provided first, that its other two parameters refer to different > +# revisions. OK. > test_cmp_rev () { > + local inverted_op > + inverted_op='!=' > + if test $# -ge 1 && test "x$1" = 'x!' > + then > + inverted_op='=' > + shift > + fi I'd rather avoid having to keep track of negation to reduce mental burden. How about using = by default and != when '!' was given (which would be more natural to readers) and call it $op, and say "if ! test $r1 $op $r2" where it is used? > if test $# != 2 > then > error "bug in the test script: test_cmp_rev requires two revisions, but got $#" > else > local r1 r2 > r1=$(git rev-parse --verify "$1") && > r2=$(git rev-parse --verify "$2") && If either of the calls fail, the assignment itself would fail, and the &&-cascade would stop without executing the if statment below. I see the "!" feature, but where is the promised "fix" for segfaulting rev-parse? Puzzled. > - if test "$r1" != "$r2" > + if test "$r1" "$inverted_op" "$r2" > then > + local comp_out > + if "x$inverted_op" = 'x=' > + then > + comp_out='the same' > + else > + comp_out='different' > + fi > cat >&4 <<-EOF > - error: two revisions point to different objects: > + error: two revisions point to $comp_out objects: > '$1': $r1 > '$2': $r2 > EOF > return 1 > fi > fi > }
On Fri, Nov 08, 2019 at 12:24:12PM +0900, Junio C Hamano wrote: > Denton Liu <liu.denton@gmail.com> writes: > > > Currently, in the case where we are using test_cmp_rev() to report > > not-equals, we write `! test_cmp_rev`. However, since test_cmp_rev() > > contains > > > > r1=$(git rev-parse --verify "$1") && > > r2=$(git rev-parse --verify "$2") && > > > > In the case where `git rev-parse` segfaults and dies unexpectedly, the > > failure will be ignored. I'll probably reword this to In the case where we are using test_cmp_rev() to report not-equals, we write `! test_cmp_rev`. However, since test_cmp_rev() contains r1=$(git rev-parse --verify "$1") && r2=$(git rev-parse --verify "$2") && `! test_cmp_rev` will succeed if any of the rev-parses fail. This behavior is not desired. We want the rev-parses to _always_ be successful. because of your puzzlement below. > > Good justification. The last two lines are continuation of the > sentence that begins the proposed log message, so downcase "In" at > the beginning of the line. Also, when we present the problem to be > solved at the beginning, it is customary to describe the status quo, > and "Currently, " is a noiseword that does not add much information, > so drop it. Thanks, I'll make a note to stop doing this in future patches as well. [...] > > > test_cmp_rev () { > > + local inverted_op > > + inverted_op='!=' > > + if test $# -ge 1 && test "x$1" = 'x!' > > + then > > + inverted_op='=' > > + shift > > + fi > > I'd rather avoid having to keep track of negation to reduce mental > burden. How about using = by default and != when '!' was given > (which would be more natural to readers) and call it $op, and say > "if ! test $r1 $op $r2" where it is used? Good idea, I felt a little uneasy doing the inverted thing but it never occurred to me to just negate the return code of `test`. > > > > if test $# != 2 > > then > > error "bug in the test script: test_cmp_rev requires two revisions, but got $#" > > else > > local r1 r2 > > r1=$(git rev-parse --verify "$1") && > > r2=$(git rev-parse --verify "$2") && > > If either of the calls fail, the assignment itself would fail, and > the &&-cascade would stop without executing the if statment below. > > I see the "!" feature, but where is the promised "fix" for > segfaulting rev-parse? > > Puzzled. I suppose your puzzlement comes from my badly worded commit message above. I meant to say that in the _hypothetical_ case that `git rev-parse` segfaults, it wouldn't be caught because we're blanket-ignoring failures if we do `! test_cmp_rev`. But I suppose I focused too much on segfaults. I guess I didn't realise that the problem is more general than that; any failure of `git rev-parse` should be reported. Thanks for the review, Denton
Denton Liu <liu.denton@gmail.com> writes: >> > local r1 r2 >> > r1=$(git rev-parse --verify "$1") && >> > r2=$(git rev-parse --verify "$2") && >> >> If either of the calls fail, the assignment itself would fail, and >> the &&-cascade would stop without executing the if statment below. >> >> I see the "!" feature, but where is the promised "fix" for >> segfaulting rev-parse? >> >> Puzzled. > > I suppose your puzzlement comes from my badly worded commit message > above. I meant to say that in the _hypothetical_ case that > `git rev-parse` segfaults, it wouldn't be caught because we're > blanket-ignoring failures if we do `! test_cmp_rev`. > > But I suppose I focused too much on segfaults. I guess I didn't realise > that the problem is more general than that; any failure of > `git rev-parse` should be reported. But if that is the case, shouldn't the part that runs two rev-parse read more like this? r1=$(git rev-parse --verify "$1") || error "'$1' does not name a valid object" r2=$(git rev-parse --verify "$2") || error "'$2' does not name a valid object" if ! test "$r1" $op "$r2" then ... they do not compare the same ... fi Offhand I do not know if the current callers depend on being able to pass a string that is not an object name in either $1 or $2 and a valid object name in the other one, and relying on the helper function to say "$1 and $2 are different!" If such callers exist, a defensive change like the above that requires the caller to always pass valid object names would need to be accompanied with changes to these callers, too. Overall, I think that would give us a better end result, but it might be a bit more work. Thanks.
Hi Junio, On Fri, Nov 08, 2019 at 09:49:02PM +0900, Junio C Hamano wrote: > Denton Liu <liu.denton@gmail.com> writes: > > >> > local r1 r2 > >> > r1=$(git rev-parse --verify "$1") && > >> > r2=$(git rev-parse --verify "$2") && > >> > >> If either of the calls fail, the assignment itself would fail, and > >> the &&-cascade would stop without executing the if statment below. > >> > >> I see the "!" feature, but where is the promised "fix" for > >> segfaulting rev-parse? > >> > >> Puzzled. > > > > I suppose your puzzlement comes from my badly worded commit message > > above. I meant to say that in the _hypothetical_ case that > > `git rev-parse` segfaults, it wouldn't be caught because we're > > blanket-ignoring failures if we do `! test_cmp_rev`. > > > > But I suppose I focused too much on segfaults. I guess I didn't realise > > that the problem is more general than that; any failure of > > `git rev-parse` should be reported. > > But if that is the case, shouldn't the part that runs two rev-parse > read more like this? > > r1=$(git rev-parse --verify "$1") || > error "'$1' does not name a valid object" > r2=$(git rev-parse --verify "$2") || > error "'$2' does not name a valid object" > if ! test "$r1" $op "$r2" > then > ... they do not compare the same ... > fi With your suggestion, we actually introduce subtle undesired behaviour. The `error` calls don't actually exit the function early. To make it work, we need to add && to the end of the `error` calls. I'm wondering why we want to do this, though. rev-parse should already output an error message on stderr in the case where the rev-parse fails. I guess the error message of "fatal: Needed a single revision" could probably be improved but that feels like an improvement that should be targeted to rev-parse. > > Offhand I do not know if the current callers depend on being able to > pass a string that is not an object name in either $1 or $2 and a > valid object name in the other one, and relying on the helper > function to say "$1 and $2 are different!" If such callers exist, a > defensive change like the above that requires the caller to always > pass valid object names would need to be accompanied with changes to > these callers, too. Overall, I think that would give us a better > end result, but it might be a bit more work. This patch changes all instances of `! test_cmp_rev` to `test_cmp_rev !`. Since nothing failed after applying the patch, I believe that all callers already pass in valid object names. Thanks, Denton > > Thanks.
Denton Liu <liu.denton@gmail.com> writes: > Hi Junio, > > On Fri, Nov 08, 2019 at 09:49:02PM +0900, Junio C Hamano wrote: >> Denton Liu <liu.denton@gmail.com> writes: >> >> >> > local r1 r2 >> >> > r1=$(git rev-parse --verify "$1") && >> >> > r2=$(git rev-parse --verify "$2") && >> >> >> >> If either of the calls fail, the assignment itself would fail, and >> >> the &&-cascade would stop without executing the if statment below. >> >> >> r1=$(git rev-parse --verify "$1") || >> error "'$1' does not name a valid object" >> r2=$(git rev-parse --verify "$2") || >> error "'$2' does not name a valid object" >> if ! test "$r1" $op "$r2" >> then >> ... they do not compare the same ... >> fi > > With your suggestion, we actually introduce subtle undesired behaviour. > The `error` calls don't actually exit the function early. To make it > work, we need to add && to the end of the `error` calls. Not &&-at-the-end, but yes, we'd need some early return after noticing a bad input from the caller. You said earlier that one of the issues that motivated you to update the helper was that this obvious typo r1=... r2= ... && ! test_rev_cmp "$r1" "$rr2" would not be noticed. For such a fix, I do not think it is sufficient to tweak the return value from the test_rev_cmp helper function if we allow callers to expect failure like so. And for that reason, your "allow 'test_rev_cmp ! R1 R2' syntax" part of the change makes quite a lot of sense. That again allows the callers to rely on failure return from test_rev_cmp as an error. > I'm wondering why we want to do this, though. rev-parse should already > output an error message on stderr in the case where the rev-parse fails. > > I guess the error message of "fatal: Needed a single revision" could > probably be improved but that feels like an improvement that should be > targeted to rev-parse. Not really. The callers of rev-parse plumbing should expect that exact string, if they want to differenciate different errors from the program. r1=$(git rev-parse --verify "$1") && r2=$(git rev-parse --verify "$2") || return 1 before we start the comparison between $r1 and $r2 may be a good way to clarify the intent of the code. Using "&&" instead of "|| return" and letting the whole function fail would not be incorrect, though. Thanks.
diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh index e819ba741e..52d476979b 100755 --- a/t/t2400-worktree-add.sh +++ b/t/t2400-worktree-add.sh @@ -438,7 +438,7 @@ test_expect_success 'git worktree add does not match remote' ' cd foo && test_must_fail git config "branch.foo.remote" && test_must_fail git config "branch.foo.merge" && - ! test_cmp_rev refs/remotes/repo_a/foo refs/heads/foo + test_cmp_rev ! refs/remotes/repo_a/foo refs/heads/foo ) ' @@ -483,7 +483,7 @@ test_expect_success 'git worktree --no-guess-remote option overrides config' ' cd foo && test_must_fail git config "branch.foo.remote" && test_must_fail git config "branch.foo.merge" && - ! test_cmp_rev refs/remotes/repo_a/foo refs/heads/foo + test_cmp_rev ! refs/remotes/repo_a/foo refs/heads/foo ) ' diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh index ab18ac5f28..f267f6cd54 100755 --- a/t/t3400-rebase.sh +++ b/t/t3400-rebase.sh @@ -64,7 +64,7 @@ test_expect_success 'rebase sets ORIG_HEAD to pre-rebase state' ' pre="$(git rev-parse --verify HEAD)" && git rebase master && test_cmp_rev "$pre" ORIG_HEAD && - ! test_cmp_rev "$pre" HEAD + test_cmp_rev ! "$pre" HEAD ' test_expect_success 'rebase, with <onto> and <upstream> specified as :/quuxery' ' diff --git a/t/t3421-rebase-topology-linear.sh b/t/t3421-rebase-topology-linear.sh index b847064f91..325072b0a3 100755 --- a/t/t3421-rebase-topology-linear.sh +++ b/t/t3421-rebase-topology-linear.sh @@ -61,7 +61,7 @@ test_run_rebase () { test_expect_$result "rebase $* -f rewrites even if upstream is an ancestor" " reset_rebase && git rebase $* -f b e && - ! test_cmp_rev e HEAD && + test_cmp_rev ! e HEAD && test_cmp_rev b HEAD~2 && test_linear_range 'd e' b.. " @@ -78,7 +78,7 @@ test_run_rebase () { test_expect_$result "rebase $* -f rewrites even if remote upstream is an ancestor" " reset_rebase && git rebase $* -f branch-b branch-e && - ! test_cmp_rev branch-e origin/branch-e && + test_cmp_rev ! branch-e origin/branch-e && test_cmp_rev branch-b HEAD~2 && test_linear_range 'd e' branch-b.. " @@ -368,7 +368,7 @@ test_run_rebase () { test_expect_$result "rebase $* -f --root on linear history causes re-write" " reset_rebase && git rebase $* -f --root c && - ! test_cmp_rev a HEAD~2 && + test_cmp_rev ! a HEAD~2 && test_linear_range 'a b c' HEAD " } diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh index 9efcf4808a..abbdc26b1b 100755 --- a/t/t3430-rebase-merges.sh +++ b/t/t3430-rebase-merges.sh @@ -346,7 +346,7 @@ test_expect_success 'A root commit can be a cousin, treat it that way' ' git merge --allow-unrelated-histories khnum && test_tick && git rebase -f -r HEAD^ && - ! test_cmp_rev HEAD^2 khnum && + test_cmp_rev ! HEAD^2 khnum && test_cmp_graph HEAD^.. <<-\EOF && * Merge branch '\''khnum'\'' into asherah |\ diff --git a/t/t3432-rebase-fast-forward.sh b/t/t3432-rebase-fast-forward.sh index 034ffc7e76..92f95b57da 100755 --- a/t/t3432-rebase-fast-forward.sh +++ b/t/t3432-rebase-fast-forward.sh @@ -64,7 +64,7 @@ test_rebase_same_head_ () { test_cmp_rev \$oldhead \$newhead elif test $cmp = diff then - ! test_cmp_rev \$oldhead \$newhead + test_cmp_rev ! \$oldhead \$newhead fi " } diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh index d1c68af8c5..1c51a9131d 100755 --- a/t/t3501-revert-cherry-pick.sh +++ b/t/t3501-revert-cherry-pick.sh @@ -106,7 +106,7 @@ test_expect_success 'cherry-pick on unborn branch' ' rm -rf * && git cherry-pick initial && git diff --quiet initial && - ! test_cmp_rev initial HEAD + test_cmp_rev ! initial HEAD ' test_expect_success 'cherry-pick "-" to pick from previous branch' ' diff --git a/t/t3508-cherry-pick-many-commits.sh b/t/t3508-cherry-pick-many-commits.sh index b457333e18..23070a7b73 100755 --- a/t/t3508-cherry-pick-many-commits.sh +++ b/t/t3508-cherry-pick-many-commits.sh @@ -5,7 +5,7 @@ test_description='test cherry-picking many commits' . ./test-lib.sh check_head_differs_from() { - ! test_cmp_rev HEAD "$1" + test_cmp_rev ! HEAD "$1" } check_head_equals() { diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index b299ecc326..064131ac39 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh @@ -1012,8 +1012,17 @@ test_must_be_empty () { fi } -# Tests that its two parameters refer to the same revision +# Tests that its two parameters refer to the same revision, or if '!' is +# provided first, that its other two parameters refer to different +# revisions. test_cmp_rev () { + local inverted_op + inverted_op='!=' + if test $# -ge 1 && test "x$1" = 'x!' + then + inverted_op='=' + shift + fi if test $# != 2 then error "bug in the test script: test_cmp_rev requires two revisions, but got $#" @@ -1021,10 +1030,17 @@ test_cmp_rev () { local r1 r2 r1=$(git rev-parse --verify "$1") && r2=$(git rev-parse --verify "$2") && - if test "$r1" != "$r2" + if test "$r1" "$inverted_op" "$r2" then + local comp_out + if "x$inverted_op" = 'x=' + then + comp_out='the same' + else + comp_out='different' + fi cat >&4 <<-EOF - error: two revisions point to different objects: + error: two revisions point to $comp_out objects: '$1': $r1 '$2': $r2 EOF
Currently, in the case where we are using test_cmp_rev() to report not-equals, we write `! test_cmp_rev`. However, since test_cmp_rev() contains r1=$(git rev-parse --verify "$1") && r2=$(git rev-parse --verify "$2") && In the case where `git rev-parse` segfaults and dies unexpectedly, the failure will be ignored. Rewrite test_cmp_rev() to optionally accept "!" as the first argument to do a not-equals comparison. Rewrite `! test_cmp_rev` to `test_cmp_rev !` in all tests to take advantage of this new functionality. Signed-off-by: Denton Liu <liu.denton@gmail.com> --- t/t2400-worktree-add.sh | 4 ++-- t/t3400-rebase.sh | 2 +- t/t3421-rebase-topology-linear.sh | 6 +++--- t/t3430-rebase-merges.sh | 2 +- t/t3432-rebase-fast-forward.sh | 2 +- t/t3501-revert-cherry-pick.sh | 2 +- t/t3508-cherry-pick-many-commits.sh | 2 +- t/test-lib-functions.sh | 22 +++++++++++++++++++--- 8 files changed, 29 insertions(+), 13 deletions(-)