# Verbose commit message diff not showing changes from pre-commit hook 6 messages from 2020-07-25 to 2020-07-27. Participants: Maxime Louet, Junio C Hamano, René Scharfe, Paolo Bonzini. Thread: https://gitlist.dev/t/53921 ## Maxime Louet, 2020-07-25 14:47 Subject: Verbose commit message diff not showing changes from pre-commit hook Message-ID: URL: https://gitlist.dev/e/CADv3qkGq3jA8iXsjhrqfsUX%3DgW%2BKOuLyeVgDzmku1tUpsMdvtw%40mail.gmail.com ``` 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 ``` ## Junio C Hamano, 2020-07-25 15:00 Subject: Re: Verbose commit message diff not showing changes from pre-commit hook Message-ID: URL: https://gitlist.dev/e/xmqqr1sziqrm.fsf%40gitster.c.googlers.com In-Reply-To: ``` Maxime Louet 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. ``` ## Junio C Hamano, 2020-07-25 15:31 Subject: Re: Verbose commit message diff not showing changes from pre-commit hook Message-ID: URL: https://gitlist.dev/e/xmqqk0yripca.fsf%40gitster.c.googlers.com In-Reply-To: ``` Junio C Hamano writes: > Maxime Louet 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. ``` ## René Scharfe, 2020-07-26 17:41 Subject: Re: Verbose commit message diff not showing changes from pre-commit hook Message-ID: URL: https://gitlist.dev/e/a8c19b13-3f8c-6602-24dd-ef58af70d702%40web.de In-Reply-To: ``` Am 25.07.20 um 17:31 schrieb Junio C Hamano: > Junio C Hamano writes: > >> Maxime Louet 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é ``` ## Maxime Louet, 2020-07-26 19:45 Subject: Re: Verbose commit message diff not showing changes from pre-commit hook Message-ID: URL: https://gitlist.dev/e/CADv3qkHK_JO6v_jM1A3kXGnKZweJme53Eq3mSjkX0P3UEO7WqA%40mail.gmail.com In-Reply-To: ``` Junio C Hamano 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 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 ``` ## Paolo Bonzini, 2020-07-27 18:13 Subject: Re: Verbose commit message diff not showing changes from pre-commit hook Message-ID: URL: https://gitlist.dev/e/b5f1769d-5c60-bca4-3f46-e55962fa1805%40redhat.com In-Reply-To: ``` 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 ```