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

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

[Felipe Contreras]

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

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

[Felipe Contreras]

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(+)

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

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

[Jeff King]

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

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

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 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;
 }
 
-- 
1.9.2+fc1.1.g5c924db

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

[Junio C Hamano]

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.



> ---
>  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;
>  }

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

[Felipe Contreras]

Junio C Hamano wrote:
> 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.

> 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

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

[Felipe Contreras]

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

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

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

[Junio C Hamano]

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, unless you want to make
sure that time spent on others in the discussion thread for the
previous round becomes wasted.

> +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?

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

[Felipe Contreras]

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.

> 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.

> > +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.

> 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

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

[Junio C Hamano]

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.

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

[Felipe Contreras]

Junio C Hamano wrote:
> 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