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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Oct 8, 2022, 17:10 UTC
Message-ID: <CAPig+cRxy5C+CqUOzmhe16j+hssxsygha3huVga8tLJ+imM4Hw@mail.gmail.com>
In-Reply-To: <a7aca891-dd37-7e5e-61fc-8012fec18ae9@gmail.com>

On Sat, Oct 8, 2022 at 5:12 AM Rubén Justo <rjusto@gmail.com> wrote:

Show 14 quoted lines

> On 8/10/22 9:23, Eric Sunshine wrote:
> > On Sat, Oct 8, 2022 at 3:07 AM Rubén Justo <rjusto@gmail.com> wrote:
> >> Oops. Thank you! I'll reroll back to using "git stripspace".
> >
> > `git stripspace` is perhaps unnecessarily heavyweight. Lightweight
> > alternatives would include:
> >
> >     printf "Branch description\n\n" >expect &&
> >     [...]
>
> Yeah, I thought about that.  What convinced me to use "git stripspace" was
> that maybe that '\n' tail could be removed sometime from the description
> setting and this will be fine with that.  I haven't found any reason for
> that '\n' and it bugs me a little seeing it in the config :-)

That reasoning occurred to me, as well, and I'd have no objection to git-stripspace if that's the motivation for using it. I don't feel strongly one way or the other, and my previous email was intended primarily to point out the lightweight alternatives in case you hadn't considered them. Feel free to use git-stripspace if you feel it is the more appropriate choice.

> But I agree with you about the unnecessarily heavyweight, though all
> involves a new process, probably echo, cat or printf are lightweight than
> another instance of git for that.

In most shells, `printf`, `echo`, `cat` are builtins, so no extra processes are involved (and `test_write_lines` is a shell function built atop `printf`). As a matter of personal preference, given the lightweight options, I find that `printf "...\n\n"` shows the intention of the code most plainly (but if you go with git-stripspace, then `echo` would be an idiomatic way to create the "expect" file).

> 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`.

> Anyway I'll switch to one of your suggestions, as it is definitely easier
> to read, understand and therefore change if needed.

It's fine to use git-stripspace if you feel it's more appropriate for the situation.