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

From: samuelyvon9@gmail.com <samuelyvon9@gmail.com>
Date: Nov 9, 2021, 03:08 UTC
Message-ID: <20211109030804.261493-1-samuelyvon9@gmail.com>
In-Reply-To: <211109.86wnlicaur.gmgdl@evledraar.gmail.com>

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,

Show 21 quoted lines

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