git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* Verbose commit message diff not showing changes from pre-commit hook
@ 2020-07-25 14:47 Maxime Louet
  2020-07-25 15:00 ` Junio C Hamano
  0 siblings, 1 reply; 6+ messages in thread
From: Maxime Louet @ 2020-07-25 14:47 UTC (permalink / raw)
  To: git

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Verbose commit message diff not showing changes from pre-commit hook
  2020-07-25 14:47 Verbose commit message diff not showing changes from pre-commit hook Maxime Louet
@ 2020-07-25 15:00 ` Junio C Hamano
  2020-07-25 15:31   ` Junio C Hamano
  0 siblings, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2020-07-25 15:00 UTC (permalink / raw)
  To: Maxime Louet; +Cc: git

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.



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Verbose commit message diff not showing changes from pre-commit hook
  2020-07-25 15:00 ` Junio C Hamano
@ 2020-07-25 15:31   ` Junio C Hamano
  2020-07-26 17:41     ` René Scharfe
  0 siblings, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2020-07-25 15:31 UTC (permalink / raw)
  To: Maxime Louet; +Cc: git

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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Verbose commit message diff not showing changes from pre-commit hook
  2020-07-25 15:31   ` Junio C Hamano
@ 2020-07-26 17:41     ` René Scharfe
  2020-07-26 19:45       ` Maxime Louet
  2020-07-27 18:13       ` Paolo Bonzini
  0 siblings, 2 replies; 6+ messages in thread
From: René Scharfe @ 2020-07-26 17:41 UTC (permalink / raw)
  To: Junio C Hamano, Maxime Louet; +Cc: git, Paolo Bonzini

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é

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Verbose commit message diff not showing changes from pre-commit hook
  2020-07-26 17:41     ` René Scharfe
@ 2020-07-26 19:45       ` Maxime Louet
  2020-07-27 18:13       ` Paolo Bonzini
  1 sibling, 0 replies; 6+ messages in thread
From: Maxime Louet @ 2020-07-26 19:45 UTC (permalink / raw)
  To: Junio C Hamano, René Scharfe; +Cc: git, Paolo Bonzini

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Verbose commit message diff not showing changes from pre-commit hook
  2020-07-26 17:41     ` René Scharfe
  2020-07-26 19:45       ` Maxime Louet
@ 2020-07-27 18:13       ` Paolo Bonzini
  1 sibling, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2020-07-27 18:13 UTC (permalink / raw)
  To: René Scharfe, Junio C Hamano, Maxime Louet; +Cc: git

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-07-27 18:14 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-25 14:47 Verbose commit message diff not showing changes from pre-commit hook Maxime Louet
2020-07-25 15:00 ` Junio C Hamano
2020-07-25 15:31   ` Junio C Hamano
2020-07-26 17:41     ` René Scharfe
2020-07-26 19:45       ` Maxime Louet
2020-07-27 18:13       ` Paolo Bonzini

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git