{"thread":{"id":"56861","subject":"[PATCH] builtin-commit: re-read file index before launching editor","startedAt":"2021-11-09T02:06:31Z","lastAt":"2021-11-18T23:51:31Z","messageCount":18,"participants":["Samuel Yvon via GitGitGadget","Ævar Arnfjörð Bjarmason","samuelyvon9@gmail.com","Samuel Yvon","Johannes Schindelin","Junio C Hamano","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"440719","messageId":"pull.1127.git.git.1636423586620.gitgitgadget@gmail.com","threadId":"56861","inReplyTo":null,"subject":"[PATCH] builtin-commit: re-read file index before launching editor","fromName":"Samuel Yvon via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-11-09T02:06:26Z","receivedAt":"2021-11-09T02:06:31Z","isPatch":true,"sender":{"key":"samuelyvon9@gmail.com","avatar":null},"body":"From: Samuel Yvon \n\nChanges made within a pre-commit hook are not reflected within\nthe editor (for instance, with `git commit --verbose`) because\nthe index is re-read after the editor has been used.\n\nThis has the consequence of not displaying the actual changes made\nby the pre-commit hook, but committing them anyways.\n\nWhile it is often argued that the hook's purpose is not to automatically\nwrite content to the repository, it is acknowledged that using the\npre-commit to apply mechanical fixes on top of the existing changes\nis a supported use case, along with verifying the content.\n\nI think not seeing these mechanical fixes before commiting is incorrect.\nA developer might expect the commit to look a certain way but if the\npre-commit goes wrong the invalid changes will go unnoticed until\ncommitted.\n\nSigned-off-by: Samuel Yvon \n---\n builtin-commit: Re-read file index before launching editor\n \n Changes made within a pre-commit hook are not reflected within the\n editor (for instance, with git commit --verbose) because the index is\n re-read after the editor has been used.\n \n This has the consequence of not displaying the actual changes made by\n the pre-commit hook, but committing them anyways.\n \n While it is often argued that the hook's purpose is not to automatically\n write content to the repository, it is acknowledged that using the\n pre-commit to apply mechanical fixes on top of the existing changes is a\n supported use case, along with verifying the content.\n \n I think not seeing these mechanical fixes before commiting is incorrect.\n A developer might expect the commit to look a certain way but if the\n pre-commit goes wrong the invalid changes will go unnoticed until\n committed.\n \n I had a small exchange in the Google Group before submitting this Pr.\n Here is a link for cross-referencing:\n https://groups.google.com/g/git-mentoring/c/FsP83I9mN6c\n \n As a side note, I do not know who manages the Github Repo but the\n following description threw me off a little bit:\n \n Git Source Code Mirror - This is a publish-only repository and all pull requests are ignored. \n \n \n since after looking deeper it seems the PRs are not ignored.\n \n Signed-off-by: Samuel Yvon samuelyvon9@gmail.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1127%2FSamuelYvon%2Fmaint-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1127/SamuelYvon/maint-v1\nPull-Request: https://github.com/git/git/pull/1127\n\n builtin/commit.c | 21 +++++++++++----------\n 1 file changed, 11 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 7c9b1e7be3a..e75b11d1c60 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -728,8 +728,17 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t/* This checks and barfs if author is badly specified */\n \tdetermine_author_info(author_ident);\n \n-\tif (!no_verify && run_commit_hook(use_editor, index_file, \"pre-commit\", NULL))\n-\t\treturn 0;\n+\tif (!no_verify && find_hook(\"pre-commit\")) {\n+\t\tif(run_commit_hook(use_editor, index_file, \"pre-commit\", NULL))\n+\t\t\treturn 0;\n+\n+\t\t/*\n+\t\t * Re-read the index as pre-commit hook could have updated it,\n+\t\t * and write it out as a tree.\n+\t\t */\n+\t\tdiscard_cache();\n+\t\tread_cache_from(index_file);\n+\t}\n \n \tif (squash_message) {\n \t\t/*\n@@ -1051,14 +1060,6 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\treturn 0;\n \t}\n \n-\tif (!no_verify && find_hook(\"pre-commit\")) {\n-\t\t/*\n-\t\t * Re-read the index as pre-commit hook could have updated it,\n-\t\t * and write it out as a tree. We must do this before we invoke\n-\t\t * the editor and after we invoke run_status above.\n-\t\t */\n-\t\tdiscard_cache();\n-\t}\n \tread_cache_from(index_file);\n \n \tif (update_main_cache_tree(0)) {\n\nbase-commit: 5fbd2fc5997dfa4d4593a862fe729b1e7a89bcf8\n-- \ngitgitgadget\n"},{"id":"440723","messageId":"211109.86wnlicaur.gmgdl@evledraar.gmail.com","threadId":"56861","inReplyTo":"pull.1127.git.git.1636423586620.gitgitgadget@gmail.com","subject":"Re: [PATCH] builtin-commit: re-read file index before launching editor","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-09T02:32:34Z","receivedAt":"2021-11-09T02:38:09Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Nov 09 2021, Samuel Yvon via GitGitGadget wrote:\n\n> From: Samuel Yvon \n>\n> Changes made within a pre-commit hook are not reflected within\n> the editor (for instance, with `git commit --verbose`) because\n> the index is re-read after the editor has been used.\n>\n> This has the consequence of not displaying the actual changes made\n> by the pre-commit hook, but committing them anyways.\n>\n> While it is often argued that the hook's purpose is not to automatically\n> write content to the repository, it is acknowledged that using the\n> pre-commit to apply mechanical fixes on top of the existing changes\n> is a supported use case, along with verifying the content.\n>\n> I think not seeing these mechanical fixes before commiting is incorrect.\n> A developer might expect the commit to look a certain way but if the\n> pre-commit goes wrong the invalid changes will go unnoticed until\n> committed.\n>\n> Signed-off-by: Samuel Yvon \n> ---\n> builtin-commit: Re-read file index before launching editor\n> \n> Changes made within a pre-commit hook are not reflected within the\n> editor (for instance, with git commit --verbose) because the index is\n> re-read after the editor has been used.\n> \n> This has the consequence of not displaying the actual changes made by\n> the pre-commit hook, but committing them anyways.\n> \n> While it is often argued that the hook's purpose is not to automatically\n> write content to the repository, it is acknowledged that using the\n> pre-commit to apply mechanical fixes on top of the existing changes is a\n> supported use case, along with verifying the content.\n> \n> I think not seeing these mechanical fixes before commiting is incorrect.\n> A developer might expect the commit to look a certain way but if the\n> pre-commit goes wrong the invalid changes will go unnoticed until\n> committed.\n> \n> I had a small exchange in the Google Group before submitting this Pr.\n> Here is a link for cross-referencing:\n> https://groups.google.com/g/git-mentoring/c/FsP83I9mN6c\n> \n> As a side note, I do not know who manages the Github Repo but the\n> following description threw me off a little bit:\n> \n> Git Source Code Mirror - This is a publish-only repository and all pull requests are ignored. \n> \n> \n> since after looking deeper it seems the PRs are not ignored.\n> \n> Signed-off-by: Samuel Yvon samuelyvon9@gmail.com\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1127%2FSamuelYvon%2Fmaint-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1127/SamuelYvon/maint-v1\n> Pull-Request: https://github.com/git/git/pull/1127\n>\n> builtin/commit.c | 21 +++++++++++----------\n> 1 file changed, 11 insertions(+), 10 deletions(-)\n>\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 7c9b1e7be3a..e75b11d1c60 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -728,8 +728,17 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n> \t/* This checks and barfs if author is badly specified */\n> \tdetermine_author_info(author_ident);\n> \n> -\tif (!no_verify && run_commit_hook(use_editor, index_file, \"pre-commit\", NULL))\n> -\t\treturn 0;\n> +\tif (!no_verify && find_hook(\"pre-commit\")) {\n> +\t\tif(run_commit_hook(use_editor, index_file, \"pre-commit\", NULL))\n> +\t\t\treturn 0;\n> +\n> +\t\t/*\n> +\t\t * Re-read the index as pre-commit hook could have updated it,\n> +\t\t * and write it out as a tree.\n> +\t\t */\n> +\t\tdiscard_cache();\n> +\t\tread_cache_from(index_file);\n> +\t}\n> \n> \tif (squash_message) {\n> \t\t/*\n> @@ -1051,14 +1060,6 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n> \t\treturn 0;\n> \t}\n> \n> -\tif (!no_verify && find_hook(\"pre-commit\")) {\n> -\t\t/*\n> -\t\t * Re-read the index as pre-commit hook could have updated it,\n> -\t\t * and write it out as a tree. We must do this before we invoke\n> -\t\t * the editor and after we invoke run_status above.\n> -\t\t */\n> -\t\tdiscard_cache();\n> -\t}\n> \tread_cache_from(index_file);\n> \n> \tif (update_main_cache_tree(0)) {\n>\n> base-commit: 5fbd2fc5997dfa4d4593a862fe729b1e7a89bcf8\n\nThis hook logic is actively being changed these days, we're currently at\nbatch 2/3 of 3/3 (or was it 4/4? I forget) to rewrite all these hook\ninterfaces.\n\nI haven't looked carefully at this, but I've got a patch that touches\nthis exact logic here that'll be in-flight before too long:\nhttps://lore.kernel.org/git/patch-v5-36.36-fe056098534-20210902T125111Z-avarab@gmail.com/\n\nAnyway, what you have here seems orthagonal, and it looks like these two\ncould be combined, but on top of my linked patch we wouldn't be racy\nwith the new behavior either.\n\nBut (and I've got to run) check out the commit message of that patch, perhaps it links to useful past discussions/commits.\n\nI.e. a thing not covered in your commit message is a discussion of if\nthe current behavior was explicitly desired by anyone, or if we just\nended up with it by accident.\n\nThe code you're moving around has a comment which seems to suggest that\nwhat you want *is* the desired behavior, i.e. we re-read it before\ninvoking the editor, so we should have the updated diff, but just don't?\n\nOr maybe I'm misunderstanding it.\n"},{"id":"440728","messageId":"20211109030804.261493-1-samuelyvon9@gmail.com","threadId":"56861","inReplyTo":"211109.86wnlicaur.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] builtin-commit: re-read file index before launching editor","fromName":"","fromEmail":"samuelyvon9@gmail.com","sentAt":"2021-11-09T03:08:04Z","receivedAt":"2021-11-09T03:08:36Z","isPatch":true,"sender":{"key":"samuelyvon9@gmail.com","avatar":null},"body":"From: Samuel Yvon \n\n> Ævar Arnfjörð Bjarmason writes:\n> The code you're moving around has a comment which seems to suggest that\n> what you want *is* the desired behavior, i.e. we re-read it before\n> invoking the editor, so we should have the updated diff, but just don't?\n\nMy understanding is that it was once the behaviour and has changed over time.\nI am saying this based on\nhttps://lore.kernel.org/git/xmqqk0yripca.fsf@gitster.c.googlers.com/t/#u\n\nSpecifically,\n\n> Junio C Hamano writes:\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> 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> - /*\n> - * Re-read the index as pre-commit hook could have updated it,\n> - * and write it out as a tree.\n> - */\n> \n> to:\n> \n> + /*\n> + * Re-read the index as pre-commit hook could have updated it,\n> + * and write it out as a tree. We must do this before we invoke\n> + * the editor and after we invoke run_status above.\n> + */\n> \n> Unfortunately there is no mention of the reason why we \"must\" here.\n\nLooking at ec84bd00 (git-commit: Refactor creation of log message., 2008-02-05),\nwe can see that the editor is launched after the cache has been reset. The only\npart that troubles me is the line in the comment that specifies that \"we must do\nthis ... after we invoke run_status above\". I have tested (with my limited knowledge\nof the internals of git) and it seems to be of no consequence of flushing before\nthe call to run_status, but I might be missing something.\n\n> The code you're moving around has a comment which seems to suggest that\n> what you want *is* the desired behavior, i.e. we re-read it before\n> invoking the editor, so we should have the updated diff, but just don't?\n\nI think this is the case (based on the previously linked conversation).\n"},{"id":"440733","messageId":"211109.868rxxvgdi.gmgdl@evledraar.gmail.com","threadId":"56861","inReplyTo":"20211109030804.261493-1-samuelyvon9@gmail.com","subject":"Re: [PATCH] builtin-commit: re-read file index before launching editor","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-09T09:11:04Z","receivedAt":"2021-11-09T09:16:15Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Nov 08 2021, samuelyvon9@gmail.com wrote:\n\n> From: Samuel Yvon \n>\n>> Ævar Arnfjörð Bjarmason writes:\n>> The code you're moving around has a comment which seems to suggest that\n>> what you want *is* the desired behavior, i.e. we re-read it before\n>> invoking the editor, so we should have the updated diff, but just don't?\n>\n> My understanding is that it was once the behaviour and has changed over time.\n> I am saying this based on\n> https://lore.kernel.org/git/xmqqk0yripca.fsf@gitster.c.googlers.com/t/#u\n>\n> Specifically,\n>\n>> Junio C Hamano writes:\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>> 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>> - /*\n>> - * Re-read the index as pre-commit hook could have updated it,\n>> - * and write it out as a tree.\n>> - */\n>> \n>> to:\n>> \n>> + /*\n>> + * Re-read the index as pre-commit hook could have updated it,\n>> + * and write it out as a tree. We must do this before we invoke\n>> + * the editor and after we invoke run_status above.\n>> + */\n>> \n>> Unfortunately there is no mention of the reason why we \"must\" here.\n>\n> Looking at ec84bd00 (git-commit: Refactor creation of log message., 2008-02-05),\n> we can see that the editor is launched after the cache has been reset. The only\n> part that troubles me is the line in the comment that specifies that \"we must do\n> this ... after we invoke run_status above\". I have tested (with my limited knowledge\n> of the internals of git) and it seems to be of no consequence of flushing before\n> the call to run_status, but I might be missing something.\n>\n>> The code you're moving around has a comment which seems to suggest that\n>> what you want *is* the desired behavior, i.e. we re-read it before\n>> invoking the editor, so we should have the updated diff, but just don't?\n>\n> I think this is the case (based on the previously linked conversation).\n\n*nod*, the implicit suggestion here being: Let's put more of that\nsummary into the commit message. It helps when looking up/discovering\nolder behavior.\n\nThe comment was first added in 2888605c649 (builtin-commit: fix\npartial-commit support, 2007-11-18), and quite suspicuous in timing is\nf5bbc3225c4 (Port git commit to C., 2007-11-08) where we moved from\ngit-commit.sh.\n\nIt's a bit of a pain to build git that old, but my hunch is that perhaps\nthis was tested with git-commit.sh, where the reading of the index would\nbe another process.\n"},{"id":"440740","messageId":"20211109152219.11037-1-samuelyvon9@gmail.com","threadId":"56861","inReplyTo":"211109.868rxxvgdi.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] builtin-commit: re-read file index before launching editor","fromName":"Samuel Yvon","fromEmail":"samuelyvon9@gmail.com","sentAt":"2021-11-09T15:22:19Z","receivedAt":"2021-11-09T15:22:38Z","isPatch":true,"sender":{"key":"samuelyvon9@gmail.com","avatar":null},"body":"> Ævar Arnfjörð Bjarmason writes:\n> *nod*, the implicit suggestion here being: Let's put more of that\n> summary into the commit message. It helps when looking up/discovering\n> older behavior.\n\nSorry! I will prepare an amended patch later. Exploring the linked commits\nhas raised more questions on my end.\n\n> Ævar Arnfjörð Bjarmason writes:\n> The comment was first added in 2888605c649 (builtin-commit: fix\n> partial-commit support, 2007-11-18), and quite suspicuous in timing is\n> f5bbc3225c4 (Port git commit to C., 2007-11-08) where we moved from\n> git-commit.sh.\n\nExploring these commits, I fear I might have been confused; the editor was\nalways launched after the reset (still today). The problem, as noted earlier,\nis the fact that the run_status is ran before the cache reset, which\ncreates the outdated diff. I can't speculate to the intent of the\nauthor, but exploring the code makes this comment:\n\n> + /*\n> + * Re-read the index as pre-commit hook could have updated it,\n> + * and write it out as a tree. We must do this before we invoke\n> + * the editor and after we invoke run_status above.\n> + */\n\nmore confusing, because there is no point of invoking the editor after the reset\nif the status is written before, since the editor won't show anything different.\n\nOn one hand, flushing then showing the editor seems to indicate that we want the\neditor to be up-to-date, but because the status is prepared before the flushing,\nmaybe not?\n\nWhile it seems the current behaviour has been the behaviour since the start,\nI am inclined to continue pushing for this change. Unless I am missing something,\nthe comment is contradictory and we should be coherent with the idea of\naccepting changes made within the pre-commit hook, as noted in \nhttps://lore.kernel.org/git/xmqqk0yripca.fsf@gitster.c.googlers.com/t/#u:\n\n>> Junio C Hamano writes:\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"},{"id":"440749","messageId":"nycvar.QRO.7.76.6.2111091732040.54@tvgsbejvaqbjf.bet","threadId":"56861","inReplyTo":"pull.1127.git.git.1636423586620.gitgitgadget@gmail.com","subject":"Description of github.com/git/git, was Re: [PATCH] builtin-commit: re-read file index before launching editor","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-11-09T16:41:39Z","receivedAt":"2021-11-09T16:41:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Samuel,\n\nOn Tue, 9 Nov 2021, Samuel Yvon via GitGitGadget wrote:\n\n> As a side note, I do not know who manages the Github Repo but the\n> following description threw me off a little bit:\n>\n> Git Source Code Mirror - This is a publish-only repository and all pull requests are ignored.\n\nI would be in favor of changing this description. How about:\n\n\tGit Source Code Mirror - Pull Requests can be submitted via GitGitGadget\n\nThoughts?\n\nCiao,\nDscho\n"},{"id":"440751","messageId":"20211109170102.14538-1-samuelyvon9@gmail.com","threadId":"56861","inReplyTo":"nycvar.QRO.7.76.6.2111091732040.54@tvgsbejvaqbjf.bet","subject":"Re: Description of github.com/git/git, was Re: [PATCH] builtin-commit: re-read file index before launching editor","fromName":"Samuel Yvon","fromEmail":"samuelyvon9@gmail.com","sentAt":"2021-11-09T17:01:02Z","receivedAt":"2021-11-09T17:01:16Z","isPatch":true,"sender":{"key":"samuelyvon9@gmail.com","avatar":null},"body":"Johannes Schindelin writes:\n\n> I would be in favor of changing this description. How about:\n> Git Source Code Mirror - Pull Requests can be submitted via GitGitGadget\n> Thoughts?\n\nI think it might be somewhat obscure for newcomers (What is GitGitGadget?).\nI know Github supports links in project descriptions. Maybe\n\n Git Source Code Mirror - Please read https://gitgitgadget.github.io/ before submitting Pull Requests\n\nThis would have helped me figure out if submitting a PR was gonna end up nowhere or not.\n\n\n\n"},{"id":"440759","messageId":"xmqqbl2t2n2p.fsf@gitster.g","threadId":"56861","inReplyTo":"20211109152219.11037-1-samuelyvon9@gmail.com","subject":"Re: [PATCH] builtin-commit: re-read file index before launching editor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-09T18:36:30Z","receivedAt":"2021-11-09T18:36:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Samuel Yvon writes:\n\n> On one hand, flushing then showing the editor seems to indicate that we want the\n> editor to be up-to-date, but because the status is prepared before the flushing,\n> maybe not?\n>\n> While it seems the current behaviour has been the behaviour since the start,\n> I am inclined to continue pushing for this change. Unless I am missing something,\n> the comment is contradictory and we should be coherent with the idea of\n> accepting changes made within the pre-commit hook, as noted in \n> https://lore.kernel.org/git/xmqqk0yripca.fsf@gitster.c.googlers.com/t/#u:\n>\n>>> Junio C Hamano writes:\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\nYou seem to be quoting the thread over and over, but what you are\nquoting is somewhat different from the concluding part of what I\nsaid, which was:\n\n If I have to guess, I think the reason is because pre-commit\n automation is expected to be some sort of mechanical change and\n not part of the actual work that the end-user produced, it would\n become easier to perform the \"final review\" of \"what have I done\n so far---does everything make sense?\" if such \"extra\" changes\n are excluded.\n\n So, in short, it is not \"undefined\", but rather it seems to be a\n designed behaviour that we are seeing.\n\nI do not personally mind if we change the philosophy but because it\nhas been a longstanding designed behaviour, it may need a careful\ntransition plan.\n"},{"id":"440761","messageId":"xmqq5yt12lta.fsf@gitster.g","threadId":"56861","inReplyTo":"nycvar.QRO.7.76.6.2111091732040.54@tvgsbejvaqbjf.bet","subject":"Re: Description of github.com/git/git, was Re: [PATCH] builtin-commit: re-read file index before launching editor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-09T19:03:45Z","receivedAt":"2021-11-09T19:03:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> I would be in favor of changing this description. How about:\n>\n> \tGit Source Code Mirror - Pull Requests can be submitted via GitGitGadget\n\nPresense of GGG does not change the fact that the repository is\npublish only. But \"all pull requests are ignored\" is not correct\nanymore.\n\n Git Source Code Mirror - This is a publish-only repository but\n pull requests can be turned into patches to the mailing list via\n GitGitGadget. Please follow Documentation/SubmittingPatches\n procedure for any of your improvements.\n\nI didn't double-check but I presume SubmittingPatches these days\ntalk about GGG?\n"},{"id":"440762","messageId":"YYrKtHJzapNdpXCF@nand.local","threadId":"56861","inReplyTo":"xmqq5yt12lta.fsf@gitster.g","subject":"Re: Description of github.com/git/git, was Re: [PATCH] builtin-commit: re-read file index before launching editor","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-11-09T19:23:32Z","receivedAt":"2021-11-09T19:23:36Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Nov 09, 2021 at 11:03:45AM -0800, Junio C Hamano wrote:\n> Johannes Schindelin writes:\n>\n> > I would be in favor of changing this description. How about:\n> >\n> > \tGit Source Code Mirror - Pull Requests can be submitted via GitGitGadget\n>\n> Presense of GGG does not change the fact that the repository is\n> publish only. But \"all pull requests are ignored\" is not correct\n> anymore.\n>\n> Git Source Code Mirror - This is a publish-only repository but\n> pull requests can be turned into patches to the mailing list via\n> GitGitGadget. Please follow Documentation/SubmittingPatches\n> procedure for any of your improvements.\n>\n> I didn't double-check but I presume SubmittingPatches these days\n> talk about GGG?\n\nI don't think so. Documentation/MyFirstContribution.txt does as of\n76644e3268 (documentation: add tutorial for first contribution,\n2019-05-17), but the same treatment was not applied elsewhere.\n\nI would not be sad to see a reference to GGG be made in\nSubmittingPatches.\n\nThanks,\nTaylor\n"},{"id":"440763","messageId":"20211109192707.19916-1-samuelyvon9@gmail.com","threadId":"56861","inReplyTo":"xmqq5yt12lta.fsf@gitster.g","subject":"Re: Description of github.com/git/git, was Re: [PATCH] builtin-commit: re-read file index before launching editor","fromName":"Samuel Yvon","fromEmail":"samuelyvon9@gmail.com","sentAt":"2021-11-09T19:27:07Z","receivedAt":"2021-11-09T19:27:13Z","isPatch":true,"sender":{"key":"samuelyvon9@gmail.com","avatar":null},"body":"Junio C Hamano writes:\n\n> I didn't double-check but I presume SubmittingPatches these days\n> talk about GGG?\n\nIt does not, but it links to Documentation/MyFirstContribution,\nwhich does. \n\nJunio C Hamano writes:\n\n> Git Source Code Mirror - This is a publish-only repository but\n> pull requests can be turned into patches to the mailing list via\n> GitGitGadget. Please follow Documentation/SubmittingPatches\n> procedure for any of your improvements.\n\nI think this is descriptive enough so that newcomers can find their way.\n"},{"id":"440764","messageId":"20211109200123.27763-1-samuelyvon9@gmail.com","threadId":"56861","inReplyTo":"xmqqbl2t2n2p.fsf@gitster.g","subject":"Re: [PATCH] builtin-commit: re-read file index before launching editor","fromName":"Samuel Yvon","fromEmail":"samuelyvon9@gmail.com","sentAt":"2021-11-09T20:01:23Z","receivedAt":"2021-11-09T20:02:10Z","isPatch":true,"sender":{"key":"samuelyvon9@gmail.com","avatar":null},"body":"Junio C Hamano writes:\n\n> You seem to be quoting the thread over and over, but what you are\n> quoting is somewhat different from the concluding part of what I\n> said, which was:\n>> If I have to guess, I think the reason is because pre-commit\n>> automation is expected to be some sort of mechanical change and\n>> not part of the actual work that the end-user produced, it would\n>> become easier to perform the \"final review\" of \"what have I done\n>> so far---does everything make sense?\" if such \"extra\" changes\n>> are excluded.\n> So, in short, it is not \"undefined\", but rather it seems to be a\n> designed behaviour that we are seeing.\n\nApologies if you feel I misquoted you. I am confused by the comment in the\noriginal code w.r.t the location of the cache reset, which (from my\nunderstanding) is contradictory with the quoted conversation and so I\nfocused on those parts.\n\nJunio C Hamano writes:\n> I do not personally mind if we change the philosophy but because it\n> has been a longstanding designed behaviour, it may need a careful\n> transition plan.\n\nOut of curiosity, what would that involve? \n"},{"id":"440833","messageId":"nycvar.QRO.7.76.6.2111101321300.21127@tvgsbejvaqbjf.bet","threadId":"56861","inReplyTo":"20211109192707.19916-1-samuelyvon9@gmail.com","subject":"Re: Description of github.com/git/git, was Re: [PATCH] builtin-commit: re-read file index before launching editor","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-11-10T12:22:12Z","receivedAt":"2021-11-10T12:22:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 9 Nov 2021, Samuel Yvon wrote:\n\n> Junio C Hamano writes:\n>\n> > Git Source Code Mirror - This is a publish-only repository but\n> > pull requests can be turned into patches to the mailing list via\n> > GitGitGadget. Please follow Documentation/SubmittingPatches\n> > procedure for any of your improvements.\n>\n> I think this is descriptive enough so that newcomers can find their way.\n\nExcellent. Junio had made the adjustment, and I merely added a link to\nGitGitGadget's high-level description.\n\nThank you, all,\nDscho\n"},{"id":"440922","messageId":"pull.1127.v2.git.git.1636653331034.gitgitgadget@gmail.com","threadId":"56861","inReplyTo":"pull.1127.git.git.1636423586620.gitgitgadget@gmail.com","subject":"[PATCH v2] builtin-commit: re-read file index before run_status","fromName":"Samuel Yvon via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-11-11T17:55:30Z","receivedAt":"2021-11-11T17:55:36Z","isPatch":true,"sender":{"key":"samuelyvon9@gmail.com","avatar":null},"body":"From: Samuel Yvon \n\nChanges made within a pre-commit hook are not reflected within\nthe editor (for instance, with `git commit --verbose`) because\nthe index is re-read after the status has been written.\n\nThis has the consequence of not displaying the actual changes made\nby the pre-commit hook, but committing them anyways.\n\nWhile it is often argued that the hook's purpose is not to automatically\nwrite content to the repository, it is acknowledged that using the\npre-commit to apply mechanical fixes on top of the existing changes\nis a supported use case, along with verifying the content.\n\nI think not seeing these mechanical fixes before commiting is incorrect.\nA developer might expect the commit to look a certain way but if the\npre-commit goes wrong the invalid changes will go unnoticed until\ncommitted.\n\nThe comment suggesting that the cache must be reset after run_status\nand before the editor being launched was added in ec84bd00,\n(git-commit: Refactor creation of log message., 2008-02-05). It is\nunclear why the run_status must be called *after* the cache reset.\nHowever, calling run_status after the cache reset does not update\nthe status line to state of the current index in the case a\npre-commit hook is ran and changes files in the staging area.\n\nSigned-off-by: Samuel Yvon \n---\n builtin-commit: re-read file index before run_status\n \n Changes made within a pre-commit hook are not reflected within the\n editor (for instance, with git commit --verbose) because the index is\n re-read after the editor has been used.\n \n This has the consequence of not displaying the actual changes made by\n the pre-commit hook, but committing them anyways.\n \n While it is often argued that the hook's purpose is not to automatically\n write content to the repository, it is acknowledged that using the\n pre-commit to apply mechanical fixes on top of the existing changes is a\n supported use case, along with verifying the content.\n \n I think not seeing these mechanical fixes before commiting is incorrect.\n A developer might expect the commit to look a certain way but if the\n pre-commit goes wrong the invalid changes will go unnoticed until\n committed.\n \n I had a small exchange in the Google Group before submitting this Pr.\n Here is a link for cross-referencing:\n https://groups.google.com/g/git-mentoring/c/FsP83I9mN6c\n \n As a side note, I do not know who manages the Github Repo but the\n following description threw me off a little bit:\n \n Git Source Code Mirror - This is a publish-only repository and all pull requests are ignored. \n \n \n since after looking deeper it seems the PRs are not ignored.\n \n Changes since v1:\n \n * Edited the title to more accurately reflect the changes\n * More details in commit messages\n \n Signed-off-by: Samuel Yvon samuelyvon9@gmail.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1127%2FSamuelYvon%2Fmaint-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1127/SamuelYvon/maint-v2\nPull-Request: https://github.com/git/git/pull/1127\n\nRange-diff vs v1:\n\n 1: 271bcf89d1e ! 1: f09244a0ba3 builtin-commit: re-read file index before launching editor\n @@ Metadata\n Author: Samuel Yvon \n \n ## Commit message ##\n - builtin-commit: re-read file index before launching editor\n + builtin-commit: re-read file index before run_status\n \n Changes made within a pre-commit hook are not reflected within\n the editor (for instance, with `git commit --verbose`) because\n - the index is re-read after the editor has been used.\n + the index is re-read after the status has been written.\n \n This has the consequence of not displaying the actual changes made\n by the pre-commit hook, but committing them anyways.\n @@ Commit message\n pre-commit goes wrong the invalid changes will go unnoticed until\n committed.\n \n + The comment suggesting that the cache must be reset after run_status\n + and before the editor being launched was added in ec84bd00,\n + (git-commit: Refactor creation of log message., 2008-02-05). It is\n + unclear why the run_status must be called *after* the cache reset.\n + However, calling run_status after the cache reset does not update\n + the status line to state of the current index in the case a\n + pre-commit hook is ran and changes files in the staging area.\n +\n Signed-off-by: Samuel Yvon \n \n ## builtin/commit.c ##\n\n\n builtin/commit.c | 21 +++++++++++----------\n 1 file changed, 11 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 7c9b1e7be3a..e75b11d1c60 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -728,8 +728,17 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t/* This checks and barfs if author is badly specified */\n \tdetermine_author_info(author_ident);\n \n-\tif (!no_verify && run_commit_hook(use_editor, index_file, \"pre-commit\", NULL))\n-\t\treturn 0;\n+\tif (!no_verify && find_hook(\"pre-commit\")) {\n+\t\tif(run_commit_hook(use_editor, index_file, \"pre-commit\", NULL))\n+\t\t\treturn 0;\n+\n+\t\t/*\n+\t\t * Re-read the index as pre-commit hook could have updated it,\n+\t\t * and write it out as a tree.\n+\t\t */\n+\t\tdiscard_cache();\n+\t\tread_cache_from(index_file);\n+\t}\n \n \tif (squash_message) {\n \t\t/*\n@@ -1051,14 +1060,6 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\treturn 0;\n \t}\n \n-\tif (!no_verify && find_hook(\"pre-commit\")) {\n-\t\t/*\n-\t\t * Re-read the index as pre-commit hook could have updated it,\n-\t\t * and write it out as a tree. We must do this before we invoke\n-\t\t * the editor and after we invoke run_status above.\n-\t\t */\n-\t\tdiscard_cache();\n-\t}\n \tread_cache_from(index_file);\n \n \tif (update_main_cache_tree(0)) {\n\nbase-commit: 5fbd2fc5997dfa4d4593a862fe729b1e7a89bcf8\n-- \ngitgitgadget\n"},{"id":"440940","messageId":"xmqqtugijqe0.fsf@gitster.g","threadId":"56861","inReplyTo":"20211109200123.27763-1-samuelyvon9@gmail.com","subject":"Re: [PATCH] builtin-commit: re-read file index before launching editor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-11T22:09:43Z","receivedAt":"2021-11-11T22:09:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Samuel Yvon writes:\n\n>> I do not personally mind if we change the philosophy but because it\n>> has been a longstanding designed behaviour, it may need a careful\n>> transition plan.\n>\n> Out of curiosity, what would that involve? \n\nWe need to make sure we do not break workflows of existing users who\nrelies on the current behaviour. \n\nAs Git is widely used, it is very likely that some of the workflow\nelement existing users use depend on the current behaviour. \n\nLetting pre-commit do its thing and letting run_status() compute\n\"commitable\" without its effect may be something they depend on. It\nis the same for any other behaviour we may be tempted to modify. We\nwould be breaking Git for them by changing.\n\nSo, not breaking by not changing behaviour, if we can do so, would\nbe ideal. Introducing a configuration option and hide the new and\ndifferent behaviour behind it, so that only the folks who agree to\ntake responsibility of adjusting to the new behaviour, would be a\ngood way to isolate the existing users from the change.\n\nIf we come up with a new behaviour that would make the world a\nbetter place if adopted by everybody, we might be tempted to\neventually make it the _only_ behaviour available to everybody. I\nam not sure if it is the case here, but if it were, the transition\nplan would become even more involved. We'd need to start by warning\nexisting users (i.e. the code must detect a case where pre-commit\nmucks with the index and having an extra \"re-read index file\" would\nhave made a difference---and tell the user that they must adjust to\nthe change of behaviour in the future), wait for a few years and\nthen flip the behaviour while removing the warning, or something\nlike that.\n\n\n"},{"id":"441038","messageId":"xmqqpmr5nekx.fsf@gitster.g","threadId":"56861","inReplyTo":"pull.1127.v2.git.git.1636653331034.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] builtin-commit: re-read file index before run_status","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-12T23:23:26Z","receivedAt":"2021-11-12T23:23:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Samuel Yvon via GitGitGadget\" writes:\n\n> The comment suggesting that the cache must be reset after run_status\n> and before the editor being launched was added in ec84bd00,\n> (git-commit: Refactor creation of log message., 2008-02-05). It is\n> unclear why the run_status must be called *after* the cache reset.\n\nAn older thread cited earlier suspected that it is to reflect the\nchanges given to \"git commit\" proper, excluding whatever pre-commit\ndid, to the return value of run_status(), which becomes the value of\ncommittable variable to answer \"do we have anything to commit?\".\n\nAnd moving the call would affect both the contents of the status\nbuffer (i.e. the list of paths got changed starts including what\npre-commit did) and the \"committable\" bit by counting such a change\nas a true change, avoiding the \"no empty commit by default\" check,\nin a consistent way, hopefully. I wonder if we have test to\ndemonstrate that, and if there isn't perhaps we would want to add\none.\n\n> However, calling run_status after the cache reset does not update\n> the status line to state of the current index in the case a\n> pre-commit hook is ran and changes files in the staging area.\n\nAnd if this change also affects the \"committable\" assignment in a\nconsistent way, it should probably want to be mentioned in this\nparagraph, too.\n\nI am not convinced by the claim that there is no need for careful\ntransition plans (yet), but I personally agree with the end state\n(with the above suggested tweaks, that is).\n\nThanks for working on the topic.\n"},{"id":"441509","messageId":"20211117164842.36381-1-samuelyvon9@gmail.com","threadId":"56861","inReplyTo":"xmqqpmr5nekx.fsf@gitster.g","subject":"Re: [PATCH v2] builtin-commit: re-read file index before run_status","fromName":"Samuel Yvon","fromEmail":"samuelyvon9@gmail.com","sentAt":"2021-11-17T16:48:42Z","receivedAt":"2021-11-17T16:48:48Z","isPatch":true,"sender":{"key":"samuelyvon9@gmail.com","avatar":null},"body":"Apologies for the time I took to reply,\n\nJunio C Hamano writes:\n> And moving the call would affect both the contents of the status\n> buffer (i.e. the list of paths got changed starts including what\n> pre-commit did) and the \"committable\" bit by counting such a change\n> as a true change, avoiding the \"no empty commit by default\" check,\n> in a consistent way, hopefully. I wonder if we have test to\n> demonstrate that, and if there isn't perhaps we would want to add\n> one.\n\nSo, just to make sure I understand well, the concern is that an empty commit\nwould trigger the commit routine, run the pre-commit hook, which may add files\n(thus making it an non-empty commit) and then push 100% automatic changes to a\nrepo. I agree that this would be invalid behaviour and very odd, I will \nmake sure no empty commit is allowed.\n\nJunio C Hamano writes:\n> Samuel Yvon writes:\n> > However, calling run_status after the cache reset does not update\n> > the status line to state of the current index in the case a\n> > pre-commit hook is ran and changes files in the staging area.\n> \n> And if this change also affects the \"committable\" assignment in a\n> consistent way, it should probably want to be mentioned in this\n> paragraph, too.\n\nWhat do you mean by \"commitable assignment\"? \n\n> I am not convinced by the claim that there is no need for careful\n> transition plans (yet), but I personally agree with the end state\n> (with the above suggested tweaks, that is).\n\nWith the last message, I agree the safest option is probably to leave this\nconfigurable for now and off by default.\n\nSo here's the next steps that I intend to take to get this merged in:\n\n- Add a test for empty commit (if non-existent) and ensure the behaviour is the same\n- Add a config option (or maybe a switch?) for migration purposes, with the default\n being the current behaviour.\n\n\n"},{"id":"441678","messageId":"xmqqv90pt43l.fsf@gitster.g","threadId":"56861","inReplyTo":"20211117164842.36381-1-samuelyvon9@gmail.com","subject":"Re: [PATCH v2] builtin-commit: re-read file index before run_status","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-18T23:51:26Z","receivedAt":"2021-11-18T23:51:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Samuel Yvon writes:\n\n>> And if this change also affects the \"committable\" assignment in a\n>> consistent way, it should probably want to be mentioned in this\n>> paragraph, too.\n>\n> What do you mean by \"commitable assignment\"? \n\nAssignment to the committable variable in the codepath you are\nlooking at, was what I meant.\n\n"}]}