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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Oct 9, 2022, 06:46 UTC
Message-ID: <CAPig+cSdQsK7xVBb=ooOfP-hq1zavSV++iorLoni6o6etoLjzQ@mail.gmail.com>
In-Reply-To: <32e4f8cf-696c-317d-236b-0f64c1398a01@gmail.com>

On Sat, Oct 8, 2022 at 7:28 PM Rubén Justo <rjusto@gmail.com> wrote:

Show 33 quoted lines

> On 8/10/22 19:10, Eric Sunshine wrote:
> > 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:
>
> +       test_cmp_str actual "Branch description\n\n"
>
> +       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 "-",...

I wouldn't be at all surprised if this sort of thing has been discussed already (it sounds familiar enough), or that such an improvement has already been submitted in the form of a patch (possibly by Ævar, Cc:'d).

My knee-jerk reaction is that the form which takes a string as argument would hardly be used, thus an unnecessary complication. The form which accepts stdin could even be retrofitted onto the existing test_cmp, thus you don't even need to invent a new function name. A different approach would be to introduce a function `test_stdout` which both accepts a command to run, as well as the "expected" text on stdin with which to compare the output of the command; i.e. a combination of the existing `test_stdout_line_count` in t/test-lib-functions.sh and your proposed function above.

That said, I'm not yet seeing all that much added value in such a function; at most, it seems to save only a single line of code, and it's not as if the code it's replacing was doing anything complicated or hard to grok. So, I'm pretty ambivalent about it. But, of course, I'm only one person expressing a knee-jerk reaction. Submitting the idea as an RFC patch might generate more feedback.