Re: [PATCH v2] builtin-commit: re-read file index before run_status

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 12, 2021, 23:23 UTC
Message-ID: <xmqqpmr5nekx.fsf@gitster.g>
In-Reply-To: <pull.1127.v2.git.git.1636653331034.gitgitgadget@gmail.com>

"Samuel Yvon via GitGitGadget" <gitgitgadget@gmail.com> writes:

> The comment suggesting that the cache must be reset after run_status
> and before the editor being launched was added in ec84bd00,
> (git-commit: Refactor creation of log message., 2008-02-05). It is
> unclear why the run_status must be called *after* the cache reset.

An older thread cited earlier suspected that it is to reflect the changes given to "git commit" proper, excluding whatever pre-commit did, to the return value of run_status(), which becomes the value of committable variable to answer "do we have anything to commit?".

And moving the call would affect both the contents of the status buffer (i.e. the list of paths got changed starts including what pre-commit did) and the "committable" bit by counting such a change as a true change, avoiding the "no empty commit by default" check, in a consistent way, hopefully. I wonder if we have test to demonstrate that, and if there isn't perhaps we would want to add one.

> However, calling run_status after the cache reset does not update
> the status line to state of the current index in the case a
> pre-commit hook is ran and changes files in the staging area.

And if this change also affects the "committable" assignment in a consistent way, it should probably want to be mentioned in this paragraph, too.

I am not convinced by the claim that there is no need for careful transition plans (yet), but I personally agree with the end state (with the above suggested tweaks, that is).

Thanks for working on the topic.