Re: [PATCH v4] branch: support for shortcuts like @{-1}, completed

From

Rubén Justo <rjusto@gmail.com>

Date

Oct 8, 2022, 23:28 UTC

Message-ID

<32e4f8cf-696c-317d-236b-0f64c1398a01@gmail.com>

In-Reply-To

<CAPig+cRxy5C+CqUOzmhe16j+hssxsygha3huVga8tLJ+imM4Hw@mail.gmail.com>

On 8/10/22 19:10, Eric Sunshine wrote:

>> All of this involves two files and that is how it is done almost everywhere
>> except in some places where it looks like an 'older way' (test_i18ngrep) of
>> doing it.  Is there any reason to do it this way and not using variables,
>> process substitution,..?
> 
> An invocation such as:
> 
>     test $(git foo) = $(git bar) &&
> 
> throws away the exit-code from the two commands, which means we'd miss
> if one or the other (or both) crashed, especially if the crash was
> after the command produced the correct output. These days we try to
> avoid losing the exit command of Git commands. It's possible to avoid
> losing the exit-code by using variables:
> 
>     expect=$(git foo) &&
>     actual=$(git bar) &&
>     test "$expect" = "$actual" &&
> 
> but, if the expected and actual output don't match, you don't learn
> much (other than that they failed). You could address that by showing
> a message saying what failed:
> 
>     expect=... &&
>     actual=... &&
>     if test "$expect" != "$actual"
>     then
>         echo "expect not match actual"
>         # maybe emit $expect and $actual too
>     fi
> 
> However, `test_cmp` gives you that behavior for free, and it emits a
> helpful "diff" upon failure, so these days we usually go with
> `test_cmp`.
>

I understand the possibility of losing exit codes or segfaults in
subcommands or pipes, but I'm more thinking in the element to compare to.
Having something like:
 
	test_cmp_str () {
		test -f "$1" || BUG "first argument must be a file"
		if test "$#" -gt 1
		then
			local F=$1
			shift
			printf "$@" | eval "$GIT_TEST_CMP" '"$F"' -
		else
			eval "$GIT_TEST_CMP" '"$1"' -
		fi
	}

to allow writing simpler tests like:

--- a/t/t3204-branch-name-interpretation.sh
+++ b/t/t3204-branch-name-interpretation.sh
@@ -142,8 +142,7 @@ test_expect_success 'edit-description via @{-1}' '
 	EDITOR=./editor git branch --edit-description @{-1} &&
 	test_must_fail git config branch.non-desc-branch.description &&
 	git config branch.desc-branch.description >actual &&
-	printf "Branch description\n\n" >expect &&
-	test_cmp expect actual
+	test_cmp_str actual "Branch description\n\n"
 '

--- a/t/t3204-branch-name-interpretation.sh
+++ b/t/t3204-branch-name-interpretation.sh
@@ -142,8 +142,10 @@ test_expect_success 'edit-description via @{-1}' '
        EDITOR=./editor git branch --edit-description @{-1} &&
        test_must_fail git config branch.non-desc-branch.description &&
        git config branch.desc-branch.description >actual &&
-       printf "Branch description\n\n" >expect &&
-       test_cmp expect actual
+       test_cmp_str actual <<-\EOF
+       Branch description
+
+       EOF
 '

My doubts are that maybe this can induce to some bad usages, is
unusable in some systems, it has already been explored and discarded,
using files gives the diff with nice names not "-",...

Maybe this needs a new RFC thread. I dunno.

Thanks for your comments and explanations.