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@xxxxxxxxxxxxx> > > > > > > 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
