{"thread":{"id":"38035","subject":"[PATCH] commit: inform pre-commit if --amend is used","startedAt":"2014-11-24T11:21:51Z","lastAt":"2014-12-01T00:56:53Z","messageCount":9,"participants":["Øystein Walle","Eric Sunshine","Jeff King","Junio C Hamano","Mark Levedahl"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"252425","messageId":"1416828111-4567-1-git-send-email-oystwa@gmail.com","threadId":"38035","inReplyTo":null,"subject":"[PATCH] commit: inform pre-commit if --amend is used","fromName":"Øystein Walle","fromEmail":"oystwa@gmail.com","sentAt":"2014-11-24T11:21:51Z","receivedAt":"2014-11-24T11:21:51Z","isPatch":true,"sender":{"key":"oystwa@gmail.com","avatar":"https://avatars.githubusercontent.com/u/794585?v=4"},"body":"When a commit is amended a pre-commit hook that verifies the commit's\ncontents might not find what it's looking for if for example it looks at\nthe differences against HEAD when HEAD~1 might be more appropriate.\nInform the commit hook that --amend is being used so that hook authors\ncan do e.g.\n\n if test \"$1\" = amend\n then\n ...\n else\n ...\n fi\n\nto handle these situations.\n\nSigned-off-by: Øystein Walle \n---\n Documentation/githooks.txt | 3 ++-\n builtin/commit.c | 3 ++-\n t/t7503-pre-commit-hook.sh | 14 ++++++++++++++\n 3 files changed, 18 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\nindex 9ef2469..e113027 100644\n--- a/Documentation/githooks.txt\n+++ b/Documentation/githooks.txt\n@@ -73,7 +73,8 @@ pre-commit\n ~~~~~~~~~~\n \n This hook is invoked by 'git commit', and can be bypassed\n-with `--no-verify` option. It takes no parameter, and is\n+with `--no-verify` option. It takes one parameter which is \"amend\" if\n+`--amend` was used when committing and empty otherwise. It is\n invoked before obtaining the proposed commit log message and\n making a commit. Exiting with non-zero status from this script\n causes the 'git commit' to abort.\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex e108c53..e38dd4a 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -694,7 +694,8 @@ 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+\tif (!no_verify && run_commit_hook(use_editor, index_file,\n+\t\t\t\t\t \"pre-commit\", amend ? \"amend\" : \"\", NULL))\n \t\treturn 0;\n \n \tif (squash_message) {\ndiff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-hook.sh\nindex 984889b..be97676 100755\n--- a/t/t7503-pre-commit-hook.sh\n+++ b/t/t7503-pre-commit-hook.sh\n@@ -136,4 +136,18 @@ test_expect_success 'check the author in hook' '\n \tgit show -s\n '\n \n+# a hook that checks if \"amend\" is passed as an argument\n+cat > \"$HOOK\" < wrote:\n> When a commit is amended a pre-commit hook that verifies the commit's\n> contents might not find what it's looking for if for example it looks at\n> the differences against HEAD when HEAD~1 might be more appropriate.\n> Inform the commit hook that --amend is being used so that hook authors\n> can do e.g.\n>\n> if test \"$1\" = amend\n> then\n> ...\n> else\n> ...\n> fi\n>\n> to handle these situations.\n>\n> Signed-off-by: Øystein Walle \n> ---\n> diff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-hook.sh\n> index 984889b..be97676 100755\n> --- a/t/t7503-pre-commit-hook.sh\n> +++ b/t/t7503-pre-commit-hook.sh\n> @@ -136,4 +136,18 @@ test_expect_success 'check the author in hook' '\n> git show -s\n> '\n>\n> +# a hook that checks if \"amend\" is passed as an argument\n> +cat > \"$HOOK\" < +#!/bin/sh\n> +test \"\\$1\" = amend\n> +EOF\n\nwrite_script would be the modern way to create this hook.\n\n> +\n> +test_expect_success 'check that \"amend\" argument is given' '\n> + git commit --amend --allow-empty\n> +'\n> +\n> +test_expect_success 'check that \"amend\" argument is NOT given' '\n> + ! git commit --allow-empty\n\nYou probably want test_must_fail rather than '!' [1].\n\n> +'\n> +\n> test_done\n> --\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/259871/focus=260092\n"},{"id":"252486","messageId":"20141125034424.GA19161@peff.net","threadId":"38035","inReplyTo":"1416828111-4567-1-git-send-email-oystwa@gmail.com","subject":"Re: [PATCH] commit: inform pre-commit if --amend is used","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-25T03:44:24Z","receivedAt":"2014-11-25T03:44:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 24, 2014 at 12:21:51PM +0100, Øystein Walle wrote:\n\n> This hook is invoked by 'git commit', and can be bypassed\n> -with `--no-verify` option. It takes no parameter, and is\n> +with `--no-verify` option. It takes one parameter which is \"amend\" if\n> +`--amend` was used when committing and empty otherwise. It is\n\nThis interface change is OK for backwards compatibility, since existing\nscripts would not look at the arguments (which are always empty\ncurrently). And I think it is OK for adding new options in the future,\ntoo, because the option is always present (just sometimes as an empty\nstring). Can we make the non-amend option something more verbose than\nthe empty string (like \"noamend\")? I have two reasons:\n\n 1. It is a bit more obvious when debugging or dumping arguments (e.g.,\n via GIT_TRACE), especially if new options are added after the\n first.\n\n 2. It makes it easier for a script to work on old and new versions of\n git. It sees either \"amend\" or \"noamend\" for the two obvious cases,\n and if it sees no argument, then it knows that it does not know\n either way (it is running on an old version of git).\n\n Technically one can tell the difference in shell between an empty\n string and a missing argument, but it is sufficiently subtle that I\n think \"noamend\" is a better route.\n\n> +# a hook that checks if \"amend\" is passed as an argument\n> +cat > \"$HOOK\" < +#!/bin/sh\n> +test \"\\$1\" = amend\n> +EOF\n\nEric mentioned write_script already (and it would be nice to convert the\nexisting uses in t7503 to use it). You may also want to use \"\\EOF\" to\ninhibit interpolation in the here-document, which means you do not have\nto quote variables inside the script.\n\n-Peff\n"},{"id":"252492","messageId":"xmqqtx1nub9e.fsf@gitster.dls.corp.google.com","threadId":"38035","inReplyTo":"20141125034424.GA19161@peff.net","subject":"Re: [PATCH] commit: inform pre-commit if --amend is used","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-25T04:58:37Z","receivedAt":"2014-11-25T04:58:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> 1. It is a bit more obvious when debugging or dumping arguments (e.g.,\n> via GIT_TRACE), especially if new options are added after the\n> first.\n>\n> 2. It makes it easier for a script to work on old and new versions of\n> git. It sees either \"amend\" or \"noamend\" for the two obvious cases,\n> and if it sees no argument, then it knows that it does not know\n> either way (it is running on an old version of git).\n>\n> Technically one can tell the difference in shell between an empty\n> string and a missing argument, but it is sufficiently subtle that I\n> think \"noamend\" is a better route.\n\nIf we ever add more info, would we want to keep piling on new\narguments, though? Wouldn't it a viable option to use \"amend\" vs\nnot giving anything (not even an empty string), so that normal case\nthere won't be no parameter?\n"},{"id":"252494","messageId":"20141125050331.GB20805@peff.net","threadId":"38035","inReplyTo":"xmqqtx1nub9e.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] commit: inform pre-commit if --amend is used","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-25T05:03:31Z","receivedAt":"2014-11-25T05:03:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 24, 2014 at 08:58:37PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > 1. It is a bit more obvious when debugging or dumping arguments (e.g.,\n> > via GIT_TRACE), especially if new options are added after the\n> > first.\n> >\n> > 2. It makes it easier for a script to work on old and new versions of\n> > git. It sees either \"amend\" or \"noamend\" for the two obvious cases,\n> > and if it sees no argument, then it knows that it does not know\n> > either way (it is running on an old version of git).\n> >\n> > Technically one can tell the difference in shell between an empty\n> > string and a missing argument, but it is sufficiently subtle that I\n> > think \"noamend\" is a better route.\n> \n> If we ever add more info, would we want to keep piling on new\n> arguments, though? Wouldn't it a viable option to use \"amend\" vs\n> not giving anything (not even an empty string), so that normal case\n> there won't be no parameter?\n\nThen when you add new arguments, the hook has to search through the\nparameters looking for one that matches, rather than just checking \"$1\"\nfor \"amend\" (and \"$2\" for the new option, and so on). As long as the set\nof options remains relatively small, I think that is preferable.\n\nWe could also just pass them through the environment, which gives nice\nnamed parameters.\n\n-Peff\n"},{"id":"252640","messageId":"547737C8.7070609@gmail.com","threadId":"38035","inReplyTo":"20141125050331.GB20805@peff.net","subject":"Re: [PATCH] commit: inform pre-commit if --amend is used","fromName":"Mark Levedahl","fromEmail":"mlevedahl@gmail.com","sentAt":"2014-11-27T14:40:08Z","receivedAt":"2014-11-27T14:40:08Z","isPatch":true,"sender":{"key":"mdl123@verizon.net","avatar":"https://avatars.githubusercontent.com/u/5302462?v=4"},"body":"On 11/25/2014 12:03 AM, Jeff King wrote:\n> On Mon, Nov 24, 2014 at 08:58:37PM -0800, Junio C Hamano wrote:\n>\n>> Jeff King writes:\n>>\n>>> 1. It is a bit more obvious when debugging or dumping arguments (e.g.,\n>>> via GIT_TRACE), especially if new options are added after the\n>>> first.\n>>>\n>>> 2. It makes it easier for a script to work on old and new versions of\n>>> git. It sees either \"amend\" or \"noamend\" for the two obvious cases,\n>>> and if it sees no argument, then it knows that it does not know\n>>> either way (it is running on an old version of git).\n>>>\n>>> Technically one can tell the difference in shell between an empty\n>>> string and a missing argument, but it is sufficiently subtle that I\n>>> think \"noamend\" is a better route.\n>>\n>> If we ever add more info, would we want to keep piling on new\n>> arguments, though? Wouldn't it a viable option to use \"amend\" vs\n>> not giving anything (not even an empty string), so that normal case\n>> there won't be no parameter?\n>\n> Then when you add new arguments, the hook has to search through the\n> parameters looking for one that matches, rather than just checking \"$1\"\n> for \"amend\" (and \"$2\" for the new option, and so on). As long as the set\n> of options remains relatively small, I think that is preferable.\n>\n> We could also just pass them through the environment, which gives nice\n> named parameters.\n>\n> -Peff\n>\n\nSee http://comments.gmane.org/gmane.comp.version-control.git/148479 for \nan earlier conversation on this exact topic. Also, see \nhttp://permalink.gmane.org/gmane.comp.version-control.git/148480 for a \nsimilar change in git-gui.\n\n-Mark\n"},{"id":"252654","messageId":"20141128051805.GA19258@peff.net","threadId":"38035","inReplyTo":"547737C8.7070609@gmail.com","subject":"Re: [PATCH] commit: inform pre-commit if --amend is used","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-28T05:18:05Z","receivedAt":"2014-11-28T05:18:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 27, 2014 at 09:40:08AM -0500, Mark Levedahl wrote:\n\n> >Then when you add new arguments, the hook has to search through the\n> >parameters looking for one that matches, rather than just checking \"$1\"\n> >for \"amend\" (and \"$2\" for the new option, and so on). As long as the set\n> >of options remains relatively small, I think that is preferable.\n> >\n> >We could also just pass them through the environment, which gives nice\n> >named parameters.\n> >\n> \n> See http://comments.gmane.org/gmane.comp.version-control.git/148479 for an\n> earlier conversation on this exact topic. Also, see\n> http://permalink.gmane.org/gmane.comp.version-control.git/148480 for a\n> similar change in git-gui.\n\nThanks for the links; I had no recollection of that thread.\nUnsurprisingly, I like the \"HEAD\"/\"HEAD~1\" suggestion. That \"peff\" guy\nseems really clever (and handsome, too, I'll bet).\n\nI'd still be OK with any of the suggestions given in this thread,\nthough.\n\n-Peff\n"},{"id":"252664","messageId":"5478996F.5050207@gmail.com","threadId":"38035","inReplyTo":"20141128051805.GA19258@peff.net","subject":"Re: [PATCH] commit: inform pre-commit if --amend is used","fromName":"Mark Levedahl","fromEmail":"mlevedahl@gmail.com","sentAt":"2014-11-28T15:49:03Z","receivedAt":"2014-11-28T15:49:03Z","isPatch":true,"sender":{"key":"mdl123@verizon.net","avatar":"https://avatars.githubusercontent.com/u/5302462?v=4"},"body":"On 11/28/2014 12:18 AM, Jeff King wrote:\n> On Thu, Nov 27, 2014 at 09:40:08AM -0500, Mark Levedahl wrote:\n>\n>>> Then when you add new arguments, the hook has to search through the\n>>> parameters looking for one that matches, rather than just checking \"$1\"\n>>> for \"amend\" (and \"$2\" for the new option, and so on). As long as the set\n>>> of options remains relatively small, I think that is preferable.\n>>>\n>>> We could also just pass them through the environment, which gives nice\n>>> named parameters.\n>>>\n>> See http://comments.gmane.org/gmane.comp.version-control.git/148479 for an\n>> earlier conversation on this exact topic. Also, see\n>> http://permalink.gmane.org/gmane.comp.version-control.git/148480 for a\n>> similar change in git-gui.\n> Thanks for the links; I had no recollection of that thread.\n> Unsurprisingly, I like the \"HEAD\"/\"HEAD~1\" suggestion. That \"peff\" guy\n> seems really clever (and handsome, too, I'll bet).\n>\n> I'd still be OK with any of the suggestions given in this thread,\n> though.\n>\n> -Peff\n> ars\n\nApparently our combined handsome-foo was insufficient to get this \naccepted way back when, hopefully the current submitter has more :^)\n\nIn any event, I've carried the patches using HEAD/HEAD~1 in my tree for \nthe last 4+ years, have a widely used pre-commit script that depends \nupon those. So, I personally would be very happy to see this finally \nshow up in Junio's tree, would prefer HEAD/HEAD~1 but can adapt to whatever.\n\nMark\n"},{"id":"252754","messageId":"xmqq3890tcfe.fsf@gitster.dls.corp.google.com","threadId":"38035","inReplyTo":"5478996F.5050207@gmail.com","subject":"Re: [PATCH] commit: inform pre-commit if --amend is used","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-01T00:56:53Z","receivedAt":"2014-12-01T00:56:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mark Levedahl writes:\n\n> On 11/28/2014 12:18 AM, Jeff King wrote:\n>\n>> Thanks for the links; I had no recollection of that thread.\n>> Unsurprisingly, I like the \"HEAD\"/\"HEAD~1\" suggestion. That \"peff\" guy\n>> seems really clever (and handsome, too, I'll bet).\n>>\n>> I'd still be OK with any of the suggestions given in this thread,\n>> though.\n>>\n>> -Peff\n>> ars\n>\n> Apparently our combined handsome-foo was insufficient to get this\n> accepted way back when, hopefully the current submitter has more :^)\n>\n> In any event, I've carried the patches using HEAD/HEAD~1 in my tree\n> for the last 4+ years, have a widely used pre-commit script that\n> depends upon those. So, I personally would be very happy to see this\n> finally show up in Junio's tree, would prefer HEAD/HEAD~1 but can\n> adapt to whatever.\n\nOne thing to be careful about is that the approach HEAD, HEAD~,\netc. does allow this to be extended to cover merge cases as the old\nthread speculated, it will make it impossible to pass any kind of\ninformation, other than \"here are the parents of the results\", to\nthe hook.\n\nOf course, there are ways to make sure that we won't paint us into\nan unescapable corner, e.g. an obvious example (not necessarily the\nbest) being to pass \"HEAD\", \"HEAD~\", \"b76b088 b260d26\" etc., in\nother words, passing these parents still as a single argument,\nmultiple parents concatenated with some delimiters, so that \"$1\"\nwill always be \"who are the parent(s)\" even when we needed to later\npass other sorts of information.\n"}]}