{"thread":{"id":"53921","subject":"Verbose commit message diff not showing changes from pre-commit hook","startedAt":"2020-07-25T14:47:49Z","lastAt":"2020-07-27T18:14:03Z","messageCount":6,"participants":["Maxime Louet","Junio C Hamano","René Scharfe","Paolo Bonzini"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"402042","messageId":"CADv3qkGq3jA8iXsjhrqfsUX=gW+KOuLyeVgDzmku1tUpsMdvtw@mail.gmail.com","threadId":"53921","inReplyTo":null,"subject":"Verbose commit message diff not showing changes from pre-commit hook","fromName":"Maxime Louet","fromEmail":"maxime@saumon.io","sentAt":"2020-07-25T14:47:19Z","receivedAt":"2020-07-25T14:47:49Z","isPatch":false,"sender":{"key":"maxime@saumon.io","avatar":null},"body":"Hi,\n\nI'm using git version 2.27.0 on Linux.\n\nI have verbose commits enabled (`git commit --verbose`) and a\nrepository configured to lint code before every commit with a\npre-commit hook. This hook may change files and `git add` them just\nbefore the commit.\n\nHowever, when that hook actually changes files (and `git add`s them\nright away) these changes are not reflected in the commit verbose diff\n(the commented lines below the commit message). The changes *are*\ntaken into account by git, as `git diff --staged` shows — and later\nthe commit info, after actually making the commit. However the\ndisplayed diff in the commit message file is a snapshot of the diff\n*before* the pre-commit hook.\n\nIs this expected behaviour? I find it somehow confusing that the diff\nin the commit message isn't the actual commit diff.\n\nThank you for your help!\n\nKind regards,\n\n-- \nMaxime Louet\n"},{"id":"402043","messageId":"xmqqr1sziqrm.fsf@gitster.c.googlers.com","threadId":"53921","inReplyTo":"CADv3qkGq3jA8iXsjhrqfsUX=gW+KOuLyeVgDzmku1tUpsMdvtw@mail.gmail.com","subject":"Re: Verbose commit message diff not showing changes from pre-commit hook","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-25T15:00:13Z","receivedAt":"2020-07-25T15:00:18Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Maxime Louet writes:\n\n> Is this expected behaviour? I find it somehow confusing that the diff\n> in the commit message isn't the actual commit diff.\n\nSince the designed purpose of pre-commit hook is to examine the\ncontents to be committed and reject the attempt to commit if there\nis something wrong found, and Git does not expect it to munge the\ncontents to be committed, if the hook does so, you would get an\nundefined behaviour. So anything is totally expected at that point.\n\nThanks.\n\n\n"},{"id":"402044","messageId":"xmqqk0yripca.fsf@gitster.c.googlers.com","threadId":"53921","inReplyTo":"xmqqr1sziqrm.fsf@gitster.c.googlers.com","subject":"Re: Verbose commit message diff not showing changes from pre-commit hook","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-25T15:31:01Z","receivedAt":"2020-07-25T15:31:10Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Maxime Louet writes:\n>\n>> Is this expected behaviour? I find it somehow confusing that the diff\n>> in the commit message isn't the actual commit diff.\n>\n> Since the designed purpose of pre-commit hook is to examine the\n> contents to be committed and reject the attempt to commit if there\n> is something wrong found, and Git does not expect it to munge the\n> contents to be committed, if the hook does so, you would get an\n> undefined behaviour. So anything is totally expected at that point.\n\nSorry, I have to take this back.\n\nEven before ec84bd00 (git-commit: Refactor creation of log message.,\n2008-02-05), the code anticipated that pre-commit may touch the index\nand tried to cope with it.\n\nHowever, ec84bd00 moved the place where we re-read the on-disk index\nin the sequence, and updated a message that used to read:\n\n-\t/*\n-\t * Re-read the index as pre-commit hook could have updated it,\n-\t * and write it out as a tree.\n-\t */\n\nto:\n\n+\t/*\n+\t * Re-read the index as pre-commit hook could have updated it,\n+\t * and write it out as a tree. We must do this before we invoke\n+\t * the editor and after we invoke run_status above.\n+\t */\n\nUnfortunately there is no mention of the reason why we \"must\" here.\nI think the \"run_status above\" is what prepared the patch in the log\nmessage template, so it is quite likely that we deliberately did so\nto exclude whatever munging pre-commit does to the index from\nappearing in the patch in the verbose mode. If I have to guess, I\nthink the reason is because pre-commit automation is expected to be\nsome sort of mechanical change and not part of the actual work that\nthe end-user produced, it would become easier to perform the \"final\nreview\" of \"what have I done so far---does everything make sense?\"\nif such \"extra\" changes are excluded.\n\nSo, in short, it is not \"undefined\", but rather it seems to be a\ndesigned behaviour that we are seeing.\n\nThanks.\n"},{"id":"402060","messageId":"a8c19b13-3f8c-6602-24dd-ef58af70d702@web.de","threadId":"53921","inReplyTo":"xmqqk0yripca.fsf@gitster.c.googlers.com","subject":"Re: Verbose commit message diff not showing changes from pre-commit hook","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-07-26T17:41:05Z","receivedAt":"2020-07-26T17:41:28Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 25.07.20 um 17:31 schrieb Junio C Hamano:\n> Junio C Hamano writes:\n>\n>> Maxime Louet writes:\n>>\n>>> Is this expected behaviour? I find it somehow confusing that the diff\n>>> in the commit message isn't the actual commit diff.\n\n> Even before ec84bd00 (git-commit: Refactor creation of log message.,\n> 2008-02-05), the code anticipated that pre-commit may touch the index\n> and tried to cope with it.\n>\n> However, ec84bd00 moved the place where we re-read the on-disk index\n> in the sequence, and updated a message that used to read:\n>\n> -\t/*\n> -\t * Re-read the index as pre-commit hook could have updated it,\n> -\t * and write it out as a tree.\n> -\t */\n>\n> to:\n>\n> +\t/*\n> +\t * Re-read the index as pre-commit hook could have updated it,\n> +\t * and write it out as a tree. We must do this before we invoke\n> +\t * the editor and after we invoke run_status above.\n> +\t */\n\nWhen I read \"refactor\" in the title, I assume that the patch in\nquestion doesn't change user-visible behavior.\n\n> Unfortunately there is no mention of the reason why we \"must\" here.\n\n@Paolo: Do you perhaps remember the reason?\n\n> I think the \"run_status above\" is what prepared the patch in the log\n> message template, so it is quite likely that we deliberately did so\n> to exclude whatever munging pre-commit does to the index from\n> appearing in the patch in the verbose mode. If I have to guess, I\n> think the reason is because pre-commit automation is expected to be\n> some sort of mechanical change and not part of the actual work that\n> the end-user produced, it would become easier to perform the \"final\n> review\" of \"what have I done so far---does everything make sense?\"\n> if such \"extra\" changes are excluded.\n\nCommitters review and sign off changes. Hiding machine-made extra\nchanges from them, that they then implicitly also accept responsibility\nfor sounds questionable to me. The prepare-commit-msg hook might be\na place for such filtering. But git commit showing the full extent of\nchanges (incl. those made by the pre-commit hook) would be a better\ndefault, wouldn't it?\n\nRené\n"},{"id":"402064","messageId":"CADv3qkHK_JO6v_jM1A3kXGnKZweJme53Eq3mSjkX0P3UEO7WqA@mail.gmail.com","threadId":"53921","inReplyTo":"a8c19b13-3f8c-6602-24dd-ef58af70d702@web.de","subject":"Re: Verbose commit message diff not showing changes from pre-commit hook","fromName":"Maxime Louet","fromEmail":"maxime@saumon.io","sentAt":"2020-07-26T19:45:02Z","receivedAt":"2020-07-26T19:45:32Z","isPatch":false,"sender":{"key":"maxime@saumon.io","avatar":null},"body":"Junio C Hamano writes:\n\n> So, in short, it is not \"undefined\", but rather it seems to be a\n> designed behaviour that we are seeing.\n\nThank you for your response and the technical explanation.\n\nRené Scharfe writes:\n\n> Committers review and sign off changes. Hiding machine-made extra\n> changes from them, that they then implicitly also accept responsibility\n> for sounds questionable to me. The prepare-commit-msg hook might be\n> a place for such filtering. But git commit showing the full extent of\n> changes (incl. those made by the pre-commit hook) would be a better\n> default, wouldn't it?\n\nI second this. For me, the commit diff should include the \"real\"/full\ncommit diff. Even if the user didn't actually make some changes, the\ncommit they're about to make will include them, so it's completely\nrelevant to show these changes.\nThat's the behaviour I was expecting, and I was confused that Git\ndidn't behave that way.\nTo me, Git shouldn't really care where changes come from; they are\npart of the commit so must logically be shown in the commit diff while\ncommitting.\n\nThank you,\n\n--\nMaxime Louet\n"},{"id":"402148","messageId":"b5f1769d-5c60-bca4-3f46-e55962fa1805@redhat.com","threadId":"53921","inReplyTo":"a8c19b13-3f8c-6602-24dd-ef58af70d702@web.de","subject":"Re: Verbose commit message diff not showing changes from pre-commit hook","fromName":"Paolo Bonzini","fromEmail":"pbonzini@redhat.com","sentAt":"2020-07-27T18:13:54Z","receivedAt":"2020-07-27T18:14:03Z","isPatch":false,"sender":{"key":"pbonzini@redhat.com","avatar":"https://avatars.githubusercontent.com/u/42082?v=4"},"body":"On 26/07/20 19:41, René Scharfe wrote:\n>>\n>> However, ec84bd00 moved the place where we re-read the on-disk index\n>> in the sequence, and updated a message that used to read:\n>>\n>> -\t/*\n>> -\t * Re-read the index as pre-commit hook could have updated it,\n>> -\t * and write it out as a tree.\n>> -\t */\n>>\n>> to:\n>>\n>> +\t/*\n>> +\t * Re-read the index as pre-commit hook could have updated it,\n>> +\t * and write it out as a tree. We must do this before we invoke\n>> +\t * the editor and after we invoke run_status above.\n>> +\t */\n> When I read \"refactor\" in the title, I assume that the patch in\n> question doesn't change user-visible behavior.\n\nThat was probably the intention.\n\n>> Unfortunately there is no mention of the reason why we \"must\" here.\n> @Paolo: Do you perhaps remember the reason?\n\nI think the idea was to use run_status for the \"commitable\" assignment.\n\nPaolo\n\n"}]}