Re: [PATCH v3 6/7] rebase --continue: refuse to commit after failed command

From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Phillip Wood <phillip.wood@dunelm.org.uk>
Cc: Phillip Wood via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
	Stefan Haller <lists@haller-berlin.de>,
	Eric Sunshine <sunshine@sunshineco.com>,
	Glen Choo <chooglen@google.com>
Subject: Re: [PATCH v3 6/7] rebase --continue: refuse to commit after failed command
Date: Tue, 5 Sep 2023 13:17:53 +0200 (CEST)	[thread overview]
Message-ID: <87fbc8c9-f42b-b374-fee1-57c58f5e8fc0@gmx.de> (raw)
In-Reply-To: <02c28b26-4658-43c8-b1d1-7f1e09bda609@gmail.com>

Hi Phillip,

On Mon, 4 Sep 2023, Phillip Wood wrote:

> On 23/08/2023 10:01, Johannes Schindelin wrote:
>
> > On Tue, 1 Aug 2023, Phillip Wood via GitGitGadget wrote:
> >
> > > From: Phillip Wood <phillip.wood@dunelm.org.uk>
> > >
> > > If a commit cannot be picked because it would overwrite an untracked
> > > file then "git rebase --continue" should refuse to commit any staged
> > > changes as the commit was not picked. This is implemented by refusing to
> > > commit if the message file is missing. The message file is chosen for
> > > this check because it is only written when "git rebase" stops for the
> > > user to resolve merge conflicts.
> > >
> > > Existing commands that refuse to commit staged changes when continuing
> > > such as a failed "exec" rely on checking for the absence of the author
> > > script in run_git_commit(). This prevents the staged changes from being
> > > committed but prints
> > >
> > >      error: could not open '.git/rebase-merge/author-script' for
> > >      reading
> > >
> > > before the message about not being able to commit. This is confusing to
> > > users and so checking for the message file instead improves the user
> > > experience. The existing test for refusing to commit after a failed exec
> > > is updated to check that we do not print the error message about a
> > > missing author script anymore.
> >
> > I am delighted to see an improvement of the user experience!
> >
> > However, I could imagine that users would still be confused when seeing
> > the advice about staged changes, even if nothing was staged at all.
>
> If nothing is staged then this message wont trigger because is_clean will be
> false.

Ah. I managed to get confused by the first sentence of the commit message
already. You clearly talk about "any staged changes". As in "*iff* there
are any staged changes". Which I missed.

A further contributing factor for my misunderstading was the slightly
convoluted logic where `is_clean` is set to true if there are _not_ any
uncommitted changes, and then we ask if `is_clean` is _not_ true. Reminds
me of Smullyan's Knights & Knaves [*1*].

With your patch, there are now four users of the `is_clean` value, and
all but one of them ask for the negated value.

It's not really the responsibility of this patch series, but I could
imagine that it would be nicer to future readers if a patch was added that
would invert the meaning of that variable and rename it to
`needs_committing`. At least to me, that would make the intention of the
code eminently clearer.

Ciao,
Johannes

Footnote *1*: https://en.wikipedia.org/wiki/Knights_and_Knaves