Re: [PATCH] builtin-commit: re-read file index before launching editor

From: Samuel Yvon <samuelyvon9@gmail.com>
Date: Nov 9, 2021, 15:22 UTC
Message-ID: <20211109152219.11037-1-samuelyvon9@gmail.com>
In-Reply-To: <211109.868rxxvgdi.gmgdl@evledraar.gmail.com>

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> *nod*, the implicit suggestion here being: Let's put more of that
> summary into the commit message. It helps when looking up/discovering
> older behavior.

Sorry! I will prepare an amended patch later. Exploring the linked commits has raised more questions on my end.

Show 5 quoted lines

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> The comment was first added in 2888605c649 (builtin-commit: fix
>         partial-commit support, 2007-11-18), and quite suspicuous in timing is
> f5bbc3225c4 (Port git commit to C., 2007-11-08) where we moved from
> git-commit.sh.

Exploring these commits, I fear I might have been confused; the editor was always launched after the reset (still today). The problem, as noted earlier, is the fact that the run_status is ran before the cache reset, which creates the outdated diff. I can't speculate to the intent of the author, but exploring the code makes this comment:

Show 5 quoted lines

> +    /*
> +     * Re-read the index as pre-commit hook could have updated it,
> +     * and write it out as a tree.  We must do this before we invoke
> +     * the editor and after we invoke run_status above.
> +     */

more confusing, because there is no point of invoking the editor after the reset if the status is written before, since the editor won't show anything different.

On one hand, flushing then showing the editor seems to indicate that we want the editor to be up-to-date, but because the status is prepared before the flushing, maybe not?

While it seems the current behaviour has been the behaviour since the start, I am inclined to continue pushing for this change. Unless I am missing something, the comment is contradictory and we should be coherent with the idea of accepting changes made within the pre-commit hook, as noted in https://lore.kernel.org/git/xmqqk0yripca.fsf@gitster.c.googlers.com/t/#u:

>> Junio C Hamano <gitster@pobox.com> writes:
>> Even before ec84bd00 (git-commit: Refactor creation of log message.,
>> 2008-02-05), the code anticipated that pre-commit may touch the index
>> and tried to cope with it.