{"thread":{"id":"28488","subject":"[PATCH/RFC 0/2] Teach receive-pack not to run update hook for corrupt/non existent ref","startedAt":"2011-09-25T05:06:20Z","lastAt":"2011-09-27T22:55:46Z","messageCount":18,"participants":["Pang Yan Han","Sitaram Chamarty","Tay Ray Chuan","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"176153","messageId":"1316927182-14212-1-git-send-email-pangyanhan@gmail.com","threadId":"28488","inReplyTo":null,"subject":"[PATCH/RFC 0/2] Teach receive-pack not to run update hook for corrupt/non existent ref","fromName":"Pang Yan Han","fromEmail":"pangyanhan@gmail.com","sentAt":"2011-09-25T05:06:20Z","receivedAt":"2011-09-25T05:06:20Z","isPatch":true,"sender":{"key":"pangyanhan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/539739?v=4"},"body":"Hi list,\n\nCurrently, receive-pack runs the pre-receive, update, post-receive and\npost-update hooks during a push to delete corrupt or non-existent refs, eg:\n\n\tgit push origin :refs/heads/foo\n\nwhere refs/heads/foo is missing from the remote origin.\n\nThe issue is reported here [1]\n\n\nThis is a patch series which teaches receive-pack not to run update hook for\ncorrupt or non existent refs during a push.\n\nPatch 1/2 isn't really relevant to the topic. It's just something I stumbled\nacross while reading the code. It removes a redundant assignment in the is_url\nfunction.\n\nPatch 2/2 teaches receive-pack not to run update hook for corrupt or non\nexistent refs. In summary, I reordered the statements in the update function\nso that the update hook is not run for corrupt / non existent refs.\n\nPerhaps this isn't a good enough solution since the pre-receive, post-receive\nand post-update hooks are still run. Also the tests aren't exactly good looking.\n\nAny advice is highly appreciated. Thanks!\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/181942 \n\nPang Yan Han (2):\n  is_url: Remove redundant assignment\n  receive-pack: Don't run update hook for corrupt or nonexistent ref\n\n builtin/receive-pack.c |   50 +++++++++++++++++++++++++++--------------------\n t/t5516-fetch-push.sh  |   33 +++++++++++++++++++++++++++++++\n url.c                  |    1 -\n 3 files changed, 62 insertions(+), 22 deletions(-)\n\n-- \n1.7.7.rc3.2.g29f2e6\n"},{"id":"176154","messageId":"1316927182-14212-2-git-send-email-pangyanhan@gmail.com","threadId":"28488","inReplyTo":"1316927182-14212-1-git-send-email-pangyanhan@gmail.com","subject":"[PATCH/RFC 1/2] is_url: Remove redundant assignment","fromName":"Pang Yan Han","fromEmail":"pangyanhan@gmail.com","sentAt":"2011-09-25T05:06:21Z","receivedAt":"2011-09-25T05:06:21Z","isPatch":true,"sender":{"key":"pangyanhan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/539739?v=4"},"body":"Signed-off-by: Pang Yan Han <pangyanhan@gmail.com>\n---\n url.c |    1 -\n 1 files changed, 0 insertions(+), 1 deletions(-)\n\ndiff --git a/url.c b/url.c\nindex 3e06fd3..d2e17e6 100644\n--- a/url.c\n+++ b/url.c\n@@ -22,7 +22,6 @@ int is_url(const char *url)\n \n \tif (!url)\n \t\treturn 0;\n-\turl2 = url;\n \tfirst_slash = strchr(url, '/');\n \n \t/* Input with no slash at all or slash first can't be URL. */\n-- \n1.7.7.rc3.2.g29f2e6\n"},{"id":"176155","messageId":"1316927182-14212-3-git-send-email-pangyanhan@gmail.com","threadId":"28488","inReplyTo":"1316927182-14212-1-git-send-email-pangyanhan@gmail.com","subject":"[PATCH/RFC 2/2] receive-pack: Don't run update hook for corrupt or nonexistent ref","fromName":"Pang Yan Han","fromEmail":"pangyanhan@gmail.com","sentAt":"2011-09-25T05:06:22Z","receivedAt":"2011-09-25T05:06:22Z","isPatch":true,"sender":{"key":"pangyanhan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/539739?v=4"},"body":"Teach receive-pack not to run update hook for a corrupt or nonexistent ref.\n\nIn addition, update the warning message for deleting a corrupt or nonexistent\nref from:\n\n\"Allowing deletion of corrupt ref\"\n\nto:\n\n\"Allowing deletion of corrupt/nonexistent ref\"\n\nNoticed-by: Sitaram Chamarty <sitaramc@gmail.com>\nSigned-off-by: Pang Yan Han <pangyanhan@gmail.com>\n---\n builtin/receive-pack.c |   50 +++++++++++++++++++++++++++--------------------\n t/t5516-fetch-push.sh  |   33 +++++++++++++++++++++++++++++++\n 2 files changed, 62 insertions(+), 21 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex ae164da..a9385cf 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -346,6 +346,17 @@ static void refuse_unconfigured_deny_delete_current(void)\n \t\trp_error(\"%s\", refuse_unconfigured_deny_delete_current_msg[i]);\n }\n \n+static const char *delete_null_sha1_ref(const char *namespaced_name,\n+\t\t\t\t\t\t\t\t\t\tconst char *name,\n+\t\t\t\t\t\t\t\t\t\tunsigned char *old_sha1)\n+{\n+\tif (delete_ref(namespaced_name, old_sha1, 0)) {\n+\t\trp_error(\"failed to delete %s\", name);\n+\t\treturn \"failed to delete\";\n+\t}\n+\treturn NULL; /* good */\n+}\n+\n static const char *update(struct command *cmd)\n {\n \tconst char *name = cmd->ref_name;\n@@ -438,33 +449,30 @@ static const char *update(struct command *cmd)\n \t\t\treturn \"non-fast-forward\";\n \t\t}\n \t}\n+\n+\t/* Don't run update hook for corrupt/nonexistent ref */\n+\tif (is_null_sha1(new_sha1) && !parse_object(old_sha1)) {\n+\t\trp_warning(\"Allowing deletion of corrupt/nonexistent ref.\");\n+\t\told_sha1 = NULL;\n+\t\treturn delete_null_sha1_ref(namespaced_name, name, old_sha1);\n+\t}\n+\n \tif (run_update_hook(cmd)) {\n \t\trp_error(\"hook declined to update %s\", name);\n \t\treturn \"hook declined\";\n \t}\n \n-\tif (is_null_sha1(new_sha1)) {\n-\t\tif (!parse_object(old_sha1)) {\n-\t\t\trp_warning(\"Allowing deletion of corrupt ref.\");\n-\t\t\told_sha1 = NULL;\n-\t\t}\n-\t\tif (delete_ref(namespaced_name, old_sha1, 0)) {\n-\t\t\trp_error(\"failed to delete %s\", name);\n-\t\t\treturn \"failed to delete\";\n-\t\t}\n-\t\treturn NULL; /* good */\n-\t}\n-\telse {\n-\t\tlock = lock_any_ref_for_update(namespaced_name, old_sha1, 0);\n-\t\tif (!lock) {\n-\t\t\trp_error(\"failed to lock %s\", name);\n-\t\t\treturn \"failed to lock\";\n-\t\t}\n-\t\tif (write_ref_sha1(lock, new_sha1, \"push\")) {\n-\t\t\treturn \"failed to write\"; /* error() already called */\n-\t\t}\n-\t\treturn NULL; /* good */\n+\tif (is_null_sha1(new_sha1))\n+\t\treturn delete_null_sha1_ref(namespaced_name, name, old_sha1);\n+\n+\tlock = lock_any_ref_for_update(namespaced_name, old_sha1, 0);\n+\tif (!lock) {\n+\t\trp_error(\"failed to lock %s\", name);\n+\t\treturn \"failed to lock\";\n \t}\n+\tif (write_ref_sha1(lock, new_sha1, \"push\"))\n+\t\treturn \"failed to write\"; /* error() already called */\n+\treturn NULL; /* good */\n }\n \n static char update_post_hook[] = \"hooks/post-update\";\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 3abb290..b7a74e3 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -40,6 +40,20 @@ mk_test () {\n \t)\n }\n \n+mk_test_with_update_hook () {\n+\tmk_test \"$@\" &&\n+\t(\n+\t\tcd testrepo &&\n+\t\tmkdir .git/hooks &&\n+\t\tcd .git/hooks &&\n+\t\tcat >update <<'EOF'\n+#!/bin/sh\n+printf %s \"$@\" >>update.args\n+EOF\n+\t\tchmod u+x update\n+\t)\n+}\n+\n mk_child() {\n \trm -rf \"$1\" &&\n \tgit clone testrepo \"$1\"\n@@ -559,6 +573,25 @@ test_expect_success 'allow deleting an invalid remote ref' '\n \n '\n \n+test_expect_success 'deleting existing ref triggers update hook' '\n+\tmk_test_with_update_hook heads/master &&\n+\t(\n+\tcd testrepo &&\n+\tgit rev-parse --verify refs/heads/master &&\n+\tgit config receive.denyDeleteCurrent warn\n+\t) &&\n+\tgit push testrepo :refs/heads/master &&\n+\t(cd testrepo && test_must_fail git rev-parse --verify refs/heads/master) &&\n+\t(cd testrepo/.git && test -s update.args)\n+'\n+\n+test_expect_success 'deleting nonexistent ref does not trigger update hook' '\n+\tmk_test_with_update_hook heads/master &&\n+\trm -f testrepo/.git/objects/??/* &&\n+\tgit push testrepo :refs/heads/master &&\n+\t(cd testrepo/.git && ! test -f update.args)\n+'\n+\n test_expect_success 'allow deleting a ref using --delete' '\n \tmk_test heads/master &&\n \t(cd testrepo && git config receive.denyDeleteCurrent warn) &&\n-- \n1.7.7.rc3.2.g29f2e6\n"},{"id":"176158","messageId":"CAMK1S_hadzaqixaW3Fx81pf=hVsvAMpVvVGqVtZ8ncfUsie_9w@mail.gmail.com","threadId":"28488","inReplyTo":"1316927182-14212-1-git-send-email-pangyanhan@gmail.com","subject":"Re: [PATCH/RFC 0/2] Teach receive-pack not to run update hook for corrupt/non existent ref","fromName":"Sitaram Chamarty","fromEmail":"sitaramc@gmail.com","sentAt":"2011-09-25T07:58:31Z","receivedAt":"2011-09-25T07:58:31Z","isPatch":true,"sender":{"key":"sitaramc@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43316?v=4"},"body":"On Sun, Sep 25, 2011 at 10:36 AM, Pang Yan Han <pangyanhan@gmail.com> wrote:\n> Hi list,\n>\n> Currently, receive-pack runs the pre-receive, update, post-receive and\n> post-update hooks during a push to delete corrupt or non-existent refs, eg:\n>\n>        git push origin :refs/heads/foo\n>\n> where refs/heads/foo is missing from the remote origin.\n>\n> The issue is reported here [1]\n\nI did not report an issue.  I asked if this was expected and could be\nrelied upon.  I'm actually happy with the current behaviour because it\nsolves a problem very neatly for me, but before documenting it I\nwanted to make sure it would not one day disappear.\n\n> This is a patch series which teaches receive-pack not to run update hook for\n> corrupt or non existent refs during a push.\n>\n> Patch 1/2 isn't really relevant to the topic. It's just something I stumbled\n> across while reading the code. It removes a redundant assignment in the is_url\n> function.\n>\n> Patch 2/2 teaches receive-pack not to run update hook for corrupt or non\n> existent refs. In summary, I reordered the statements in the update function\n> so that the update hook is not run for corrupt / non existent refs.\n>\n> Perhaps this isn't a good enough solution since the pre-receive, post-receive\n> and post-update hooks are still run. Also the tests aren't exactly good looking.\n\nIt doesn't make sense to disable only the update hook.  And although I\ndid not come right out and say it, it is the post-update that I care\nabout.  If that still runs, my \"issue\" still exists.\n\n> Any advice is highly appreciated. Thanks!\n>\n> [1] http://thread.gmane.org/gmane.comp.version-control.git/181942\n>\n> Pang Yan Han (2):\n>  is_url: Remove redundant assignment\n>  receive-pack: Don't run update hook for corrupt or nonexistent ref\n>\n>  builtin/receive-pack.c |   50 +++++++++++++++++++++++++++--------------------\n>  t/t5516-fetch-push.sh  |   33 +++++++++++++++++++++++++++++++\n>  url.c                  |    1 -\n>  3 files changed, 62 insertions(+), 22 deletions(-)\n>\n> --\n> 1.7.7.rc3.2.g29f2e6\n>\n>\n\n\n\n-- \nSitaram\n"},{"id":"176159","messageId":"CALUzUxp-++A7azzimE1mjvPVoGxRM4rzYt9z66CmFb0B3vi7dQ@mail.gmail.com","threadId":"28488","inReplyTo":"1316927182-14212-2-git-send-email-pangyanhan@gmail.com","subject":"Re: [PATCH/RFC 1/2] is_url: Remove redundant assignment","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2011-09-25T09:26:27Z","receivedAt":"2011-09-25T09:26:27Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"On Sun, Sep 25, 2011 at 1:06 PM, Pang Yan Han <pangyanhan@gmail.com> wrote:\n> Signed-off-by: Pang Yan Han <pangyanhan@gmail.com>\n> ---\n>  url.c |    1 -\n>  1 files changed, 0 insertions(+), 1 deletions(-)\n>\n> diff --git a/url.c b/url.c\n> index 3e06fd3..d2e17e6 100644\n> --- a/url.c\n> +++ b/url.c\n> @@ -22,7 +22,6 @@ int is_url(const char *url)\n>\n>        if (!url)\n>                return 0;\n> -       url2 = url;\n>        first_slash = strchr(url, '/');\n>\n>        /* Input with no slash at all or slash first can't be URL. */\n\nLooks correct. Perhaps you could mention in the patch message that\n\n  There are no operations on url2 until another assignment to it later\nat line 41.\n\n-- \nCheers,\nRay Chuan\n"},{"id":"176160","messageId":"20110925094822.GA1702@myhost","threadId":"28488","inReplyTo":"CAMK1S_hadzaqixaW3Fx81pf=hVsvAMpVvVGqVtZ8ncfUsie_9w@mail.gmail.com","subject":"Re: [PATCH/RFC 0/2] Teach receive-pack not to run update hook for corrupt/non existent ref","fromName":"Pang Yan Han","fromEmail":"pangyanhan@gmail.com","sentAt":"2011-09-25T09:48:22Z","receivedAt":"2011-09-25T09:48:22Z","isPatch":true,"sender":{"key":"pangyanhan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/539739?v=4"},"body":"On Sun, Sep 25, 2011 at 01:28:31PM +0530, Sitaram Chamarty wrote:\n> On Sun, Sep 25, 2011 at 10:36 AM, Pang Yan Han <pangyanhan@gmail.com> wrote:\n> > Hi list,\n> >\n> > Currently, receive-pack runs the pre-receive, update, post-receive and\n> > post-update hooks during a push to delete corrupt or non-existent refs, eg:\n> >\n> >        git push origin :refs/heads/foo\n> >\n> > where refs/heads/foo is missing from the remote origin.\n> >\n> > The issue is reported here [1]\n> \n> I did not report an issue.  I asked if this was expected and could be\n> relied upon.  I'm actually happy with the current behaviour because it\n> solves a problem very neatly for me, but before documenting it I\n> wanted to make sure it would not one day disappear.\n> \n> > This is a patch series which teaches receive-pack not to run update hook for\n> > corrupt or non existent refs during a push.\n> >\n> > Patch 1/2 isn't really relevant to the topic. It's just something I stumbled\n> > across while reading the code. It removes a redundant assignment in the is_url\n> > function.\n> >\n> > Patch 2/2 teaches receive-pack not to run update hook for corrupt or non\n> > existent refs. In summary, I reordered the statements in the update function\n> > so that the update hook is not run for corrupt / non existent refs.\n> >\n> > Perhaps this isn't a good enough solution since the pre-receive, post-receive\n> > and post-update hooks are still run. Also the tests aren't exactly good looking.\n> \n> It doesn't make sense to disable only the update hook.  And although I\n> did not come right out and say it, it is the post-update that I care\n> about.  If that still runs, my \"issue\" still exists.\n\nUm I'm rather new to Git and the reason why I didn't reply this initially was\nbecause I didn't know what to reply. Sorry but you sound rather aggressive and\nI was really taken aback by this.\n\nI've taken a look at the code again and here's another approach which will\nresult in heavier changes to builtin/receive-pack and may possibly work:\n\nCheck through the list of refs to be updated before even executing the\npre receive hook. Ensure that there is at least one \"valid\" ref update.\nEssentially this is kind of like \"dry running\" the update function.\n\nOne way to do it is to shift the bulk of work determining valid and invalid\nref updates from the update function to a separate function.\nWe maintain 2 lists, one to store valid refs to be updated and another to\nstore non-existent/corrupt refs which will be deleted. Specifically, we will\nbe storing 'struct command'.\n\nThe update function can be cut down to these parts:\n- determining namespaced_name\n- lock_any_ref_for_update\n- write_ref_sha1\n\nThe pre receive hook will only be executed if the list containing valid ref\npushes is non-empty. Same goes for the post receive and post update hooks.\n\nWhat do you think of this approach (if it's even correct)?\n\n> \n> > Any advice is highly appreciated. Thanks!\n> >\n> > [1] http://thread.gmane.org/gmane.comp.version-control.git/181942\n> >\n> > Pang Yan Han (2):\n> >  is_url: Remove redundant assignment\n> >  receive-pack: Don't run update hook for corrupt or nonexistent ref\n> >\n> >  builtin/receive-pack.c |   50 +++++++++++++++++++++++++++--------------------\n> >  t/t5516-fetch-push.sh  |   33 +++++++++++++++++++++++++++++++\n> >  url.c                  |    1 -\n> >  3 files changed, 62 insertions(+), 22 deletions(-)\n> >\n> > --\n> > 1.7.7.rc3.2.g29f2e6\n> >\n> >\n> \n> \n> \n> -- \n> Sitaram\n"},{"id":"176162","messageId":"CAMK1S_h3ufrK29_ajpcSSW7HV6ZA8z8ZVHvhHr2bx5Cga5FAKQ@mail.gmail.com","threadId":"28488","inReplyTo":"20110925094822.GA1702@myhost","subject":"Re: [PATCH/RFC 0/2] Teach receive-pack not to run update hook for corrupt/non existent ref","fromName":"Sitaram Chamarty","fromEmail":"sitaramc@gmail.com","sentAt":"2011-09-25T12:05:03Z","receivedAt":"2011-09-25T12:05:03Z","isPatch":true,"sender":{"key":"sitaramc@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43316?v=4"},"body":"On Sun, Sep 25, 2011 at 3:18 PM, Pang Yan Han <pangyanhan@gmail.com> wrote:\n> On Sun, Sep 25, 2011 at 01:28:31PM +0530, Sitaram Chamarty wrote:\n\n[snip]\n\n>> It doesn't make sense to disable only the update hook.  And although I\n>> did not come right out and say it, it is the post-update that I care\n>> about.  If that still runs, my \"issue\" still exists.\n\n> Um I'm rather new to Git and the reason why I didn't reply this initially was\n> because I didn't know what to reply. Sorry but you sound rather aggressive and\n> I was really taken aback by this.\n\nSorry if I sounded aggressive; I was going to brevity, and levity suffered :-)\n\n[snip lots of stuff about new approach]\n\n> What do you think of this approach (if it's even correct)?\n\nI'm sorry again but it's been almost 2 decades since I did any serious\nC and I've never dug into git internals, so I can't tell you if you're\neven on the right track.  You should wait for one of the other folks\nyou cc-d to weigh in with their opinions.\n\nPersonally, anytime someone says \"disable the update hook\" I get very\nworried -- I've got a heck of a lot invested in update hooks ;-)\n\nI wasn't even *asking* about disabling that; I was asking about\n*post*-update, which you didn't even address in your code.\n\nFrom a philosophical point of view, update and pre-receive *check*\nthings to make sure everything is OK.  IMO they should be allowed to\nrun even if the ref being deleted doesn't exist -- that could well be\nan error condition that the guy who owns the repo wants to trap and\nalert himself to in some special way.  I would *not* like them\ndisabled.\n\nPost-{update,receive} are for *after* a successful push.  My\nsuggestion would be to make sure the inputs supplied to those hooks\n(via STDIN for post-receive, and as arguments in case of post-update)\nreflect this -- only successfully updated refs are sent in as args.\n\nThis might mean that in the case of 'git push origin\n:refs/heads/non-existent-ref' the post-receive hook would run but\nSTDIN would be empty, and post-update would run but have no arguments.\n\nThat is, IMO, the correct way to deal with this.\n"},{"id":"176175","messageId":"20110925173734.GA29256@myhost","threadId":"28488","inReplyTo":"1316927182-14212-3-git-send-email-pangyanhan@gmail.com","subject":"[PATCH/RFCv2 2/2] run post-receive and post-update hooks with empty stdin/no args for invalid ref deletion","fromName":"Pang Yan Han","fromEmail":"pangyanhan@gmail.com","sentAt":"2011-09-25T17:37:34Z","receivedAt":"2011-09-25T17:37:34Z","isPatch":true,"sender":{"key":"pangyanhan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/539739?v=4"},"body":"The post-receive and post-update hooks are triggered with invalid input on\nstdin and invalid args respectively during the deletion of corrupt or\nnon-existent refs during a push.\n\nTeach receive-pack to run post-receive hook with empty stdin and post-update\nhook with empty args in the event of an invalid ref deletion.\n\nSigned-off-by: Pang Yan Han <pangyanhan@gmail.com>\n---\nThis has been updated with the suggestions from Sitaram Chamarty, as stated in\nhttp://thread.gmane.org/gmane.comp.version-control.git/182056/focus=182065\n\n builtin/receive-pack.c |   35 +++++++++++++-\n t/t5516-fetch-push.sh  |  118 ++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 150 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex ae164da..28d0b09 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -153,6 +153,26 @@ struct command {\n \tchar ref_name[FLEX_ARRAY]; /* more */\n };\n \n+/* For invalid refs */\n+static struct command **invalid_delete;\n+static size_t invalid_delete_nr;\n+static size_t invalid_delete_alloc;\n+\n+static void invalid_delete_append(struct command *cmd)\n+{\n+\tALLOC_GROW(invalid_delete, invalid_delete_nr + 1, invalid_delete_alloc);\n+\tinvalid_delete[invalid_delete_nr++] = cmd;\n+}\n+\n+static int is_invalid_delete(struct command *cmd)\n+{\n+\tsize_t i;\n+\tfor (i = 0; i < invalid_delete_nr; i++)\n+\t\tif (invalid_delete[i] == cmd)\n+\t\t\treturn 1;\n+\treturn 0;\n+}\n+\n static const char pre_receive_hook[] = \"hooks/pre-receive\";\n static const char post_receive_hook[] = \"hooks/post-receive\";\n \n@@ -248,7 +268,8 @@ static int run_receive_hook(struct command *commands, const char *hook_name)\n \t}\n \n \tfor (cmd = commands; cmd; cmd = cmd->next) {\n-\t\tif (!cmd->error_string) {\n+\t\t/* Run with empty stdin for invalid ref deletion */\n+\t\tif (!cmd->error_string && !is_invalid_delete(cmd)) {\n \t\t\tsize_t n = snprintf(buf, sizeof(buf), \"%s %s %s\\n\",\n \t\t\t\tsha1_to_hex(cmd->old_sha1),\n \t\t\t\tsha1_to_hex(cmd->new_sha1),\n@@ -447,6 +468,7 @@ static const char *update(struct command *cmd)\n \t\tif (!parse_object(old_sha1)) {\n \t\t\trp_warning(\"Allowing deletion of corrupt ref.\");\n \t\t\told_sha1 = NULL;\n+\t\t\tinvalid_delete_append(cmd);\n \t\t}\n \t\tif (delete_ref(namespaced_name, old_sha1, 0)) {\n \t\t\trp_error(\"failed to delete %s\", name);\n@@ -490,8 +512,14 @@ static void run_update_post_hook(struct command *commands)\n \t\tchar *p;\n \t\tif (cmd->error_string)\n \t\t\tcontinue;\n-\t\tp = xmalloc(strlen(cmd->ref_name) + 1);\n-\t\tstrcpy(p, cmd->ref_name);\n+\t\tif (is_invalid_delete(cmd)) {\n+\t\t\t/* Run with empty args for invalid ref deletion */\n+\t\t\tp = xmalloc(1);\n+\t\t\tp[0] = '\\0';\n+\t\t} else {\n+\t\t\tp = xmalloc(strlen(cmd->ref_name) + 1);\n+\t\t\tstrcpy(p, cmd->ref_name);\n+\t\t}\n \t\targv[argc] = p;\n \t\targc++;\n \t}\n@@ -866,5 +894,6 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \t}\n \tif (use_sideband)\n \t\tpacket_flush(1);\n+\tfree(invalid_delete);\n \treturn 0;\n }\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 3abb290..038a3b3 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -40,6 +40,39 @@ mk_test () {\n \t)\n }\n \n+mk_test_with_hooks() {\n+\tmk_test \"$@\" &&\n+\t(\n+\tcd testrepo &&\n+\tmkdir .git/hooks &&\n+\tcd .git/hooks &&\n+\n+\tcat >pre-receive <<'EOF' &&\n+#!/bin/sh\n+cat - >>pre-receive.actual\n+EOF\n+\n+\tcat >update <<'EOF' &&\n+#!/bin/sh\n+printf \"%s %s %s\\n\" \"$@\" >>update.actual\n+EOF\n+\tcat >post-receive <<'EOF' &&\n+#!/bin/sh\n+cat - >>post-receive.actual\n+EOF\n+\n+\tcat >post-update <<'EOF' &&\n+#!/bin/sh\n+for ref in \"$@\"\n+do\n+\tprintf \"%s\\n\" \"$ref\" >>post-update.actual\n+done\n+EOF\n+\n+\tchmod u+x pre-receive update post-receive post-update\n+\t)\n+}\n+\n mk_child() {\n \trm -rf \"$1\" &&\n \tgit clone testrepo \"$1\"\n@@ -559,6 +592,91 @@ test_expect_success 'allow deleting an invalid remote ref' '\n \n '\n \n+test_expect_success 'pushing valid refs triggers post-receive and post-update hooks' '\n+\tmk_test_with_hooks heads/master &&\n+\torgmaster=$(cd testrepo && git show-ref -s --verify refs/heads/master) &&\n+\tnewmaster=$(git show-ref -s --verify refs/heads/master) &&\n+\tgit push testrepo refs/heads/master:refs/heads/master :refs/heads/nonexistent &&\n+\t(cd testrepo/.git &&\n+\tcat >pre-receive.expect <<'EOF' &&\n+$orgmaster $newmaster refs/heads/master\n+$_z40 $_z40 refs/heads/nonexistent\n+EOF\n+\n+\tcat >update.expect <<'EOF' &&\n+refs/heads/master $orgmaster $newmaster\n+refs/heads/nonexistent $_z40 $_z40\n+EOF\n+\n+\tcat >post-receive.expect <<'EOF' &&\n+$orgmaster $newmaster refs/heads/master\n+EOF\n+\n+\tcat >post-update.expect <<'EOF' &&\n+refs/heads/master\n+\n+EOF\n+\n+\ttest_cmp pre-receive.expect pre-receive.actual &&\n+\ttest_cmp update.expect update.actual &&\n+\ttest_cmp post-receive.expect post-receive.actual &&\n+\ttest_cmp post-update.expect post-update.actual\n+\t)\n+'\n+\n+test_expect_success 'deleting non-existent ref triggers post-receive and post-update hooks with empty args' '\n+\tmk_test_with_hooks heads/master &&\n+\tgit push testrepo :refs/heads/nonexistent &&\n+\t(cd testrepo/.git &&\n+\tcat >pre-receive.expect <<'EOF' &&\n+$_z40 $_z40 refs/heads/nonexistent\n+EOF\n+\n+\tcat >update.expect <<'EOF' &&\n+refs/heads/nonexistent $_z40 $_z40\n+EOF\n+\n+\tcat >post-receive.expect <<'EOF' &&\n+EOF\n+\n+\tcat >post-update.expect <<'EOF' &&\n+\n+EOF\n+\n+\ttest_cmp pre-receive.expect pre-receive.actual &&\n+\ttest_cmp update.expect update.actual &&\n+\ttest_cmp post-receive.expect post-receive.actual &&\n+\ttest_cmp post-update.expect post-update.actual\n+\t)\n+'\n+\n+test_expect_success 'deleting invalid ref triggers post-receive and post-update hooks with empty args' '\n+\tmk_test_with_hooks heads/master &&\n+\trm -f testrepo/.git/objects/??/* &&\n+\tgit push testrepo :refs/heads/master &&\n+\t(cd testrepo/.git &&\n+\tcat >pre-receive.expect <<'EOF' &&\n+$_z40 $_z40 refs/heads/master\n+EOF\n+\n+\tcat >update.expect <<'EOF' &&\n+refs/heads/master $_z40 $_z40\n+EOF\n+\n+\tcat >post-receive.expect <<'EOF' &&\n+EOF\n+\n+\tcat >post-update.expect <<'EOF' &&\n+\n+EOF\n+\n+\ttest_cmp pre-receive.expect pre-receive.actual &&\n+\ttest_cmp update.expect update.actual &&\n+\ttest_cmp post-receive.expect post-receive.actual &&\n+\ttest_cmp post-update.expect post-update.actual\n+\t)\n+'\n+\n test_expect_success 'allow deleting a ref using --delete' '\n \tmk_test heads/master &&\n \t(cd testrepo && git config receive.denyDeleteCurrent warn) &&\n-- \n1.7.7.rc3.2.gd706f.dirty\n"},{"id":"176228","messageId":"7vsjnj455l.fsf@alter.siamese.dyndns.org","threadId":"28488","inReplyTo":"CALUzUxp-++A7azzimE1mjvPVoGxRM4rzYt9z66CmFb0B3vi7dQ@mail.gmail.com","subject":"Re: [PATCH/RFC 1/2] is_url: Remove redundant assignment","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-26T16:52:54Z","receivedAt":"2011-09-26T16:52:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tay Ray Chuan <rctay89@gmail.com> writes:\n\n> On Sun, Sep 25, 2011 at 1:06 PM, Pang Yan Han <pangyanhan@gmail.com> wrote:\n>> Signed-off-by: Pang Yan Han <pangyanhan@gmail.com>\n>> ---\n>>  url.c |    1 -\n>>  1 files changed, 0 insertions(+), 1 deletions(-)\n>>\n>> diff --git a/url.c b/url.c\n>> index 3e06fd3..d2e17e6 100644\n>> --- a/url.c\n>> +++ b/url.c\n>> @@ -22,7 +22,6 @@ int is_url(const char *url)\n>>\n>>        if (!url)\n>>                return 0;\n>> -       url2 = url;\n>>        first_slash = strchr(url, '/');\n>>\n>>        /* Input with no slash at all or slash first can't be URL. */\n>\n> Looks correct. Perhaps you could mention in the patch message that\n>\n>   There are no operations on url2 until another assignment to it later\n> at line 41.\n\nThe looks correct, so I'll queue it, but it looks like that the function\nis implemented in an overly complicated way.\n\nWhy aren't we checking from left to right in a single pass, perhaps like\nthis?\n\n\t/* Make sure it is of form \"scheme://something\" */\n\tint is_url(const char *url)\n\t{\n\t\t/* Is \"scheme\" part reasonable? */\n\t\tif (!url || !is_urlschemechar(1, *url++))\n\t        \treturn 0;\n\t\twhile (*url && *url != ':') {\n\t\t\tif (!is_urlschemechar(0, *url++))\n\t\t\t\treturn 0;\n\t\t}\n\t\t/* We've seen \"scheme\"; we want colon-slash-slash */\n\t\treturn (url[0] == ':' && url[1] == '/' && url[2] == '/');\n\t}\n"},{"id":"176263","messageId":"20110926213216.GA5415@sigill.intra.peff.net","threadId":"28488","inReplyTo":"7vsjnj455l.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH/RFC 1/2] is_url: Remove redundant assignment","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-09-26T21:32:16Z","receivedAt":"2011-09-26T21:32:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"[+cc Ilari, who wrote the code originally]\n\nOn Mon, Sep 26, 2011 at 09:52:54AM -0700, Junio C Hamano wrote:\n\n> Tay Ray Chuan <rctay89@gmail.com> writes:\n> \n> > On Sun, Sep 25, 2011 at 1:06 PM, Pang Yan Han <pangyanhan@gmail.com> wrote:\n> >> Signed-off-by: Pang Yan Han <pangyanhan@gmail.com>\n> >> ---\n> >>  url.c |    1 -\n> >>  1 files changed, 0 insertions(+), 1 deletions(-)\n> >>\n> >> diff --git a/url.c b/url.c\n> >> index 3e06fd3..d2e17e6 100644\n> >> --- a/url.c\n> >> +++ b/url.c\n> >> @@ -22,7 +22,6 @@ int is_url(const char *url)\n> >>\n> >>        if (!url)\n> >>                return 0;\n> >> -       url2 = url;\n> >>        first_slash = strchr(url, '/');\n> >>\n> >>        /* Input with no slash at all or slash first can't be URL. */\n> >\n> > Looks correct. Perhaps you could mention in the patch message that\n> >\n> >   There are no operations on url2 until another assignment to it later\n> > at line 41.\n> \n> The looks correct, so I'll queue it, but it looks like that the function\n> is implemented in an overly complicated way.\n> \n> Why aren't we checking from left to right in a single pass, perhaps like\n> this?\n> \n> \t/* Make sure it is of form \"scheme://something\" */\n> \tint is_url(const char *url)\n> \t{\n> \t\t/* Is \"scheme\" part reasonable? */\n> \t\tif (!url || !is_urlschemechar(1, *url++))\n> \t        \treturn 0;\n> \t\twhile (*url && *url != ':') {\n> \t\t\tif (!is_urlschemechar(0, *url++))\n> \t\t\t\treturn 0;\n> \t\t}\n> \t\t/* We've seen \"scheme\"; we want colon-slash-slash */\n> \t\treturn (url[0] == ':' && url[1] == '/' && url[2] == '/');\n> \t}\n\nThat looks right to me, and is way more readable.\n\n-Peff\n"},{"id":"176283","messageId":"7vwrcuzy44.fsf@alter.siamese.dyndns.org","threadId":"28488","inReplyTo":"CAMK1S_h3ufrK29_ajpcSSW7HV6ZA8z8ZVHvhHr2bx5Cga5FAKQ@mail.gmail.com","subject":"Re: [PATCH/RFC 0/2] Teach receive-pack not to run update hook for corrupt/non existent ref","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-26T23:23:55Z","receivedAt":"2011-09-26T23:23:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sitaram Chamarty <sitaramc@gmail.com> writes:\n\n> From a philosophical point of view, update and pre-receive *check*\n> things to make sure everything is OK.  IMO they should be allowed to\n> run even if the ref being deleted doesn't exist -- that could well be\n> an error condition that the guy who owns the repo wants to trap and\n> alert himself to in some special way.  I would *not* like them\n> disabled.\n\nI think this is a sane thing to do.\n\n> Post-{update,receive} are for *after* a successful push.  My\n> suggestion would be to make sure the inputs supplied to those hooks\n> (via STDIN for post-receive, and as arguments in case of post-update)\n> reflect this -- only successfully updated refs are sent in as args.\n\nPerhaps sane.\n\n> This might mean that in the case of 'git push origin\n> :refs/heads/non-existent-ref' the post-receive hook would run but\n> STDIN would be empty, and post-update would run but have no arguments.\n\nHmm?\n\nIn that case (if \"non-existent-ref\" was indeed non-existent, and not just\npointing at a dangling commit), I would say the post anything hook should\nnot be called for that ref.  These hooks of course need to run if there\nare _other_ refs that were updated, though, to handle these _other_ refs,\nbut I do not think they should be told about the no-op.\n"},{"id":"176288","messageId":"CAMK1S_gR6U=OxzGsjTD8LbvZFS125=p1fQ8Af7aRD2XSsRur_Q@mail.gmail.com","threadId":"28488","inReplyTo":"7vwrcuzy44.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH/RFC 0/2] Teach receive-pack not to run update hook for corrupt/non existent ref","fromName":"Sitaram Chamarty","fromEmail":"sitaramc@gmail.com","sentAt":"2011-09-26T23:44:02Z","receivedAt":"2011-09-26T23:44:02Z","isPatch":true,"sender":{"key":"sitaramc@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43316?v=4"},"body":"On Tue, Sep 27, 2011 at 4:53 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Sitaram Chamarty <sitaramc@gmail.com> writes:\n>\n>> From a philosophical point of view, update and pre-receive *check*\n>> things to make sure everything is OK.  IMO they should be allowed to\n>> run even if the ref being deleted doesn't exist -- that could well be\n>> an error condition that the guy who owns the repo wants to trap and\n>> alert himself to in some special way.  I would *not* like them\n>> disabled.\n>\n> I think this is a sane thing to do.\n>\n>> Post-{update,receive} are for *after* a successful push.  My\n>> suggestion would be to make sure the inputs supplied to those hooks\n>> (via STDIN for post-receive, and as arguments in case of post-update)\n>> reflect this -- only successfully updated refs are sent in as args.\n>\n> Perhaps sane.\n>\n>> This might mean that in the case of 'git push origin\n>> :refs/heads/non-existent-ref' the post-receive hook would run but\n>> STDIN would be empty, and post-update would run but have no arguments.\n>\n> Hmm?\n>\n> In that case (if \"non-existent-ref\" was indeed non-existent, and not just\n> pointing at a dangling commit), I would say the post anything hook should\n> not be called for that ref.  These hooks of course need to run if there\n> are _other_ refs that were updated, though, to handle these _other_ refs,\n> but I do not think they should be told about the no-op.\n\nQuestion is what happens if none of them existed.  It's a difference\nbetween not calling the hook at all, versus calling it with no\narguments/empty stdin (as the case may be) -- which would you do?  I\nsay the hook *should* always run, and the code inside the hook should\ntake care of the fact that no arguments/no input means nothing\nactually happened.\n\n-- \nSitaram\n"},{"id":"176289","messageId":"7voby6zwxg.fsf@alter.siamese.dyndns.org","threadId":"28488","inReplyTo":"CAMK1S_gR6U=OxzGsjTD8LbvZFS125=p1fQ8Af7aRD2XSsRur_Q@mail.gmail.com","subject":"Re: [PATCH/RFC 0/2] Teach receive-pack not to run update hook for corrupt/non existent ref","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-26T23:49:31Z","receivedAt":"2011-09-26T23:49:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sitaram Chamarty <sitaramc@gmail.com> writes:\n\n>> In that case (if \"non-existent-ref\" was indeed non-existent, and not just\n>> pointing at a dangling commit), I would say the post anything hook should\n>> not be called for that ref. These hooks of course need to run if there\n>> are _other_ refs that were updated, though, to handle these _other_ refs,\n>> but I do not think they should be told about the no-op.\n>\n> Question is what happens if none of them existed.  It's a difference\n> between not calling the hook at all, versus calling it with no\n> arguments/empty stdin (as the case may be) -- which would you do?\n\nIn case it was unclear, I was trying to say the hooks should not run with\nempty input.\n"},{"id":"176290","messageId":"7vd3emzw8n.fsf@alter.siamese.dyndns.org","threadId":"28488","inReplyTo":"7voby6zwxg.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH/RFC 0/2] Teach receive-pack not to run update hook for corrupt/non existent ref","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-27T00:04:24Z","receivedAt":"2011-09-27T00:04:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Sitaram Chamarty <sitaramc@gmail.com> writes:\n>\n>>> In that case (if \"non-existent-ref\" was indeed non-existent, and not just\n>>> pointing at a dangling commit), I would say the post anything hook should\n>>> not be called for that ref. These hooks of course need to run if there\n>>> are _other_ refs that were updated, though, to handle these _other_ refs,\n>>> but I do not think they should be told about the no-op.\n>>\n>> Question is what happens if none of them existed.  It's a difference\n>> between not calling the hook at all, versus calling it with no\n>> arguments/empty stdin (as the case may be) -- which would you do?\n>\n> In case it was unclear, I was trying to say the hooks should not run with\n> empty input.\n\nIf the purpose of \"post-update\" (or \"post-receive\") hooks were to trigger\nevery time anybody attempted to push into the repository, then it would\nmake perfect sense for them to trigger when \"push origin :no-such-branch\"\nwere attempted. But if that were the purpose of these hooks, they should\nalso trigger when \"push origin master\" is run and \"master\" is already at\nthe right commit, as that is the same kind of no-op -- the pushed into\nrepository was already up-to-date with respect to the wish of the pusher.\n\nI do not mind, and I do prefer, these hooks to run when somebody deleted\nan existing ref that points at a corrupt or non-existent object, as that\nis _not_ a no-op but is a meaningful event that has an effect that is\nobservable from the outside world (e.g. ls-remote).\n"},{"id":"176292","messageId":"CAMK1S_gZFxtCwUnzRU3PocB9LcewZ-f5RyraCebJdaBASODaPg@mail.gmail.com","threadId":"28488","inReplyTo":"7voby6zwxg.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH/RFC 0/2] Teach receive-pack not to run update hook for corrupt/non existent ref","fromName":"Sitaram Chamarty","fromEmail":"sitaramc@gmail.com","sentAt":"2011-09-27T00:05:33Z","receivedAt":"2011-09-27T00:05:33Z","isPatch":true,"sender":{"key":"sitaramc@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43316?v=4"},"body":"On Tue, Sep 27, 2011 at 5:19 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Sitaram Chamarty <sitaramc@gmail.com> writes:\n>\n>>> In that case (if \"non-existent-ref\" was indeed non-existent, and not just\n>>> pointing at a dangling commit), I would say the post anything hook should\n>>> not be called for that ref. These hooks of course need to run if there\n>>> are _other_ refs that were updated, though, to handle these _other_ refs,\n>>> but I do not think they should be told about the no-op.\n>>\n>> Question is what happens if none of them existed.  It's a difference\n>> between not calling the hook at all, versus calling it with no\n>> arguments/empty stdin (as the case may be) -- which would you do?\n>\n> In case it was unclear, I was trying to say the hooks should not run with\n> empty input.\n\nI saw \"should not be called for that ref\" and I did get confused;\nthanks for clarifying.\n\nI perfectly fine with it for post-{update,receive}.  My interest is in\nmaking sure the *update* hook runs, which (in an earlier email in the\nthread) I explained why and you agreed it made sense.\n\nThanks,\n\n-- \nSitaram\n"},{"id":"176319","messageId":"20110927090225.GA1493@myhost","threadId":"28488","inReplyTo":"7vd3emzw8n.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH/RFC 0/2] Teach receive-pack not to run update hook for corrupt/non existent ref","fromName":"Pang Yan Han","fromEmail":"pangyanhan@gmail.com","sentAt":"2011-09-27T09:02:25Z","receivedAt":"2011-09-27T09:02:25Z","isPatch":true,"sender":{"key":"pangyanhan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/539739?v=4"},"body":"Hi Junio,\n\nShould I reroll this patch with this behaviour:\n\n- Everything as usual for valid ref updates and deletes\n- For deleting corrupt (dangling?) ref, post-receive and post-update hooks\n  also receive the same args as per valid update / delete\n- For deleting non-existent refs:\n  - post-receive shall have empty stdin for those refs\n  - post-update shall have an empty arg for those refs\n\nThanks.\n\nOn Mon, Sep 26, 2011 at 05:04:24PM -0700, Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > Sitaram Chamarty <sitaramc@gmail.com> writes:\n> >\n> >>> In that case (if \"non-existent-ref\" was indeed non-existent, and not just\n> >>> pointing at a dangling commit), I would say the post anything hook should\n> >>> not be called for that ref. These hooks of course need to run if there\n> >>> are _other_ refs that were updated, though, to handle these _other_ refs,\n> >>> but I do not think they should be told about the no-op.\n> >>\n> >> Question is what happens if none of them existed.  It's a difference\n> >> between not calling the hook at all, versus calling it with no\n> >> arguments/empty stdin (as the case may be) -- which would you do?\n> >\n> > In case it was unclear, I was trying to say the hooks should not run with\n> > empty input.\n> \n> If the purpose of \"post-update\" (or \"post-receive\") hooks were to trigger\n> every time anybody attempted to push into the repository, then it would\n> make perfect sense for them to trigger when \"push origin :no-such-branch\"\n> were attempted. But if that were the purpose of these hooks, they should\n> also trigger when \"push origin master\" is run and \"master\" is already at\n> the right commit, as that is the same kind of no-op -- the pushed into\n> repository was already up-to-date with respect to the wish of the pusher.\n> \n> I do not mind, and I do prefer, these hooks to run when somebody deleted\n> an existing ref that points at a corrupt or non-existent object, as that\n> is _not_ a no-op but is a meaningful event that has an effect that is\n> observable from the outside world (e.g. ls-remote).\n"},{"id":"176343","messageId":"7vhb3xyld7.fsf@alter.siamese.dyndns.org","threadId":"28488","inReplyTo":"20110927090225.GA1493@myhost","subject":"Re: [PATCH/RFC 0/2] Teach receive-pack not to run update hook for corrupt/non existent ref","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-27T16:56:52Z","receivedAt":"2011-09-27T16:56:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"[offtopic: where does that annoying M-F-T header come from? It even seems\nto be pointless in this case as it lists the same people as are already on\nTo/Cc/From of the message.]\n\nPang Yan Han <pangyanhan@gmail.com> writes:\n\n> Should I reroll this patch with this behaviour:\n>\n> - Everything as usual for valid ref updates and deletes\n> - For deleting corrupt (dangling?) ref, post-receive and post-update hooks\n>   also receive the same args as per valid update / delete\n\nSuonds sensible.\n\n> - For deleting non-existent refs:\n>   - post-receive shall have empty stdin for those refs\n>   - post-update shall have an empty arg for those refs\n\nI do not think these hooks should see names of refs that ended up being a\nno-op. If the push is only about attempting to delete a ref that did not\nexist, these hooks should not even get called. If there were other refs\nthat got updated, these hooks have to be called, but they should not be\ntold about the no-op.  IOW\n\n    $ git push $there :no-such-ref master:refs/remotes/origin/master\n\nshould:\n\n (1) not call the post-* hooks if the refs/remotes/origin/master was\n     already pointing at the same commit; or\n\n (2) invoke the post-* hooks if refs/remotes/origin/master is updated, but\n     should tell hooks only about the update of refs/remotes/origin/master.\n\nThat is pretty much in line with how a normal attempt to push the same\ncommit to an already up-to-date ref works.  For example, if you:\n\n    $ git push $there master next\n\nwhen 'master' is lagging and 'next' is already up-to-date, post-update and\npost-receive hooks run and told only about 'master' and not 'next'.\n"},{"id":"176365","messageId":"20110927225546.GA1648@myhost","threadId":"28488","inReplyTo":"7vhb3xyld7.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH/RFC 0/2] Teach receive-pack not to run update hook for corrupt/non existent ref","fromName":"Pang Yan Han","fromEmail":"pangyanhan@gmail.com","sentAt":"2011-09-27T22:55:46Z","receivedAt":"2011-09-27T22:55:46Z","isPatch":true,"sender":{"key":"pangyanhan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/539739?v=4"},"body":"On Tue, Sep 27, 2011 at 09:56:52AM -0700, Junio C Hamano wrote:\n> [offtopic: where does that annoying M-F-T header come from? It even seems\n> to be pointless in this case as it lists the same people as are already on\n> To/Cc/From of the message.]\n\nSorry, it's due to my lack of familiarity with mutt.\n\n> \n> Pang Yan Han <pangyanhan@gmail.com> writes:\n> \n> > Should I reroll this patch with this behaviour:\n> >\n> > - Everything as usual for valid ref updates and deletes\n> > - For deleting corrupt (dangling?) ref, post-receive and post-update hooks\n> >   also receive the same args as per valid update / delete\n> \n> Suonds sensible.\n> \n> > - For deleting non-existent refs:\n> >   - post-receive shall have empty stdin for those refs\n> >   - post-update shall have an empty arg for those refs\n> \n> I do not think these hooks should see names of refs that ended up being a\n> no-op. If the push is only about attempting to delete a ref that did not\n> exist, these hooks should not even get called. If there were other refs\n> that got updated, these hooks have to be called, but they should not be\n> told about the no-op.  IOW\n> \n>     $ git push $there :no-such-ref master:refs/remotes/origin/master\n> \n> should:\n> \n>  (1) not call the post-* hooks if the refs/remotes/origin/master was\n>      already pointing at the same commit; or\n> \n>  (2) invoke the post-* hooks if refs/remotes/origin/master is updated, but\n>      should tell hooks only about the update of refs/remotes/origin/master.\n> \n> That is pretty much in line with how a normal attempt to push the same\n> commit to an already up-to-date ref works.  For example, if you:\n> \n>     $ git push $there master next\n> \n> when 'master' is lagging and 'next' is already up-to-date, post-update and\n> post-receive hooks run and told only about 'master' and not 'next'.\n\nThanks, I will reroll this later.\n"}]}