{"thread":{"id":"36484","subject":"[RFC/PATCH v2 0/3] New 'update-branch' hook","startedAt":"2014-04-23T19:42:37Z","lastAt":"2014-04-23T21:35:54Z","messageCount":12,"participants":["Felipe Contreras","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":3},"messages":[{"id":"239472","messageId":"1398282160-26151-1-git-send-email-felipe.contreras@gmail.com","threadId":"36484","inReplyTo":null,"subject":"[RFC/PATCH v2 0/3] New 'update-branch' hook","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2014-04-23T19:42:37Z","receivedAt":"2014-04-23T19:42:37Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Currently it's not possible to keep track of changes that happen to a branch,\nspecifically; when a branch is created and rebased. This patch series aims to fix that.\n\nThe last patch is the important one, but in the process of testing this I found\nout that the GIT_DIR environment variable is not always set, so the hooks (all\nof them) get confused.\n\nToo many changes since v1 to list them all.\n\n\nFelipe Contreras (3):\n  sh-setup: export GIT_DIR\n  run-command: make sure hooks have always GIT_DIR\n  Add 'update-branch' hook\n\n Documentation/githooks.txt    | 15 +++++++++\n branch.c                      | 12 ++++++-\n builtin/clone.c               |  8 +++--\n builtin/reset.c               |  5 +++\n git-rebase--interactive.sh    |  4 +++\n git-rebase.sh                 |  4 +++\n git-sh-setup.sh               |  1 +\n run-command.c                 | 24 ++++++++++++--\n t/t5408-update-branch-hook.sh | 76 +++++++++++++++++++++++++++++++++++++++++++\n 9 files changed, 144 insertions(+), 5 deletions(-)\n create mode 100755 t/t5408-update-branch-hook.sh\n\n-- \n1.9.2+fc1.1.g5c924db\n"},{"id":"239473","messageId":"1398282160-26151-2-git-send-email-felipe.contreras@gmail.com","threadId":"36484","inReplyTo":"1398282160-26151-1-git-send-email-felipe.contreras@gmail.com","subject":"[RFC/PATCH v2 1/3] sh-setup: export GIT_DIR","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2014-04-23T19:42:38Z","receivedAt":"2014-04-23T19:42:38Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"It is what the clients of this library expect.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n git-sh-setup.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/git-sh-setup.sh b/git-sh-setup.sh\nindex 5f28b32..fb0362f 100644\n--- a/git-sh-setup.sh\n+++ b/git-sh-setup.sh\n@@ -346,6 +346,7 @@ then\n \t\texit 1\n \t}\n \t: ${GIT_OBJECT_DIRECTORY=\"$GIT_DIR/objects\"}\n+\texport GIT_DIR\n fi\n \n peel_committish () {\n-- \n1.9.2+fc1.1.g5c924db\n"},{"id":"239474","messageId":"1398282160-26151-3-git-send-email-felipe.contreras@gmail.com","threadId":"36484","inReplyTo":"1398282160-26151-1-git-send-email-felipe.contreras@gmail.com","subject":"[RFC/PATCH v2 2/3] run-command: make sure hooks have always GIT_DIR","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2014-04-23T19:42:39Z","receivedAt":"2014-04-23T19:42:39Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n run-command.c | 24 ++++++++++++++++++++++--\n 1 file changed, 22 insertions(+), 2 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 75abc47..8e188f6 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -765,12 +765,29 @@ int run_hook_ve(const char *const *env, const char *name, va_list args)\n \tstruct child_process hook;\n \tstruct argv_array argv = ARGV_ARRAY_INIT;\n \tconst char *p;\n-\tint ret;\n+\tconst char **mod_env = NULL;\n+\tint ret, i = 0;\n+\tstruct strbuf buf = STRBUF_INIT;\n \n \tp = find_hook(name);\n \tif (!p)\n \t\treturn 0;\n \n+\tif (!getenv(GIT_DIR_ENVIRONMENT)) {\n+\t\tif (env)\n+\t\t\tfor (i = 0; env[i]; i++);\n+\n+\t\tmod_env = xcalloc(i + 2, sizeof(*mod_env));\n+\n+\t\tif (env)\n+\t\t\tfor (i = 0; env[i]; i++)\n+\t\t\t\tmod_env[i] = env[i];\n+\n+\t\tstrbuf_addf(&buf, \"GIT_DIR=%s\", get_git_dir());\n+\t\tmod_env[i++] = buf.buf;\n+\t\tmod_env[i++] = NULL;\n+\t}\n+\n \targv_array_push(&argv, p);\n \n \twhile ((p = va_arg(args, const char *)))\n@@ -778,12 +795,15 @@ int run_hook_ve(const char *const *env, const char *name, va_list args)\n \n \tmemset(&hook, 0, sizeof(hook));\n \thook.argv = argv.argv;\n-\thook.env = env;\n+\thook.env = mod_env ? mod_env : env;\n \thook.no_stdin = 1;\n \thook.stdout_to_stderr = 1;\n \n \tret = run_command(&hook);\n \targv_array_clear(&argv);\n+\tstrbuf_release(&buf);\n+\tfree(mod_env);\n+\n \treturn ret;\n }\n \n-- \n1.9.2+fc1.1.g5c924db\n"},{"id":"239475","messageId":"1398282160-26151-4-git-send-email-felipe.contreras@gmail.com","threadId":"36484","inReplyTo":"1398282160-26151-1-git-send-email-felipe.contreras@gmail.com","subject":"[RFC/PATCH v2 3/3] Add 'update-branch' hook","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2014-04-23T19:42:40Z","receivedAt":"2014-04-23T19:42:40Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"This hook is invoked before a branch is updated, either when a branch is\ncreated or updated with 'git branch', or when it's rebased with 'git\nrebase'.  It receives three parameters; the name of the branch, the\nSHA-1 of the latest commit, and the SHA-1 of the first commit of the\nbranch.\n\nWhen a branch is created the first and last commits of the branch are\nthe same, however when a branch is rebased they are not. If the SHA-1 of\nthe first commit of the branch is not available (e.g. git reset) a null\nSHA-1 (40 zeroes) would be passed.\n\nThe hook exit status can be used to deny the branch update.\n\nIt can be used to verify the validity of branch names, and also to keep\ntrack of the origin point of a branch, which is otherwise not possible\nto find out [1].\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/198587\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n Documentation/githooks.txt    | 15 +++++++++\n branch.c                      | 12 ++++++-\n builtin/clone.c               |  8 +++--\n builtin/reset.c               |  5 +++\n git-rebase--interactive.sh    |  4 +++\n git-rebase.sh                 |  4 +++\n t/t5408-update-branch-hook.sh | 76 +++++++++++++++++++++++++++++++++++++++++++\n 7 files changed, 121 insertions(+), 3 deletions(-)\n create mode 100755 t/t5408-update-branch-hook.sh\n\ndiff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\nindex d954bf6..4a44262 100644\n--- a/Documentation/githooks.txt\n+++ b/Documentation/githooks.txt\n@@ -381,6 +381,21 @@ rebase::\n The commits are guaranteed to be listed in the order that they were\n processed by rebase.\n \n+update-branch\n+~~~~~~~~~~~~~\n+\n+This hook is invoked before a branch is updated, either when a branch is\n+created or updated with 'git branch', or when it's rebased with 'git rebase'.\n+It receives three parameters; the name of the branch, the SHA-1 of the latest\n+commit, and the SHA-1 of the first commit of the branch.\n+\n+When a branch is created the first and last commits of the branch are the same,\n+however when a branch is rebased they are not. If the SHA-1 of the first\n+commit of the branch is not available (e.g. git reset) a null SHA-1 (40 zeroes)\n+would be passed.\n+\n+The hook exit status can be used to deny the branch update.\n+\n \n GIT\n ---\ndiff --git a/branch.c b/branch.c\nindex 660097b..7ec0be5 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -4,6 +4,7 @@\n #include \"refs.h\"\n #include \"remote.h\"\n #include \"commit.h\"\n+#include \"run-command.h\"\n \n struct tracking {\n \tstruct refspec spec;\n@@ -234,6 +235,7 @@ void create_branch(const char *head,\n \tint forcing = 0;\n \tint dont_change_ref = 0;\n \tint explicit_tracking = 0;\n+\tunsigned const char *orig_sha1;\n \n \tif (track == BRANCH_TRACK_EXPLICIT || track == BRANCH_TRACK_OVERRIDE)\n \t\texplicit_tracking = 1;\n@@ -304,9 +306,17 @@ void create_branch(const char *head,\n \tif (real_ref && track)\n \t\tsetup_tracking(ref.buf + 11, real_ref, track, quiet);\n \n-\tif (!dont_change_ref)\n+\tif (!dont_change_ref) {\n+\t\torig_sha1 = forcing ? null_sha1 : sha1;\n+\t\tif (run_hook_le(NULL, \"update-branch\", ref.buf + 11,\n+\t\t\t\t\tsha1_to_hex(sha1), sha1_to_hex(orig_sha1), NULL)) {\n+\t\t\tunlock_ref(lock);\n+\t\t\tdie(\"hook 'update-branch' returned error\");\n+\t\t}\n+\n \t\tif (write_ref_sha1(lock, sha1, msg) < 0)\n \t\t\tdie_errno(_(\"Failed to write ref\"));\n+\t}\n \n \tstrbuf_release(&ref);\n \tfree(real_ref);\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 9b3c04d..d68c49b 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -581,7 +581,7 @@ static void update_remote_refs(const struct ref *refs,\n \t}\n }\n \n-static void update_head(const struct ref *our, const struct ref *remote,\n+static int update_head(const struct ref *our, const struct ref *remote,\n \t\t\tconst char *msg)\n {\n \tif (our && starts_with(our->name, \"refs/heads/\")) {\n@@ -589,6 +589,9 @@ static void update_head(const struct ref *our, const struct ref *remote,\n \t\tcreate_symref(\"HEAD\", our->name, NULL);\n \t\tif (!option_bare) {\n \t\t\tconst char *head = skip_prefix(our->name, \"refs/heads/\");\n+\t\t\tif (run_hook_le(NULL, \"update-branch\", head,\n+\t\t\t\t\t\tsha1_to_hex(our->old_sha1), sha1_to_hex(our->old_sha1), NULL))\n+\t\t\t\treturn 1;\n \t\t\tupdate_ref(msg, \"HEAD\", our->old_sha1, NULL, 0, DIE_ON_ERR);\n \t\t\tinstall_branch_config(0, head, option_origin, our->name);\n \t\t}\n@@ -606,6 +609,7 @@ static void update_head(const struct ref *our, const struct ref *remote,\n \t\tupdate_ref(msg, \"HEAD\", remote->old_sha1,\n \t\t\t   NULL, REF_NODEREF, DIE_ON_ERR);\n \t}\n+\treturn 0;\n }\n \n static int checkout(void)\n@@ -987,7 +991,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tupdate_remote_refs(refs, mapped_refs, remote_head_points_at,\n \t\t\t   branch_top.buf, reflog_msg.buf, transport, !is_local);\n \n-\tupdate_head(our_head_points_at, remote_head, reflog_msg.buf);\n+\terr = update_head(our_head_points_at, remote_head, reflog_msg.buf);\n \n \ttransport_unlock_pack(transport);\n \ttransport_disconnect(transport);\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex f4e0875..9d8cc1f 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -246,11 +246,16 @@ static int reset_refs(const char *rev, const unsigned char *sha1)\n \tstruct strbuf msg = STRBUF_INIT;\n \tunsigned char *orig = NULL, sha1_orig[20],\n \t\t*old_orig = NULL, sha1_old_orig[20];\n+\tconst char *head;\n \n \tif (!get_sha1(\"ORIG_HEAD\", sha1_old_orig))\n \t\told_orig = sha1_old_orig;\n \tif (!get_sha1(\"HEAD\", sha1_orig)) {\n \t\torig = sha1_orig;\n+\t\thead = resolve_ref_unsafe(\"HEAD\", sha1_orig, 1, NULL);\n+\t\tif (run_hook_le(NULL, \"update-branch\", skip_prefix(head, \"refs/heads/\"),\n+\t\t\t\t\tsha1_to_hex(sha1), sha1_to_hex(null_sha1), NULL))\n+\t\t\treturn 1;\n \t\tset_reflog_message(&msg, \"updating ORIG_HEAD\", NULL);\n \t\tupdate_ref(msg.buf, \"ORIG_HEAD\", orig, old_orig, 0, MSG_ON_ERR);\n \t} else if (old_orig)\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 1c41cbd..1bedb27 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -628,6 +628,10 @@ do_next () {\n \tcase $head_name in\n \trefs/*)\n \t\tmessage=\"$GIT_REFLOG_ACTION: $head_name onto $onto\" &&\n+\t\tif test -x \"$GIT_DIR\"/hooks/update-branch; then\n+\t\t\t\"$GIT_DIR\"/hooks/update-branch $branch_name \\\n+\t\t\t\t$newhead $onto\n+\t\tfi &&\n \t\tgit update-ref -m \"$message\" $head_name $newhead $orig_head &&\n \t\tgit symbolic-ref \\\n \t\t  -m \"$GIT_REFLOG_ACTION: returning to $head_name\" \\\ndiff --git a/git-rebase.sh b/git-rebase.sh\nindex 2c75e9f..3ede5be 100755\n--- a/git-rebase.sh\n+++ b/git-rebase.sh\n@@ -145,6 +145,10 @@ move_to_original_branch () {\n \tcase \"$head_name\" in\n \trefs/*)\n \t\tmessage=\"rebase finished: $head_name onto $onto\"\n+\t\tif test -x \"$GIT_DIR\"/hooks/update-branch; then\n+\t\t\t\"$GIT_DIR\"/hooks/update-branch $branch_name \\\n+\t\t\t\t$(git rev-parse HEAD) $onto\n+\t\tfi &&\n \t\tgit update-ref -m \"$message\" \\\n \t\t\t$head_name $(git rev-parse HEAD) $orig_head &&\n \t\tgit symbolic-ref \\\ndiff --git a/t/t5408-update-branch-hook.sh b/t/t5408-update-branch-hook.sh\nnew file mode 100755\nindex 0000000..8a05aa2\n--- /dev/null\n+++ b/t/t5408-update-branch-hook.sh\n@@ -0,0 +1,76 @@\n+#!/bin/sh\n+\n+test_description='Test the update-branch hook'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\tmkdir -p .git/hooks &&\n+\tcat > .git/hooks/update-branch <<-\\EOF &&\n+\t#!/bin/sh\n+\techo $@ > \"$GIT_DIR\"/update-branch.out\n+\tEOF\n+\tchmod +x .git/hooks/update-branch &&\n+\techo one > content &&\n+\tgit add content &&\n+\tgit commit -a -m one &&\n+\techo two > content &&\n+\tgit commit -a -m two\n+'\n+\n+test_expect_success 'creating a branch' '\n+\ttest_when_finished \"rm -f .git/update-branch.out\" &&\n+\tgit branch new master &&\n+\techo \"new $(git rev-parse master) $(git rev-parse master)\" > expected &&\n+\ttest_cmp expected .git/update-branch.out\n+'\n+\n+test_expect_success 'creating a branch with checkout' '\n+\ttest_when_finished \"rm -f .git/update-branch.out\" &&\n+\tgit checkout -b test master &&\n+\techo three > content &&\n+\tgit commit -a -m three &&\n+\techo \"test $(git rev-parse master) $(git rev-parse master)\" > expected &&\n+\ttest_cmp expected .git/update-branch.out\n+'\n+\n+test_expect_success 'reseting a branch' '\n+\ttest_when_finished \"rm -f .git/update-branch.out\" &&\n+\tgit branch --force new master &&\n+\techo \"new $(git rev-parse master) 0000000000000000000000000000000000000000\" > expected &&\n+\ttest_cmp expected .git/update-branch.out\n+'\n+\n+test_expect_success 'reseting a branch with reset' '\n+\ttest_when_finished \"rm -f .git/update-branch.out\" &&\n+\tgit checkout test &&\n+\tgit reset --hard master &&\n+\techo \"test $(git rev-parse master) 0000000000000000000000000000000000000000\" > expected &&\n+\ttest_cmp expected .git/update-branch.out\n+'\n+\n+test_expect_success 'cloning' '\n+\ttest_when_finished \"rm -f .git/update-branch.out\" &&\n+\tmkdir -p templates/hooks &&\n+\tcat > templates/hooks/update-branch <<-\\EOF &&\n+\t#!/bin/sh\n+\techo $@ > \"$GIT_DIR\"/update-branch.out\n+\tEOF\n+\tchmod +x templates/hooks/update-branch &&\n+\tgit clone --template=templates . clone &&\n+\techo \"test $(git rev-parse test) $(git rev-parse test)\" > expected &&\n+\ttest_cmp expected clone/.git/update-branch.out\n+'\n+\n+test_expect_success 'doing a rebase' '\n+\ttest_when_finished \"rm -f .git/update-branch.out\" &&\n+\tgit checkout -b next master &&\n+\techo three > content &&\n+\tgit commit -a -m three &&\n+\tgit branch test-old test &\n+\tgit rebase -f --onto next master test &&\n+\techo \"test $(git rev-parse HEAD) $(git rev-parse next)\" > expected &&\n+\ttest_cmp expected .git/update-branch.out\n+'\n+\n+test_done\n-- \n1.9.2+fc1.1.g5c924db\n"},{"id":"239478","messageId":"20140423200150.GA31356@sigill.intra.peff.net","threadId":"36484","inReplyTo":"1398282160-26151-2-git-send-email-felipe.contreras@gmail.com","subject":"Re: [RFC/PATCH v2 1/3] sh-setup: export GIT_DIR","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-04-23T20:01:50Z","receivedAt":"2014-04-23T20:01:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 23, 2014 at 02:42:38PM -0500, Felipe Contreras wrote:\n\n> It is what the clients of this library expect.\n\nIs it? Passing GIT_DIR to sub-invocations of git will change how they\ndetermine the repo and working tree. Your patch seems to cause failures\nall over the test suite.\n\nWithout looking too hard, I'd guess the problems are one of:\n\n  1. Setting GIT_DIR fixes the repo directory for all sub-invocations. A\n     script that does \"cd some-other-repo.git && git ...\". You'd need to\n     teach calling scripts to unset GIT_DIR when trying to move to\n     another repo.\n\n  2. If GIT_DIR is set but GIT_WORK_TREE is not, then GIT_WORK_TREE will\n     default to \".\". It might be sufficient to set GIT_WORK_TREE when\n     you are setting GIT_DIR here. But as I said, I didn't look too\n     hard, so there might be other complications.\n\n-Peff\n"},{"id":"239484","messageId":"xmqqwqefyf3l.fsf@gitster.dls.corp.google.com","threadId":"36484","inReplyTo":"20140423200150.GA31356@sigill.intra.peff.net","subject":"Re: [RFC/PATCH v2 1/3] sh-setup: export GIT_DIR","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-23T20:18:22Z","receivedAt":"2014-04-23T20:18:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Wed, Apr 23, 2014 at 02:42:38PM -0500, Felipe Contreras wrote:\n>\n>> It is what the clients of this library expect.\n>\n> Is it? Passing GIT_DIR to sub-invocations of git will change how they\n> determine the repo and working tree. Your patch seems to cause failures\n> all over the test suite.\n>\n> Without looking too hard, I'd guess the problems are one of:\n>\n>   1. Setting GIT_DIR fixes the repo directory for all sub-invocations. A\n>      script that does \"cd some-other-repo.git && git ...\". You'd need to\n>      teach calling scripts to unset GIT_DIR when trying to move to\n>      another repo.\n>\n>   2. If GIT_DIR is set but GIT_WORK_TREE is not, then GIT_WORK_TREE will\n>      default to \".\". It might be sufficient to set GIT_WORK_TREE when\n>      you are setting GIT_DIR here. But as I said, I didn't look too\n>      hard, so there might be other complications.\n>\n> -Peff\n\nI do not recall the details but I do remember that it is very\ndeliberate not to export GIT_DIR but only set in git-sh-setup.\n"},{"id":"239486","messageId":"xmqqsip3yemg.fsf@gitster.dls.corp.google.com","threadId":"36484","inReplyTo":"1398282160-26151-4-git-send-email-felipe.contreras@gmail.com","subject":"Re: [RFC/PATCH v2 3/3] Add 'update-branch' hook","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-23T20:28:39Z","receivedAt":"2014-04-23T20:28:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> This hook is invoked before a branch is updated, either when a branch is\n> created or updated with 'git branch', or when it's rebased with 'git\n> rebase'.  It receives three parameters; the name of the branch, the\n> SHA-1 of the latest commit, and the SHA-1 of the first commit of the\n> branch.\n>\n> When a branch is created the first and last commits of the branch are\n> the same, however when a branch is rebased they are not. If the SHA-1 of\n> the first commit of the branch is not available (e.g. git reset) a null\n> SHA-1 (40 zeroes) would be passed.\n>\n> The hook exit status can be used to deny the branch update.\n>\n> It can be used to verify the validity of branch names, and also to keep\n> track of the origin point of a branch, which is otherwise not possible\n> to find out [1].\n\nPlease call it pre-update-branch at least, unless you want to make\nsure that time spent on others in the discussion thread for the\nprevious round becomes wasted.\n\n> +update-branch\n> +~~~~~~~~~~~~~\n> +\n> +This hook is invoked before a branch is updated, either when a branch is\n> +created or updated with 'git branch', or when it's rebased with 'git rebase'.\n\nDoes \"git checkout -B aBranch\" count?\n\nDoes \"git reset $there\" count?\n\nI guess \"git commit\" or \"git merge\" on a branch to advance its tip\nin a usual way would not count (and I can think of good reasons why\nthey should not count), but it is only a weak \"guess\".  The above\ntwo lines does not give readers enough hint to determine if \"branch\nand rebase will be the only two and no other command will ever\ntrigger\" or \"branch and rebase are only examples---for others, guess\non your own\" (and if the latter, enough clue to use in guessing).\nI cannot guess if \"git fetch $there $that:refs/heads/master\" should\ntrigger the hook, for example.\n\nTo put it another way.\n\nHow does one, who is adding a new command that causes a branch tip\nto be updated, to decide if it should trigger this hook?  What are\nthe common things among commands that can update branch tips that\nthis hook gets called that are missing from commands that update\nbranch tips that this hook does not get called?\n"},{"id":"239487","messageId":"xmqqoazrye6w.fsf@gitster.dls.corp.google.com","threadId":"36484","inReplyTo":"1398282160-26151-3-git-send-email-felipe.contreras@gmail.com","subject":"Re: [RFC/PATCH v2 2/3] run-command: make sure hooks have always GIT_DIR","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-23T20:37:59Z","receivedAt":"2014-04-23T20:37:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n\nWhy is this a good change?  From a zero-line log message, I cannot\neven tell if this is trying to fix some problem, or trying to give\nnew capabilities to hooks.\n\nHow does it prevent existing hook scripts from suddenly start\nmisbehaving, where they do *not* expect to see an explicit GIT_DIR\npointing at the original repository hook gets run in exported into\ntheir environment?  For example, one of my post-receive hooks in a\nrepository I push into records $cwd (which is the GIT_DIR of\nreceiving repository), chdir's to another repository and then\nexecutes \"git pull $cwd\" from there, and that relies on the fact\nthat being at the top-level of that other repository without GIT_DIR\nenvironment pointing at elsewhere but having .git directory in that\ntop-level repository is sufficient to kick the auto-discovery of the\nrepository that receives the \"pull\" in order to work correctly.\n\n\n\n> ---\n>  run-command.c | 24 ++++++++++++++++++++++--\n>  1 file changed, 22 insertions(+), 2 deletions(-)\n>\n> diff --git a/run-command.c b/run-command.c\n> index 75abc47..8e188f6 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -765,12 +765,29 @@ int run_hook_ve(const char *const *env, const char *name, va_list args)\n>  \tstruct child_process hook;\n>  \tstruct argv_array argv = ARGV_ARRAY_INIT;\n>  \tconst char *p;\n> -\tint ret;\n> +\tconst char **mod_env = NULL;\n> +\tint ret, i = 0;\n> +\tstruct strbuf buf = STRBUF_INIT;\n>  \n>  \tp = find_hook(name);\n>  \tif (!p)\n>  \t\treturn 0;\n>  \n> +\tif (!getenv(GIT_DIR_ENVIRONMENT)) {\n> +\t\tif (env)\n> +\t\t\tfor (i = 0; env[i]; i++);\n> +\n> +\t\tmod_env = xcalloc(i + 2, sizeof(*mod_env));\n> +\n> +\t\tif (env)\n> +\t\t\tfor (i = 0; env[i]; i++)\n> +\t\t\t\tmod_env[i] = env[i];\n> +\n> +\t\tstrbuf_addf(&buf, \"GIT_DIR=%s\", get_git_dir());\n> +\t\tmod_env[i++] = buf.buf;\n> +\t\tmod_env[i++] = NULL;\n> +\t}\n> +\n>  \targv_array_push(&argv, p);\n>  \n>  \twhile ((p = va_arg(args, const char *)))\n> @@ -778,12 +795,15 @@ int run_hook_ve(const char *const *env, const char *name, va_list args)\n>  \n>  \tmemset(&hook, 0, sizeof(hook));\n>  \thook.argv = argv.argv;\n> -\thook.env = env;\n> +\thook.env = mod_env ? mod_env : env;\n>  \thook.no_stdin = 1;\n>  \thook.stdout_to_stderr = 1;\n>  \n>  \tret = run_command(&hook);\n>  \targv_array_clear(&argv);\n> +\tstrbuf_release(&buf);\n> +\tfree(mod_env);\n> +\n>  \treturn ret;\n>  }\n"},{"id":"239496","messageId":"53582bb6ceb64_24448772ecaf@nysa.notmuch","threadId":"36484","inReplyTo":"xmqqsip3yemg.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC/PATCH v2 3/3] Add 'update-branch' hook","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2014-04-23T21:08:06Z","receivedAt":"2014-04-23T21:08:06Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Junio C Hamano wrote:\n> Felipe Contreras <felipe.contreras@gmail.com> writes:\n> \n> > This hook is invoked before a branch is updated, either when a branch is\n> > created or updated with 'git branch', or when it's rebased with 'git\n> > rebase'.  It receives three parameters; the name of the branch, the\n> > SHA-1 of the latest commit, and the SHA-1 of the first commit of the\n> > branch.\n> >\n> > When a branch is created the first and last commits of the branch are\n> > the same, however when a branch is rebased they are not. If the SHA-1 of\n> > the first commit of the branch is not available (e.g. git reset) a null\n> > SHA-1 (40 zeroes) would be passed.\n> >\n> > The hook exit status can be used to deny the branch update.\n> >\n> > It can be used to verify the validity of branch names, and also to keep\n> > track of the origin point of a branch, which is otherwise not possible\n> > to find out [1].\n> \n> Please call it pre-update-branch at least,\n\nI will do so when I see a good argument for it.\n\n> unless you want to make sure that time spent on others in the discussion\n> thread for the previous round becomes wasted.\n\nUnless you want to make sure the time *I* spent in the discussion is wasted.\n\nI'm still waiting for replies to my last arguments.\n\n> > +update-branch\n> > +~~~~~~~~~~~~~\n> > +\n> > +This hook is invoked before a branch is updated, either when a branch is\n> > +created or updated with 'git branch', or when it's rebased with 'git rebase'.\n> \n> Does \"git checkout -B aBranch\" count?\n\nYes.\n\n> Does \"git reset $there\" count?\n\nYes.\n\n> I guess \"git commit\" or \"git merge\" on a branch to advance its tip\n> in a usual way would not count (and I can think of good reasons why\n> they should not count), but it is only a weak \"guess\".  The above\n> two lines does not give readers enough hint to determine if \"branch\n> and rebase will be the only two and no other command will ever\n> trigger\" or \"branch and rebase are only examples---for others, guess\n> on your own\" (and if the latter, enough clue to use in guessing).\n> I cannot guess if \"git fetch $there $that:refs/heads/master\" should\n> trigger the hook, for example.\n> \n> To put it another way.\n> \n> How does one, who is adding a new command that causes a branch tip\n> to be updated, to decide if it should trigger this hook?  What are\n> the common things among commands that can update branch tips that\n> this hook gets called that are missing from commands that update\n> branch tips that this hook does not get called?\n\nI guess the guideline should be: if the branch is clearly moving forward the\nhook is not called, if the branch is manually changed in any way, it should.\n\nThis omits non-porcelain commands, such as update-ref. I don't think those\nshould trigger the hook.\n\n-- \nFelipe Contreras\n"},{"id":"239498","messageId":"53582d78e82e6_24448772ecf8@nysa.notmuch","threadId":"36484","inReplyTo":"xmqqoazrye6w.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC/PATCH v2 2/3] run-command: make sure hooks have always GIT_DIR","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2014-04-23T21:15:36Z","receivedAt":"2014-04-23T21:15:36Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Junio C Hamano wrote:\n> Felipe Contreras <felipe.contreras@gmail.com> writes:\n> \n> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n> \n> Why is this a good change?\n\nWhen a hook is called from a command without NEED_WORK_TREE, GIT_DIR is not set\n(e.g. git branch).\n\n> How does it prevent existing hook scripts from suddenly start\n> misbehaving, where they do *not* expect to see an explicit GIT_DIR\n> pointing at the original repository hook gets run in exported into\n> their environment?\n\nFine, I'll use \"${GIT_DIR-.git}\" in my hook tests.\n\n> For example, one of my post-receive hooks in a repository I push into records\n> $cwd (which is the GIT_DIR of receiving repository), chdir's to another\n> repository and then executes \"git pull $cwd\" from there, and that relies on\n> the fact that being at the top-level of that other repository without GIT_DIR\n> environment pointing at elsewhere but having .git directory in that top-level\n> repository is sufficient to kick the auto-discovery of the repository that\n> receives the \"pull\" in order to work correctly.\n\nLet's hope post-receive is never called from a command that has NEED_WORK_TREE then.\n\n-- \nFelipe Contreras\n"},{"id":"239497","messageId":"xmqq8uqvyc53.fsf@gitster.dls.corp.google.com","threadId":"36484","inReplyTo":"53582bb6ceb64_24448772ecaf@nysa.notmuch","subject":"Re: [RFC/PATCH v2 3/3] Add 'update-branch' hook","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-23T21:22:16Z","receivedAt":"2014-04-23T21:22:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> Junio C Hamano wrote:\n>> Felipe Contreras <felipe.contreras@gmail.com> writes:\n>> \n>> > This hook is invoked before a branch is updated, either when a branch is\n>> > created or updated with 'git branch', or when it's rebased with 'git\n>> > rebase'.  It receives three parameters; the name of the branch, the\n>> > SHA-1 of the latest commit, and the SHA-1 of the first commit of the\n>> > branch.\n>> >\n>> > When a branch is created the first and last commits of the branch are\n>> > the same, however when a branch is rebased they are not. If the SHA-1 of\n>> > the first commit of the branch is not available (e.g. git reset) a null\n>> > SHA-1 (40 zeroes) would be passed.\n>> >\n>> > The hook exit status can be used to deny the branch update.\n>> >\n>> > It can be used to verify the validity of branch names, and also to keep\n>> > track of the origin point of a branch, which is otherwise not possible\n>> > to find out [1].\n>> \n>> Please call it pre-update-branch at least,\n>\n> I will do so when I see a good argument for it.\n\nIf you choose to ignore \"a user cannot tell from the name\nupdate-branch when it will be called, we cannot introduce\npost-update-branch later without making things more inconsistent if\nwe do not name it pre-something\" and label them not \"a good\nargument\", then I do not have anything to say to you.\n"},{"id":"239502","messageId":"5358323a29162_24448772ec5a@nysa.notmuch","threadId":"36484","inReplyTo":"xmqq8uqvyc53.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC/PATCH v2 3/3] Add 'update-branch' hook","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2014-04-23T21:35:54Z","receivedAt":"2014-04-23T21:35:54Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Junio C Hamano wrote:\n> Felipe Contreras <felipe.contreras@gmail.com> writes:\n> \n> > Junio C Hamano wrote:\n> >> Felipe Contreras <felipe.contreras@gmail.com> writes:\n> >> \n> >> > This hook is invoked before a branch is updated, either when a branch is\n> >> > created or updated with 'git branch', or when it's rebased with 'git\n> >> > rebase'.  It receives three parameters; the name of the branch, the\n> >> > SHA-1 of the latest commit, and the SHA-1 of the first commit of the\n> >> > branch.\n> >> >\n> >> > When a branch is created the first and last commits of the branch are\n> >> > the same, however when a branch is rebased they are not. If the SHA-1 of\n> >> > the first commit of the branch is not available (e.g. git reset) a null\n> >> > SHA-1 (40 zeroes) would be passed.\n> >> >\n> >> > The hook exit status can be used to deny the branch update.\n> >> >\n> >> > It can be used to verify the validity of branch names, and also to keep\n> >> > track of the origin point of a branch, which is otherwise not possible\n> >> > to find out [1].\n> >> \n> >> Please call it pre-update-branch at least,\n> >\n> > I will do so when I see a good argument for it.\n> \n> If you choose to ignore \"a user cannot tell from the name\n> update-branch when it will be called, we cannot introduce\n> post-update-branch later without making things more inconsistent if\n> we do not name it pre-something\" and label them not \"a good\n> argument\", then I do not have anything to say to you.\n\nI did not ignore it, it's an invalid argument, and I explained why.\n\nYou are the one making the assumption that there will be a post-udpate-branch\nwithout actually providing any reasoning *why* we would want such a hook.\n\n-- \nFelipe Contreras\n"}]}