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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Mon, Nov 08 2021, samuelyvon9@gmail.com wrote:

> From: Samuel Yvon <samuelyvon9@gmail.com>
>
>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>> The code you're moving around has a comment which seems to suggest that
>> what you want *is* the desired behavior, i.e. we re-read it before
>> invoking the editor, so we should have the updated diff, but just don't?
>
> My understanding is that it was once the behaviour and has changed over time.
> I am saying this based on
> https://lore.kernel.org/git/xmqqk0yripca.fsf@gitster.c.googlers.com/t/#u
>
> Specifically,
>
>> 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.
>> However, ec84bd00 moved the place where we re-read the on-disk index
>> in the sequence, and updated a message that used to read:
>> 
>> -    /*
>> -     * Re-read the index as pre-commit hook could have updated it,
>> -     * and write it out as a tree.
>> -     */
>> 
>> to:
>> 
>> +    /*
>> +     * 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.
>> +     */
>> 
>> Unfortunately there is no mention of the reason why we "must" here.
>
> Looking at ec84bd00 (git-commit: Refactor creation of log message., 2008-02-05),
> we can see that the editor is launched after the cache has been reset. The only
> part that troubles me is the line in the comment that specifies that "we must do
> this ... after we invoke run_status above". I have tested (with my limited knowledge
> of the internals of git) and it seems to be of no consequence of flushing before
> the call to run_status, but I might be missing something.
>
>> The code you're moving around has a comment which seems to suggest that
>> what you want *is* the desired behavior, i.e. we re-read it before
>> invoking the editor, so we should have the updated diff, but just don't?
>
> I think this is the case (based on the previously linked conversation).

*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.

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.

It's a bit of a pain to build git that old, but my hunch is that perhaps
this was tested with git-commit.sh, where the reading of the index would
be another process.