threads / rfc / 36484

RFC v2, 3 partsNew 'update-branch' hook

Subject: [RFC/PATCH v2 0/3] New 'update-branch' hook

## tl;dr

12 messages between Apr 23, 2014 and Apr 23, 2014. Diffs are folded; open one to read it.

replies: 11people: 3as markdown or json

Felipe Contreras· Apr 23, 2014, 19:42 UTC · lore

Currently it's not possible to keep track of changes that happen to a branch, specifically; when a branch is created and rebased. This patch series aims to fix that.

The last patch is the important one, but in the process of testing this I found out that the GIT_DIR environment variable is not always set, so the hooks (all of them) get confused.

Too many changes since v1 to list them all.
Felipe Contreras (3):
  sh-setup: export GIT_DIR
  run-command: make sure hooks have always GIT_DIR
  Add 'update-branch' hook
 Documentation/githooks.txt    | 15 +++++++++
 branch.c                      | 12 ++++++-
 builtin/clone.c               |  8 +++--
 builtin/reset.c               |  5 +++
 git-rebase--interactive.sh    |  4 +++
 git-rebase.sh                 |  4 +++
 git-sh-setup.sh               |  1 +
 run-command.c                 | 24 ++++++++++++--
 t/t5408-update-branch-hook.sh | 76 +++++++++++++++++++++++++++++++++++++++++++
 9 files changed, 144 insertions(+), 5 deletions(-)
 create mode 100755 t/t5408-update-branch-hook.sh
-- 
1.9.2+fc1.1.g5c924db
Felipe Contreras· Apr 23, 2014, 19:42 UTC · re: Felipe Contreras · lore

[RFC/PATCH v2 1/3] sh-setup: export GIT_DIR

It is what the clients of this library expect.
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 git-sh-setup.sh | 1 +
 1 file changed, 1 insertion(+)
Show changes to git-sh-setup.sh +1 −0
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 5f28b32..fb0362f 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -346,6 +346,7 @@ then
 		exit 1
 	}
 	: ${GIT_OBJECT_DIRECTORY="$GIT_DIR/objects"}
+	export GIT_DIR
 fi
 
 peel_committish () {
-- 
1.9.2+fc1.1.g5c924db
Jeff King· Apr 23, 2014, 20:01 UTC · re: Felipe Contreras · lore

Re: [RFC/PATCH v2 1/3] sh-setup: export GIT_DIR

On Wed, Apr 23, 2014 at 02:42:38PM -0500, Felipe Contreras wrote:
> It is what the clients of this library expect.

Is it? Passing GIT_DIR to sub-invocations of git will change how they determine the repo and working tree. Your patch seems to cause failures all over the test suite.

Without looking too hard, I'd guess the problems are one of:
  1. Setting GIT_DIR fixes the repo directory for all sub-invocations. A
     script that does "cd some-other-repo.git && git ...". You'd need to
     teach calling scripts to unset GIT_DIR when trying to move to
     another repo.
  2. If GIT_DIR is set but GIT_WORK_TREE is not, then GIT_WORK_TREE will
     default to ".". It might be sufficient to set GIT_WORK_TREE when
     you are setting GIT_DIR here. But as I said, I didn't look too
     hard, so there might be other complications.
-Peff
Junio C Hamano· Apr 23, 2014, 20:18 UTC · re: Jeff King · lore

Re: [RFC/PATCH v2 1/3] sh-setup: export GIT_DIR

Jeff King <peff@peff.net> writes:
Show 21 quoted lines
> On Wed, Apr 23, 2014 at 02:42:38PM -0500, Felipe Contreras wrote:
>
>> It is what the clients of this library expect.
>
> Is it? Passing GIT_DIR to sub-invocations of git will change how they
> determine the repo and working tree. Your patch seems to cause failures
> all over the test suite.
>
> Without looking too hard, I'd guess the problems are one of:
>
>   1. Setting GIT_DIR fixes the repo directory for all sub-invocations. A
>      script that does "cd some-other-repo.git && git ...". You'd need to
>      teach calling scripts to unset GIT_DIR when trying to move to
>      another repo.
>
>   2. If GIT_DIR is set but GIT_WORK_TREE is not, then GIT_WORK_TREE will
>      default to ".". It might be sufficient to set GIT_WORK_TREE when
>      you are setting GIT_DIR here. But as I said, I didn't look too
>      hard, so there might be other complications.
>
> -Peff

I do not recall the details but I do remember that it is very deliberate not to export GIT_DIR but only set in git-sh-setup.

Felipe Contreras· Apr 23, 2014, 19:42 UTC · re: Felipe Contreras · lore

[RFC/PATCH v2 2/3] run-command: make sure hooks have always GIT_DIR

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 run-command.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)
Show changes to run-command.c +22 −2
diff --git a/run-command.c b/run-command.c
index 75abc47..8e188f6 100644
--- a/run-command.c
+++ b/run-command.c
@@ -765,12 +765,29 @@ int run_hook_ve(const char *const *env, const char *name, va_list args)
 	struct child_process hook;
 	struct argv_array argv = ARGV_ARRAY_INIT;
 	const char *p;
-	int ret;
+	const char **mod_env = NULL;
+	int ret, i = 0;
+	struct strbuf buf = STRBUF_INIT;
 
 	p = find_hook(name);
 	if (!p)
 		return 0;
 
+	if (!getenv(GIT_DIR_ENVIRONMENT)) {
+		if (env)
+			for (i = 0; env[i]; i++);
+
+		mod_env = xcalloc(i + 2, sizeof(*mod_env));
+
+		if (env)
+			for (i = 0; env[i]; i++)
+				mod_env[i] = env[i];
+
+		strbuf_addf(&buf, "GIT_DIR=%s", get_git_dir());
+		mod_env[i++] = buf.buf;
+		mod_env[i++] = NULL;
+	}
+
 	argv_array_push(&argv, p);
 
 	while ((p = va_arg(args, const char *)))
@@ -778,12 +795,15 @@ int run_hook_ve(const char *const *env, const char *name, va_list args)
 
 	memset(&hook, 0, sizeof(hook));
 	hook.argv = argv.argv;
-	hook.env = env;
+	hook.env = mod_env ? mod_env : env;
 	hook.no_stdin = 1;
 	hook.stdout_to_stderr = 1;
 
 	ret = run_command(&hook);
 	argv_array_clear(&argv);
+	strbuf_release(&buf);
+	free(mod_env);
+
 	return ret;
 }
 
-- 
1.9.2+fc1.1.g5c924db
Junio C Hamano· Apr 23, 2014, 20:37 UTC · re: Felipe Contreras · lore

Re: [RFC/PATCH v2 2/3] run-command: make sure hooks have always GIT_DIR

Felipe Contreras <felipe.contreras@gmail.com> writes:
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>

Why is this a good change? From a zero-line log message, I cannot even tell if this is trying to fix some problem, or trying to give new capabilities to hooks.

How does it prevent existing hook scripts from suddenly start misbehaving, where they do *not* expect to see an explicit GIT_DIR pointing at the original repository hook gets run in exported into their environment? For example, one of my post-receive hooks in a repository I push into records $cwd (which is the GIT_DIR of receiving repository), chdir's to another repository and then executes "git pull $cwd" from there, and that relies on the fact that being at the top-level of that other repository without GIT_DIR environment pointing at elsewhere but having .git directory in that top-level repository is sufficient to kick the auto-discovery of the repository that receives the "pull" in order to work correctly.

Show 55 quoted lines
> ---
>  run-command.c | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/run-command.c b/run-command.c
> index 75abc47..8e188f6 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -765,12 +765,29 @@ int run_hook_ve(const char *const *env, const char *name, va_list args)
>  	struct child_process hook;
>  	struct argv_array argv = ARGV_ARRAY_INIT;
>  	const char *p;
> -	int ret;
> +	const char **mod_env = NULL;
> +	int ret, i = 0;
> +	struct strbuf buf = STRBUF_INIT;
>  
>  	p = find_hook(name);
>  	if (!p)
>  		return 0;
>  
> +	if (!getenv(GIT_DIR_ENVIRONMENT)) {
> +		if (env)
> +			for (i = 0; env[i]; i++);
> +
> +		mod_env = xcalloc(i + 2, sizeof(*mod_env));
> +
> +		if (env)
> +			for (i = 0; env[i]; i++)
> +				mod_env[i] = env[i];
> +
> +		strbuf_addf(&buf, "GIT_DIR=%s", get_git_dir());
> +		mod_env[i++] = buf.buf;
> +		mod_env[i++] = NULL;
> +	}
> +
>  	argv_array_push(&argv, p);
>  
>  	while ((p = va_arg(args, const char *)))
> @@ -778,12 +795,15 @@ int run_hook_ve(const char *const *env, const char *name, va_list args)
>  
>  	memset(&hook, 0, sizeof(hook));
>  	hook.argv = argv.argv;
> -	hook.env = env;
> +	hook.env = mod_env ? mod_env : env;
>  	hook.no_stdin = 1;
>  	hook.stdout_to_stderr = 1;
>  
>  	ret = run_command(&hook);
>  	argv_array_clear(&argv);
> +	strbuf_release(&buf);
> +	free(mod_env);
> +
>  	return ret;
>  }
Felipe Contreras· Apr 23, 2014, 21:15 UTC · re: Junio C Hamano · lore

Re: [RFC/PATCH v2 2/3] run-command: make sure hooks have always GIT_DIR

Junio C Hamano wrote:
Show 5 quoted lines
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> 
> Why is this a good change?

When a hook is called from a command without NEED_WORK_TREE, GIT_DIR is not set (e.g. git branch).

> How does it prevent existing hook scripts from suddenly start
> misbehaving, where they do *not* expect to see an explicit GIT_DIR
> pointing at the original repository hook gets run in exported into
> their environment?
Fine, I'll use "${GIT_DIR-.git}" in my hook tests.
Show 7 quoted lines
> For example, one of my post-receive hooks in a repository I push into records
> $cwd (which is the GIT_DIR of receiving repository), chdir's to another
> repository and then executes "git pull $cwd" from there, and that relies on
> the fact that being at the top-level of that other repository without GIT_DIR
> environment pointing at elsewhere but having .git directory in that top-level
> repository is sufficient to kick the auto-discovery of the repository that
> receives the "pull" in order to work correctly.
Let's hope post-receive is never called from a command that has NEED_WORK_TREE then.
-- 
Felipe Contreras
Felipe Contreras· Apr 23, 2014, 19:42 UTC · re: Felipe Contreras · lore

[RFC/PATCH v2 3/3] Add 'update-branch' hook

This hook is invoked before a branch is updated, either when a branch is created or updated with 'git branch', or when it's rebased with 'git rebase'. It receives three parameters; the name of the branch, the SHA-1 of the latest commit, and the SHA-1 of the first commit of the branch.

When a branch is created the first and last commits of the branch are the same, however when a branch is rebased they are not. If the SHA-1 of the first commit of the branch is not available (e.g. git reset) a null SHA-1 (40 zeroes) would be passed.

The hook exit status can be used to deny the branch update.

It can be used to verify the validity of branch names, and also to keep track of the origin point of a branch, which is otherwise not possible to find out [1].

[1] http://thread.gmane.org/gmane.comp.version-control.git/198587
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/githooks.txt    | 15 +++++++++
 branch.c                      | 12 ++++++-
 builtin/clone.c               |  8 +++--
 builtin/reset.c               |  5 +++
 git-rebase--interactive.sh    |  4 +++
 git-rebase.sh                 |  4 +++
 t/t5408-update-branch-hook.sh | 76 +++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 121 insertions(+), 3 deletions(-)
 create mode 100755 t/t5408-update-branch-hook.sh
Show changes to 7 files +121 −3

Documentation/githooks.txt, branch.c, builtin/clone.c, builtin/reset.c, git-rebase--interactive.sh, git-rebase.sh, t/t5408-update-branch-hook.sh

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index d954bf6..4a44262 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -381,6 +381,21 @@ rebase::
 The commits are guaranteed to be listed in the order that they were
 processed by rebase.
 
+update-branch
+~~~~~~~~~~~~~
+
+This hook is invoked before a branch is updated, either when a branch is
+created or updated with 'git branch', or when it's rebased with 'git rebase'.
+It receives three parameters; the name of the branch, the SHA-1 of the latest
+commit, and the SHA-1 of the first commit of the branch.
+
+When a branch is created the first and last commits of the branch are the same,
+however when a branch is rebased they are not. If the SHA-1 of the first
+commit of the branch is not available (e.g. git reset) a null SHA-1 (40 zeroes)
+would be passed.
+
+The hook exit status can be used to deny the branch update.
+
 
 GIT
 ---
diff --git a/branch.c b/branch.c
index 660097b..7ec0be5 100644
--- a/branch.c
+++ b/branch.c
@@ -4,6 +4,7 @@
 #include "refs.h"
 #include "remote.h"
 #include "commit.h"
+#include "run-command.h"
 
 struct tracking {
 	struct refspec spec;
@@ -234,6 +235,7 @@ void create_branch(const char *head,
 	int forcing = 0;
 	int dont_change_ref = 0;
 	int explicit_tracking = 0;
+	unsigned const char *orig_sha1;
 
 	if (track == BRANCH_TRACK_EXPLICIT || track == BRANCH_TRACK_OVERRIDE)
 		explicit_tracking = 1;
@@ -304,9 +306,17 @@ void create_branch(const char *head,
 	if (real_ref && track)
 		setup_tracking(ref.buf + 11, real_ref, track, quiet);
 
-	if (!dont_change_ref)
+	if (!dont_change_ref) {
+		orig_sha1 = forcing ? null_sha1 : sha1;
+		if (run_hook_le(NULL, "update-branch", ref.buf + 11,
+					sha1_to_hex(sha1), sha1_to_hex(orig_sha1), NULL)) {
+			unlock_ref(lock);
+			die("hook 'update-branch' returned error");
+		}
+
 		if (write_ref_sha1(lock, sha1, msg) < 0)
 			die_errno(_("Failed to write ref"));
+	}
 
 	strbuf_release(&ref);
 	free(real_ref);
diff --git a/builtin/clone.c b/builtin/clone.c
index 9b3c04d..d68c49b 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -581,7 +581,7 @@ static void update_remote_refs(const struct ref *refs,
 	}
 }
 
-static void update_head(const struct ref *our, const struct ref *remote,
+static int update_head(const struct ref *our, const struct ref *remote,
 			const char *msg)
 {
 	if (our && starts_with(our->name, "refs/heads/")) {
@@ -589,6 +589,9 @@ static void update_head(const struct ref *our, const struct ref *remote,
 		create_symref("HEAD", our->name, NULL);
 		if (!option_bare) {
 			const char *head = skip_prefix(our->name, "refs/heads/");
+			if (run_hook_le(NULL, "update-branch", head,
+						sha1_to_hex(our->old_sha1), sha1_to_hex(our->old_sha1), NULL))
+				return 1;
 			update_ref(msg, "HEAD", our->old_sha1, NULL, 0, DIE_ON_ERR);
 			install_branch_config(0, head, option_origin, our->name);
 		}
@@ -606,6 +609,7 @@ static void update_head(const struct ref *our, const struct ref *remote,
 		update_ref(msg, "HEAD", remote->old_sha1,
 			   NULL, REF_NODEREF, DIE_ON_ERR);
 	}
+	return 0;
 }
 
 static int checkout(void)
@@ -987,7 +991,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	update_remote_refs(refs, mapped_refs, remote_head_points_at,
 			   branch_top.buf, reflog_msg.buf, transport, !is_local);
 
-	update_head(our_head_points_at, remote_head, reflog_msg.buf);
+	err = update_head(our_head_points_at, remote_head, reflog_msg.buf);
 
 	transport_unlock_pack(transport);
 	transport_disconnect(transport);
diff --git a/builtin/reset.c b/builtin/reset.c
index f4e0875..9d8cc1f 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -246,11 +246,16 @@ static int reset_refs(const char *rev, const unsigned char *sha1)
 	struct strbuf msg = STRBUF_INIT;
 	unsigned char *orig = NULL, sha1_orig[20],
 		*old_orig = NULL, sha1_old_orig[20];
+	const char *head;
 
 	if (!get_sha1("ORIG_HEAD", sha1_old_orig))
 		old_orig = sha1_old_orig;
 	if (!get_sha1("HEAD", sha1_orig)) {
 		orig = sha1_orig;
+		head = resolve_ref_unsafe("HEAD", sha1_orig, 1, NULL);
+		if (run_hook_le(NULL, "update-branch", skip_prefix(head, "refs/heads/"),
+					sha1_to_hex(sha1), sha1_to_hex(null_sha1), NULL))
+			return 1;
 		set_reflog_message(&msg, "updating ORIG_HEAD", NULL);
 		update_ref(msg.buf, "ORIG_HEAD", orig, old_orig, 0, MSG_ON_ERR);
 	} else if (old_orig)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 1c41cbd..1bedb27 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -628,6 +628,10 @@ do_next () {
 	case $head_name in
 	refs/*)
 		message="$GIT_REFLOG_ACTION: $head_name onto $onto" &&
+		if test -x "$GIT_DIR"/hooks/update-branch; then
+			"$GIT_DIR"/hooks/update-branch $branch_name \
+				$newhead $onto
+		fi &&
 		git update-ref -m "$message" $head_name $newhead $orig_head &&
 		git symbolic-ref \
 		  -m "$GIT_REFLOG_ACTION: returning to $head_name" \
diff --git a/git-rebase.sh b/git-rebase.sh
index 2c75e9f..3ede5be 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -145,6 +145,10 @@ move_to_original_branch () {
 	case "$head_name" in
 	refs/*)
 		message="rebase finished: $head_name onto $onto"
+		if test -x "$GIT_DIR"/hooks/update-branch; then
+			"$GIT_DIR"/hooks/update-branch $branch_name \
+				$(git rev-parse HEAD) $onto
+		fi &&
 		git update-ref -m "$message" \
 			$head_name $(git rev-parse HEAD) $orig_head &&
 		git symbolic-ref \
diff --git a/t/t5408-update-branch-hook.sh b/t/t5408-update-branch-hook.sh
new file mode 100755
index 0000000..8a05aa2
--- /dev/null
+++ b/t/t5408-update-branch-hook.sh
@@ -0,0 +1,76 @@
+#!/bin/sh
+
+test_description='Test the update-branch hook'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	mkdir -p .git/hooks &&
+	cat > .git/hooks/update-branch <<-\EOF &&
+	#!/bin/sh
+	echo $@ > "$GIT_DIR"/update-branch.out
+	EOF
+	chmod +x .git/hooks/update-branch &&
+	echo one > content &&
+	git add content &&
+	git commit -a -m one &&
+	echo two > content &&
+	git commit -a -m two
+'
+
+test_expect_success 'creating a branch' '
+	test_when_finished "rm -f .git/update-branch.out" &&
+	git branch new master &&
+	echo "new $(git rev-parse master) $(git rev-parse master)" > expected &&
+	test_cmp expected .git/update-branch.out
+'
+
+test_expect_success 'creating a branch with checkout' '
+	test_when_finished "rm -f .git/update-branch.out" &&
+	git checkout -b test master &&
+	echo three > content &&
+	git commit -a -m three &&
+	echo "test $(git rev-parse master) $(git rev-parse master)" > expected &&
+	test_cmp expected .git/update-branch.out
+'
+
+test_expect_success 'reseting a branch' '
+	test_when_finished "rm -f .git/update-branch.out" &&
+	git branch --force new master &&
+	echo "new $(git rev-parse master) 0000000000000000000000000000000000000000" > expected &&
+	test_cmp expected .git/update-branch.out
+'
+
+test_expect_success 'reseting a branch with reset' '
+	test_when_finished "rm -f .git/update-branch.out" &&
+	git checkout test &&
+	git reset --hard master &&
+	echo "test $(git rev-parse master) 0000000000000000000000000000000000000000" > expected &&
+	test_cmp expected .git/update-branch.out
+'
+
+test_expect_success 'cloning' '
+	test_when_finished "rm -f .git/update-branch.out" &&
+	mkdir -p templates/hooks &&
+	cat > templates/hooks/update-branch <<-\EOF &&
+	#!/bin/sh
+	echo $@ > "$GIT_DIR"/update-branch.out
+	EOF
+	chmod +x templates/hooks/update-branch &&
+	git clone --template=templates . clone &&
+	echo "test $(git rev-parse test) $(git rev-parse test)" > expected &&
+	test_cmp expected clone/.git/update-branch.out
+'
+
+test_expect_success 'doing a rebase' '
+	test_when_finished "rm -f .git/update-branch.out" &&
+	git checkout -b next master &&
+	echo three > content &&
+	git commit -a -m three &&
+	git branch test-old test &
+	git rebase -f --onto next master test &&
+	echo "test $(git rev-parse HEAD) $(git rev-parse next)" > expected &&
+	test_cmp expected .git/update-branch.out
+'
+
+test_done
-- 
1.9.2+fc1.1.g5c924db
Junio C Hamano· Apr 23, 2014, 20:28 UTC · re: Felipe Contreras · lore

Re: [RFC/PATCH v2 3/3] Add 'update-branch' hook

Felipe Contreras <felipe.contreras@gmail.com> writes:
Show 16 quoted lines
> This hook is invoked before a branch is updated, either when a branch is
> created or updated with 'git branch', or when it's rebased with 'git
> rebase'.  It receives three parameters; the name of the branch, the
> SHA-1 of the latest commit, and the SHA-1 of the first commit of the
> branch.
>
> When a branch is created the first and last commits of the branch are
> the same, however when a branch is rebased they are not. If the SHA-1 of
> the first commit of the branch is not available (e.g. git reset) a null
> SHA-1 (40 zeroes) would be passed.
>
> The hook exit status can be used to deny the branch update.
>
> It can be used to verify the validity of branch names, and also to keep
> track of the origin point of a branch, which is otherwise not possible
> to find out [1].

Please call it pre-update-branch at least, unless you want to make sure that time spent on others in the discussion thread for the previous round becomes wasted.

Show 5 quoted lines
> +update-branch
> +~~~~~~~~~~~~~
> +
> +This hook is invoked before a branch is updated, either when a branch is
> +created or updated with 'git branch', or when it's rebased with 'git rebase'.
Does "git checkout -B aBranch" count?
Does "git reset $there" count?

I guess "git commit" or "git merge" on a branch to advance its tip in a usual way would not count (and I can think of good reasons why they should not count), but it is only a weak "guess". The above two lines does not give readers enough hint to determine if "branch and rebase will be the only two and no other command will ever trigger" or "branch and rebase are only examples---for others, guess on your own" (and if the latter, enough clue to use in guessing). I cannot guess if "git fetch $there $that:refs/heads/master" should trigger the hook, for example.

To put it another way.

How does one, who is adding a new command that causes a branch tip to be updated, to decide if it should trigger this hook? What are the common things among commands that can update branch tips that this hook gets called that are missing from commands that update branch tips that this hook does not get called?

Felipe Contreras· Apr 23, 2014, 21:08 UTC · re: Junio C Hamano · lore

Re: [RFC/PATCH v2 3/3] Add 'update-branch' hook

Junio C Hamano wrote:
Show 20 quoted lines
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > This hook is invoked before a branch is updated, either when a branch is
> > created or updated with 'git branch', or when it's rebased with 'git
> > rebase'.  It receives three parameters; the name of the branch, the
> > SHA-1 of the latest commit, and the SHA-1 of the first commit of the
> > branch.
> >
> > When a branch is created the first and last commits of the branch are
> > the same, however when a branch is rebased they are not. If the SHA-1 of
> > the first commit of the branch is not available (e.g. git reset) a null
> > SHA-1 (40 zeroes) would be passed.
> >
> > The hook exit status can be used to deny the branch update.
> >
> > It can be used to verify the validity of branch names, and also to keep
> > track of the origin point of a branch, which is otherwise not possible
> > to find out [1].
> 
> Please call it pre-update-branch at least,
I will do so when I see a good argument for it.
> unless you want to make sure that time spent on others in the discussion
> thread for the previous round becomes wasted.
Unless you want to make sure the time *I* spent in the discussion is wasted.
I'm still waiting for replies to my last arguments.
Show 7 quoted lines
> > +update-branch
> > +~~~~~~~~~~~~~
> > +
> > +This hook is invoked before a branch is updated, either when a branch is
> > +created or updated with 'git branch', or when it's rebased with 'git rebase'.
> 
> Does "git checkout -B aBranch" count?
Yes.
> Does "git reset $there" count?
Yes.
Show 17 quoted lines
> I guess "git commit" or "git merge" on a branch to advance its tip
> in a usual way would not count (and I can think of good reasons why
> they should not count), but it is only a weak "guess".  The above
> two lines does not give readers enough hint to determine if "branch
> and rebase will be the only two and no other command will ever
> trigger" or "branch and rebase are only examples---for others, guess
> on your own" (and if the latter, enough clue to use in guessing).
> I cannot guess if "git fetch $there $that:refs/heads/master" should
> trigger the hook, for example.
> 
> To put it another way.
> 
> How does one, who is adding a new command that causes a branch tip
> to be updated, to decide if it should trigger this hook?  What are
> the common things among commands that can update branch tips that
> this hook gets called that are missing from commands that update
> branch tips that this hook does not get called?

I guess the guideline should be: if the branch is clearly moving forward the hook is not called, if the branch is manually changed in any way, it should.

This omits non-porcelain commands, such as update-ref. I don't think those should trigger the hook.

-- 
Felipe Contreras
Junio C Hamano· Apr 23, 2014, 21:22 UTC · re: Felipe Contreras · lore

Re: [RFC/PATCH v2 3/3] Add 'update-branch' hook

Felipe Contreras <felipe.contreras@gmail.com> writes:
Show 23 quoted lines
> Junio C Hamano wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>> 
>> > This hook is invoked before a branch is updated, either when a branch is
>> > created or updated with 'git branch', or when it's rebased with 'git
>> > rebase'.  It receives three parameters; the name of the branch, the
>> > SHA-1 of the latest commit, and the SHA-1 of the first commit of the
>> > branch.
>> >
>> > When a branch is created the first and last commits of the branch are
>> > the same, however when a branch is rebased they are not. If the SHA-1 of
>> > the first commit of the branch is not available (e.g. git reset) a null
>> > SHA-1 (40 zeroes) would be passed.
>> >
>> > The hook exit status can be used to deny the branch update.
>> >
>> > It can be used to verify the validity of branch names, and also to keep
>> > track of the origin point of a branch, which is otherwise not possible
>> > to find out [1].
>> 
>> Please call it pre-update-branch at least,
>
> I will do so when I see a good argument for it.

If you choose to ignore "a user cannot tell from the name update-branch when it will be called, we cannot introduce post-update-branch later without making things more inconsistent if we do not name it pre-something" and label them not "a good argument", then I do not have anything to say to you.

Felipe Contreras· Apr 23, 2014, 21:35 UTC · re: Junio C Hamano · lore

Re: [RFC/PATCH v2 3/3] Add 'update-branch' hook

Junio C Hamano wrote:
Show 31 quoted lines
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > Junio C Hamano wrote:
> >> Felipe Contreras <felipe.contreras@gmail.com> writes:
> >> 
> >> > This hook is invoked before a branch is updated, either when a branch is
> >> > created or updated with 'git branch', or when it's rebased with 'git
> >> > rebase'.  It receives three parameters; the name of the branch, the
> >> > SHA-1 of the latest commit, and the SHA-1 of the first commit of the
> >> > branch.
> >> >
> >> > When a branch is created the first and last commits of the branch are
> >> > the same, however when a branch is rebased they are not. If the SHA-1 of
> >> > the first commit of the branch is not available (e.g. git reset) a null
> >> > SHA-1 (40 zeroes) would be passed.
> >> >
> >> > The hook exit status can be used to deny the branch update.
> >> >
> >> > It can be used to verify the validity of branch names, and also to keep
> >> > track of the origin point of a branch, which is otherwise not possible
> >> > to find out [1].
> >> 
> >> Please call it pre-update-branch at least,
> >
> > I will do so when I see a good argument for it.
> 
> If you choose to ignore "a user cannot tell from the name
> update-branch when it will be called, we cannot introduce
> post-update-branch later without making things more inconsistent if
> we do not name it pre-something" and label them not "a good
> argument", then I do not have anything to say to you.
I did not ignore it, it's an invalid argument, and I explained why.

You are the one making the assumption that there will be a post-udpate-branch without actually providing any reasoning *why* we would want such a hook.

-- 
Felipe Contreras

← back to recent threads