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

[Eric Sunshine <sunshine@sunshineco.com>]

On Sat, Oct 8, 2022 at 5:12 AM Rubén Justo <rjusto@gmail.com> wrote:
> 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.