Verbose commit message diff not showing changes from pre-commit hook

Verbose commit message diff not showing changes from pre-commit hook

[Maxime Louet]

Hi,

I'm using git version 2.27.0 on Linux.

I have verbose commits enabled (`git commit --verbose`) and a
repository configured to lint code before every commit with a
pre-commit hook. This hook may change files and `git add` them just
before the commit.

However, when that hook actually changes files (and `git add`s them
right away) these changes are not reflected in the commit verbose diff
(the commented lines below the commit message). The changes *are*
taken into account by git, as `git diff --staged` shows — and later
the commit info, after actually making the commit. However the
displayed diff in the commit message file is a snapshot of the diff
*before* the pre-commit hook.

Is this expected behaviour? I find it somehow confusing that the diff
in the commit message isn't the actual commit diff.

Thank you for your help!

Kind regards,

-- 
Maxime Louet

Re: Verbose commit message diff not showing changes from pre-commit hook

[Junio C Hamano]

Maxime Louet <maxime@saumon.io> writes:

> Is this expected behaviour? I find it somehow confusing that the diff
> in the commit message isn't the actual commit diff.

Since the designed purpose of pre-commit hook is to examine the
contents to be committed and reject the attempt to commit if there
is something wrong found, and Git does not expect it to munge the
contents to be committed, if the hook does so, you would get an
undefined behaviour.  So anything is totally expected at that point.

Thanks.

Re: Verbose commit message diff not showing changes from pre-commit hook

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Maxime Louet <maxime@saumon.io> writes:
>
>> Is this expected behaviour? I find it somehow confusing that the diff
>> in the commit message isn't the actual commit diff.
>
> Since the designed purpose of pre-commit hook is to examine the
> contents to be committed and reject the attempt to commit if there
> is something wrong found, and Git does not expect it to munge the
> contents to be committed, if the hook does so, you would get an
> undefined behaviour.  So anything is totally expected at that point.

Sorry, I have to take this back.

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.
I think the "run_status above" is what prepared the patch in the log
message template, so it is quite likely that we deliberately did so
to exclude whatever munging pre-commit does to the index from
appearing in the patch in the verbose mode.  If I have to guess, I
think the reason is because pre-commit automation is expected to be
some sort of mechanical change and not part of the actual work that
the end-user produced, it would become easier to perform the "final
review" of "what have I done so far---does everything make sense?"
if such "extra" changes are excluded.

So, in short, it is not "undefined", but rather it seems to be a
designed behaviour that we are seeing.

Thanks.

Re: Verbose commit message diff not showing changes from pre-commit hook

[René Scharfe]

Am 25.07.20 um 17:31 schrieb Junio C Hamano:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Maxime Louet <maxime@saumon.io> writes:
>>
>>> Is this expected behaviour? I find it somehow confusing that the diff
>>> in the commit message isn't the actual commit diff.

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

When I read "refactor" in the title, I assume that the patch in
question doesn't change user-visible behavior.

> Unfortunately there is no mention of the reason why we "must" here.

@Paolo: Do you perhaps remember the reason?

> I think the "run_status above" is what prepared the patch in the log
> message template, so it is quite likely that we deliberately did so
> to exclude whatever munging pre-commit does to the index from
> appearing in the patch in the verbose mode.  If I have to guess, I
> think the reason is because pre-commit automation is expected to be
> some sort of mechanical change and not part of the actual work that
> the end-user produced, it would become easier to perform the "final
> review" of "what have I done so far---does everything make sense?"
> if such "extra" changes are excluded.

Committers review and sign off changes.  Hiding machine-made extra
changes from them, that they then implicitly also accept responsibility
for sounds questionable to me.  The prepare-commit-msg hook might be
a place for such filtering.  But git commit showing the full extent of
changes (incl. those made by the pre-commit hook) would be a better
default, wouldn't it?

René

Re: Verbose commit message diff not showing changes from pre-commit hook

[Maxime Louet]

Junio C Hamano <gitster@pobox.com> writes:

> So, in short, it is not "undefined", but rather it seems to be a
> designed behaviour that we are seeing.

Thank you for your response and the technical explanation.

René Scharfe <l.s.r@web.de> writes:

> Committers review and sign off changes.  Hiding machine-made extra
> changes from them, that they then implicitly also accept responsibility
> for sounds questionable to me.  The prepare-commit-msg hook might be
> a place for such filtering.  But git commit showing the full extent of
> changes (incl. those made by the pre-commit hook) would be a better
> default, wouldn't it?

I second this. For me, the commit diff should include the "real"/full
commit diff. Even if the user didn't actually make some changes, the
commit they're about to make will include them, so it's completely
relevant to show these changes.
That's the behaviour I was expecting, and I was confused that Git
didn't behave that way.
To me, Git shouldn't really care where changes come from; they are
part of the commit so must logically be shown in the commit diff while
committing.

Thank you,

--
Maxime Louet

Re: Verbose commit message diff not showing changes from pre-commit hook

[Paolo Bonzini]

On 26/07/20 19:41, René Scharfe wrote:
>>
>> 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.
>> +	 */
> When I read "refactor" in the title, I assume that the patch in
> question doesn't change user-visible behavior.

That was probably the intention.

>> Unfortunately there is no mention of the reason why we "must" here.
> @Paolo: Do you perhaps remember the reason?

I think the idea was to use run_status for the "commitable" assignment.

Paolo