Re: [PATCH v3] notes: teach the -e option to edit messages in editor

From: Taylor Blau <me@ttaylorr.com>
Date: Oct 21, 2024, 19:52 UTC
Message-ID: <Zxaw+pUMT4juAePD@nand.local>
In-Reply-To: <CAPig+cSw_F97nBzO3Z7t2Zrv5TZwGnYiQLhpq2iKgLfxhhxvfQ@mail.gmail.com>

On Mon, Oct 21, 2024 at 02:28:05PM -0400, Eric Sunshine wrote:

Show 24 quoted lines

> On Mon, Oct 21, 2024 at 12:53 PM Taylor Blau <me@ttaylorr.com> wrote:
> > On Mon, Oct 21, 2024 at 02:38:15PM +0000, Samuel Adekunle Abraham via GitGitGadget wrote:
> > > +     MSG="Edited notes message" git notes add -m "Initial notes message" -e &&
> > > +     echo "Edited notes message" >expect &&
> >
> > Very nice use of the fake_editor script here.
> >
> > It is a little cumbersome to repeat the same message in MSG= and when
> > populating the 'expect' file. Perhaps instead this could be written as:
> >
> >     echo "edited notes message" >expect &&
> >     MSG="$(cat expect)" git notes -add -m "initial" -e
>
> This suggested rewrite feels unusually roundabout and increases
> cognitive load for readers who now have to trace the message flow from
> script to file and back into script, and to consider how the loss of
> trailing newline caused by use of $(...) impacts the behavior. It also
> wastes a subprocess (which can be expensive on some platforms, such as
> Windows). If we're really concerned about this minor duplication of
> the message, we can instead do this:
>
>     msg="edited notes message" &&
>     echo "$msg" >expect &&
>     MSG="$msg" git notes -add -m "initial" -e

I am not sure I agree that the suggested version is clearer. The way I read mine is that we are writing what we expect to see in "expect", and then setting up MSG to be the same thing.

I definitely do not feel strongly between the two and would rather avoid nitpicking something as trivial as this when compared to the rest of the patch, especially considering that I would be equally happy with your version.

I think the whole thing is a little bit of a moot point, though, because by far the thing that is least clear here is that setting MSG has the side-effect of modifying the behavior of the fake-editor that we set up earlier in the script. So I don't know that optimizing for clarity in how we setup and write to the "expect" file is the right thing to spend our time on.

Thanks, Taylor