Denton Liu <liu.denton@xxxxxxxxx> writes: > Hi Junio, > > On Fri, Nov 08, 2019 at 09:49:02PM +0900, Junio C Hamano wrote: >> Denton Liu <liu.denton@xxxxxxxxx> 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.
