[RTC/PATCH] Add 'update-branch' hook

[RTC/PATCH] Add 'update-branch' hook

[Felipe Contreras]

This hook is invoked whenever 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 two parameters; the name of the branch, and the
SHA-1 of the latest commit, additionally, if there was a base commit the
branch was rebased onto, a third parameter contains it.

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    |  9 +++++++++
 branch.c                      |  6 ++++++
 builtin/clone.c               |  7 +++++--
 git-rebase--interactive.sh    |  6 +++++-
 git-rebase.sh                 |  6 +++++-
 t/t5408-update-branch-hook.sh | 39 +++++++++++++++++++++++++++++++++++++++
 6 files changed, 69 insertions(+), 4 deletions(-)
 create mode 100755 t/t5408-update-branch-hook.sh

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index d954bf6..9e50697 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -381,6 +381,15 @@ rebase::
 The commits are guaranteed to be listed in the order that they were
 processed by rebase.
 
+update-branch
+~~~~~~~~~~~~~
+
+This hook is invoked whenever 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 two parameters; the name of the branch, and the SHA-1 of the latest
+commit, additionally, if there was a base commit the branch was rebased onto, a
+third parameter contains it.
+
 
 GIT
 ---
diff --git a/branch.c b/branch.c
index 660097b..c2058d1 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;
@@ -304,6 +305,11 @@ void create_branch(const char *head,
 	if (real_ref && track)
 		setup_tracking(ref.buf + 11, real_ref, track, quiet);
 
+	if (run_hook_le(NULL, "update-branch", ref.buf + 11, sha1_to_hex(sha1), NULL)) {
+		unlock_ref(lock);
+		die("hook 'update-branch' returned error");
+	}
+
 	if (!dont_change_ref)
 		if (write_ref_sha1(lock, sha1, msg) < 0)
 			die_errno(_("Failed to write ref"));
diff --git a/builtin/clone.c b/builtin/clone.c
index 9b3c04d..6ec96e5 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -581,9 +581,10 @@ 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)
 {
+	int err = 0;
 	if (our && starts_with(our->name, "refs/heads/")) {
 		/* Local default branch link */
 		create_symref("HEAD", our->name, NULL);
@@ -591,6 +592,7 @@ static void update_head(const struct ref *our, const struct ref *remote,
 			const char *head = skip_prefix(our->name, "refs/heads/");
 			update_ref(msg, "HEAD", our->old_sha1, NULL, 0, DIE_ON_ERR);
 			install_branch_config(0, head, option_origin, our->name);
+			err = run_hook_le(NULL, "update-branch", head, sha1_to_hex(our->old_sha1), NULL);
 		}
 	} else if (our) {
 		struct commit *c = lookup_commit_reference(our->old_sha1);
@@ -606,6 +608,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 err;
 }
 
 static int checkout(void)
@@ -987,7 +990,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/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 1c41cbd..084dc36 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -631,7 +631,11 @@ do_next () {
 		git update-ref -m "$message" $head_name $newhead $orig_head &&
 		git symbolic-ref \
 		  -m "$GIT_REFLOG_ACTION: returning to $head_name" \
-		  HEAD $head_name
+		  HEAD $head_name &&
+		if test -x "$GIT_DIR"/hooks/update-branch; then
+			"$GIT_DIR"/hooks/update-branch $branch_name \
+				$newhead $onto
+		fi
 		;;
 	esac && {
 		test ! -f "$state_dir"/verbose ||
diff --git a/git-rebase.sh b/git-rebase.sh
index 2c75e9f..ededa32 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -149,7 +149,11 @@ move_to_original_branch () {
 			$head_name $(git rev-parse HEAD) $orig_head &&
 		git symbolic-ref \
 			-m "rebase finished: returning to $head_name" \
-			HEAD $head_name ||
+			HEAD $head_name &&
+		if test -x "$GIT_DIR"/hooks/update-branch; then
+			"$GIT_DIR"/hooks/update-branch $branch_name \
+				$(git rev-parse HEAD) $onto
+		fi ||
 		die "$(gettext "Could not move back to $head_name")"
 		;;
 	esac
diff --git a/t/t5408-update-branch-hook.sh b/t/t5408-update-branch-hook.sh
new file mode 100755
index 0000000..d921c0e
--- /dev/null
+++ b/t/t5408-update-branch-hook.sh
@@ -0,0 +1,39 @@
+#!/bin/sh
+
+test_description='Test the update-branch hook'
+
+. ./test-lib.sh
+
+setup () {
+	mkdir -p .git/hooks &&
+	cat > .git/hooks/update-branch <<-'EOF' &&
+	#!/bin/sh
+	echo $@ > .git/update-branch.args
+	EOF
+	chmod +x .git/hooks/update-branch &&
+	echo one > content &&
+	git add content &&
+	git commit -a -m one
+}
+
+setup
+
+test_expect_success 'creating a branch' '
+	git checkout -b test master &&
+	echo two > new &&
+	git add new &&
+	git commit -a -m two
+	echo "test $(git rev-parse master)" > expected &&
+	test_cmp expected .git/update-branch.args
+'
+
+test_expect_success 'doing a rebase' '
+	git checkout -b next master &&
+	echo three > content &&
+	git commit -a -m three &&
+	git rebase --onto next master test &&
+	echo "test $(git rev-parse HEAD) $(git rev-parse next)" > expected &&
+	test_cmp expected .git/update-branch.args
+'
+
+test_done
-- 
1.9.2+fc1.1.g5c924db

Re: [RTC/PATCH] Add 'update-branch' hook

[Eric Sunshine]

On Sun, Apr 20, 2014 at 10:23 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> This hook is invoked whenever 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 two parameters; the name of the branch, and the
> SHA-1 of the latest commit, additionally, if there was a base commit the
> branch was rebased onto, a third parameter contains it.
>
> 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>
> ---
> diff --git a/t/t5408-update-branch-hook.sh b/t/t5408-update-branch-hook.sh
> new file mode 100755
> index 0000000..d921c0e
> --- /dev/null
> +++ b/t/t5408-update-branch-hook.sh
> @@ -0,0 +1,39 @@
> +#!/bin/sh
> +
> +test_description='Test the update-branch hook'
> +
> +. ./test-lib.sh
> +
> +setup () {
> +       mkdir -p .git/hooks &&
> +       cat > .git/hooks/update-branch <<-'EOF' &&
> +       #!/bin/sh
> +       echo $@ > .git/update-branch.args
> +       EOF
> +       chmod +x .git/hooks/update-branch &&
> +       echo one > content &&
> +       git add content &&
> +       git commit -a -m one
> +}
> +
> +setup
> +
> +test_expect_success 'creating a branch' '
> +       git checkout -b test master &&
> +       echo two > new &&
> +       git add new &&
> +       git commit -a -m two

Broken &&-chain.

> +       echo "test $(git rev-parse master)" > expected &&
> +       test_cmp expected .git/update-branch.args
> +'
> +
> +test_expect_success 'doing a rebase' '
> +       git checkout -b next master &&
> +       echo three > content &&
> +       git commit -a -m three &&
> +       git rebase --onto next master test &&
> +       echo "test $(git rev-parse HEAD) $(git rev-parse next)" > expected &&
> +       test_cmp expected .git/update-branch.args
> +'
> +
> +test_done
> --
> 1.9.2+fc1.1.g5c924db

Re: [RTC/PATCH] Add 'update-branch' hook

[Ilya Bobyr]

On 4/20/2014 7:23 PM, Felipe Contreras wrote:
> This hook is invoked whenever 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 two parameters; the name of the branch, and the
> SHA-1 of the latest commit, additionally, if there was a base commit the
> branch was rebased onto, a third parameter contains it.

And the old branch SHA could be found from in the reflog, correct?
Maybe it is possible to add it as an extra argument?
Or if reflog could be used, a note in the description that would say so.

Re: [RTC/PATCH] Add 'update-branch' hook

[Felipe Contreras]

Ilya Bobyr wrote:
> On 4/20/2014 7:23 PM, Felipe Contreras wrote:
> > This hook is invoked whenever 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 two parameters; the name of the branch, and the
> > SHA-1 of the latest commit, additionally, if there was a base commit the
> > branch was rebased onto, a third parameter contains it.
> 
> And the old branch SHA could be found from in the reflog, correct?

Actually the old branch SHA-1 is actually the current one, since the branch
hasn't been updated at that point. Personally I don't see much value in adding
something the script can easily find out.

-- 
Felipe Contreras

Re: [RTC/PATCH] Add 'update-branch' hook

[Ilya Bobyr]

On 4/21/2014 1:49 PM, Felipe Contreras wrote:
> Ilya Bobyr wrote:
>> On 4/20/2014 7:23 PM, Felipe Contreras wrote:
>>> This hook is invoked whenever 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 two parameters; the name of the branch, and the
>>> SHA-1 of the latest commit, additionally, if there was a base commit the
>>> branch was rebased onto, a third parameter contains it.
>> And the old branch SHA could be found from in the reflog, correct?
> Actually the old branch SHA-1 is actually the current one, since the branch
> hasn't been updated at that point. Personally I don't see much value in adding
> something the script can easily find out.

I did not understand that from the description.  That was my next
comment that I did not send just yet.
All the other hooks describe in detail if they are run before or after
the operation, and if it is possible to cancel the operation.
Also, most have names that start with either "pre-" or "post-".
It seems reasonable for both "pre-update-branch" and
"post-update-branch" to exist.
This one would be "pre-update-branch", I guess.

I was also wondering about "git reset".  It could also change the branch
position, right?

Re: [RTC/PATCH] Add 'update-branch' hook

[Felipe Contreras]

Ilya Bobyr wrote:
> On 4/20/2014 7:23 PM, Felipe Contreras wrote:
> > [...]
> >
> > diff --git a/t/t5408-update-branch-hook.sh b/t/t5408-update-branch-hook.sh
> > new file mode 100755
> > index 0000000..d921c0e
> > --- /dev/null
> > +++ b/t/t5408-update-branch-hook.sh
> > @@ -0,0 +1,39 @@
> > +#!/bin/sh
> > +
> > +test_description='Test the update-branch hook'
> > +
> > +. ./test-lib.sh
> > +
> > +setup () {
> > +	mkdir -p .git/hooks &&
> > +	cat > .git/hooks/update-branch <<-'EOF' &&
> > +	#!/bin/sh
> > +	echo $@ > .git/update-branch.args
> > +	EOF
> > +	chmod +x .git/hooks/update-branch &&
> > +	echo one > content &&
> > +	git add content &&
> > +	git commit -a -m one
> > +}
> > +
> > +setup
> 
> According to t/README `setup` should be inside an assertion just as any
> other test:

I have a bunch of 'setup' calls outside such assertions already in other test
scripts. If you know how to put single quotes inside of single quotes in a
shell script, please share that knowledge, otherwise the setup must be outside.

Of course we could do the extremely reduntant:

test_expect_success 'setup' '
  setup
'

-- 
Felipe Contreras

Re: [RTC/PATCH] Add 'update-branch' hook

[Ilya Bobyr]

On 4/20/2014 7:23 PM, Felipe Contreras wrote:
> [...]
>
> diff --git a/t/t5408-update-branch-hook.sh b/t/t5408-update-branch-hook.sh
> new file mode 100755
> index 0000000..d921c0e
> --- /dev/null
> +++ b/t/t5408-update-branch-hook.sh
> @@ -0,0 +1,39 @@
> +#!/bin/sh
> +
> +test_description='Test the update-branch hook'
> +
> +. ./test-lib.sh
> +
> +setup () {
> +	mkdir -p .git/hooks &&
> +	cat > .git/hooks/update-branch <<-'EOF' &&
> +	#!/bin/sh
> +	echo $@ > .git/update-branch.args
> +	EOF
> +	chmod +x .git/hooks/update-branch &&
> +	echo one > content &&
> +	git add content &&
> +	git commit -a -m one
> +}
> +
> +setup

According to t/README `setup` should be inside an assertion just as any
other test:

> Do's, don'ts & things to keep in mind
> -------------------------------------
>
> Here are a few examples of things you probably should and shouldn't do
> when writing tests.
>
> Do:
>
>  - Put all code inside test_expect_success and other assertions.
>
>    Even code that isn't a test per se, but merely some setup code
>    should be inside a test assertion.

Re: [RTC/PATCH] Add 'update-branch' hook

[Felipe Contreras]

Ilya Bobyr wrote:

> Also, most have names that start with either "pre-" or "post-".
> It seems reasonable for both "pre-update-branch" and
> "post-update-branch" to exist.

I don't see what would be the point in that.

> This one would be "pre-update-branch", I guess.
> 
> I was also wondering about "git reset".  It could also change the branch
> position, right?

That's right, maybe that command should call the hook as well.

-- 
Felipe Contreras

Re: [RTC/PATCH] Add 'update-branch' hook

[Felipe Contreras]

Ilya Bobyr wrote:
> On 4/21/2014 2:15 PM, Felipe Contreras wrote:
> > Ilya Bobyr wrote:
> >> On 4/20/2014 7:23 PM, Felipe Contreras wrote:
> >>> [...]
> >>>
> >>> diff --git a/t/t5408-update-branch-hook.sh b/t/t5408-update-branch-hook.sh
> >>> new file mode 100755
> >>> index 0000000..d921c0e
> >>> --- /dev/null
> >>> +++ b/t/t5408-update-branch-hook.sh
> >>> @@ -0,0 +1,39 @@
> >>> +#!/bin/sh
> >>> +
> >>> +test_description='Test the update-branch hook'
> >>> +
> >>> +. ./test-lib.sh
> >>> +
> >>> +setup () {
> >>> +	mkdir -p .git/hooks &&
> >>> +	cat > .git/hooks/update-branch <<-'EOF' &&
> >>> +	#!/bin/sh
> >>> +	echo $@ > .git/update-branch.args
> >>> +	EOF
> >>> +	chmod +x .git/hooks/update-branch &&
> >>> +	echo one > content &&
> >>> +	git add content &&
> >>> +	git commit -a -m one
> >>> +}
> >>> +
> >>> +setup
> >> According to t/README `setup` should be inside an assertion just as any
> >> other test:
> > I have a bunch of 'setup' calls outside such assertions already in other test
> > scripts. If you know how to put single quotes inside of single quotes in a
> > shell script, please share that knowledge, otherwise the setup must be outside.
> >
> > Of course we could do the extremely reduntant:
> >
> > test_expect_success 'setup' '
> >   setup
> > '
> 
> Setup does not look any different from the other tests.
> If you need single quotes you could use double quotes outside.  Though,
> you would have to quote other things as well.
> t0000-basic.sh has a lot of tests that do that.
> Like this, for example:
> 
> test_expect_success 'setup' "
> 	mkdir -p .git/hooks &&
> 	cat > .git/hooks/update-branch <<-\\EOF &&
> 	#!/bin/sh
> 	echo \$@ > .git/update-branch.args
> 	EOF
> 	chmod +x .git/hooks/update-branch &&
> 	echo one > content &&
> 	git add content &&
> 	git commit -a -m one
> "

That is not maintainable at all.

-- 
Felipe Contreras

Re: [RTC/PATCH] Add 'update-branch' hook

[Felipe Contreras]

Ilya Bobyr wrote:
> On 4/21/2014 2:17 PM, Felipe Contreras wrote:
> > Ilya Bobyr wrote:
> >
> >> Also, most have names that start with either "pre-" or "post-".
> >> It seems reasonable for both "pre-update-branch" and
> >> "post-update-branch" to exist.
> > I don't see what would be the point in that.
> 
> Do you see the point in the other hooks doing that?

Yes, there a reason for the existance of those hooks. Now tell me why would
anybody use post-update-branch instead of pre-update-branch?

-- 
Felipe Contreras

Re: [RTC/PATCH] Add 'update-branch' hook

[Ilya Bobyr]

On 4/21/2014 2:15 PM, Felipe Contreras wrote:
> Ilya Bobyr wrote:
>> On 4/20/2014 7:23 PM, Felipe Contreras wrote:
>>> [...]
>>>
>>> diff --git a/t/t5408-update-branch-hook.sh b/t/t5408-update-branch-hook.sh
>>> new file mode 100755
>>> index 0000000..d921c0e
>>> --- /dev/null
>>> +++ b/t/t5408-update-branch-hook.sh
>>> @@ -0,0 +1,39 @@
>>> +#!/bin/sh
>>> +
>>> +test_description='Test the update-branch hook'
>>> +
>>> +. ./test-lib.sh
>>> +
>>> +setup () {
>>> +	mkdir -p .git/hooks &&
>>> +	cat > .git/hooks/update-branch <<-'EOF' &&
>>> +	#!/bin/sh
>>> +	echo $@ > .git/update-branch.args
>>> +	EOF
>>> +	chmod +x .git/hooks/update-branch &&
>>> +	echo one > content &&
>>> +	git add content &&
>>> +	git commit -a -m one
>>> +}
>>> +
>>> +setup
>> According to t/README `setup` should be inside an assertion just as any
>> other test:
> I have a bunch of 'setup' calls outside such assertions already in other test
> scripts. If you know how to put single quotes inside of single quotes in a
> shell script, please share that knowledge, otherwise the setup must be outside.
>
> Of course we could do the extremely reduntant:
>
> test_expect_success 'setup' '
>   setup
> '

Setup does not look any different from the other tests.
If you need single quotes you could use double quotes outside.  Though,
you would have to quote other things as well.
t0000-basic.sh has a lot of tests that do that.
Like this, for example:

test_expect_success 'setup' "
	mkdir -p .git/hooks &&
	cat > .git/hooks/update-branch <<-\\EOF &&
	#!/bin/sh
	echo \$@ > .git/update-branch.args
	EOF
	chmod +x .git/hooks/update-branch &&
	echo one > content &&
	git add content &&
	git commit -a -m one
"

Re: [RTC/PATCH] Add 'update-branch' hook

[Ilya Bobyr]

On 4/21/2014 2:17 PM, Felipe Contreras wrote:
> Ilya Bobyr wrote:
>
>> Also, most have names that start with either "pre-" or "post-".
>> It seems reasonable for both "pre-update-branch" and
>> "post-update-branch" to exist.
> I don't see what would be the point in that.

Do you see the point in the other hooks doing that?

Re: [RTC/PATCH] Add 'update-branch' hook

[Junio C Hamano]

Ilya Bobyr <ilya.bobyr@gmail.com> writes:

> On 4/21/2014 2:17 PM, Felipe Contreras wrote:
>> Ilya Bobyr wrote:
>>
>>> Also, most have names that start with either "pre-" or "post-".
>>> It seems reasonable for both "pre-update-branch" and
>>> "post-update-branch" to exist.
>> I don't see what would be the point in that.
>
> Do you see the point in the other hooks doing that?

pre- and post- are primarily so that people can tell that "pre-
happens before the operation and its primary motivation is to stop
an operation from happening" as opposed to "post- is called after
the fact and there is no way for it to intervene---it is too late;
it is primarily for things like logging" easily.

As long as you can tell what you can use it for and when it is
called from the name of the hook, there is no fundamental reason why
you need to have pre- or post- prefix in your hook names, but unless
there is no other strong reason not to, it is probably a good idea
to follow suit.  There is not much value in trying to be "original"
in naming things, just to be different; it will only confuse the
users.

Re: [RTC/PATCH] Add 'update-branch' hook

[Felipe Contreras]

Ilya Bobyr wrote:
> On Mon, Apr 21, 2014 at 2:35 PM, Felipe Contreras <
> felipe.contreras@gmail.com> wrote:
> > Ilya Bobyr wrote:
> > > test_expect_success 'setup' "
> > >       mkdir -p .git/hooks &&
> > >       cat > .git/hooks/update-branch <<-\\EOF &&
> > >       #!/bin/sh
> > >       echo \$@ > .git/update-branch.args
> > >       EOF
> > >       chmod +x .git/hooks/update-branch &&
> > >       echo one > content &&
> > >       git add content &&
> > >       git commit -a -m one
> > > "
> >
> > That is not maintainable at all.
> 
> Maybe you could explain how is this less maintainable, compared to a separate
> function?

Do I really have to explain that manually escaping a shell script is not
maintainable?

> This is how it is suggested by t/README and how it is done in the other
> test suites.
> I can not see how your case is different, but I might be missing something.

Let's take a cursoy look at `git grep -l "'EOF'" t`.

== t/t0009-prio-queue.sh ==

  cat >expect <<'EOF'
  1
  2
  3
  4
  5
  5
  6
  7
  8
  9
  10
  EOF
  test_expect_success 'basic ordering' '
	  test-prio-queue 2 6 3 10 9 5 7 4 5 8 1 dump >actual &&
	  test_cmp expect actual
  '

Look at that, code outside the cage, not once, but in every test.

== t/t0040-parse-options.sh ==

  cat >>expect <<'EOF'
  list: foo
  list: bar
  list: baz
  EOF
  test_expect_success '--list keeps list of strings' '
	  test-parse-options --list foo --list=bar --list=baz >output &&
	  test_cmp expect output
  '

Once again.

== t/t1411-reflog-show.sh ==
== t/t2020-checkout-detach.sh ==
== t/t3203-branch-output.sh ==
== t/t3412-rebase-root.sh ==
== t/t4014-format-patch.sh ==
== t/t4030-diff-textconv.sh ==

All these do something similar, not once, but many many times.

== t/t4031-diff-rewrite-binary.sh ==

  {
	  echo "#!$SHELL_PATH"
	  cat <<'EOF'
  "$PERL_PATH" -e '$/ = undef; $_ = <>; s/./ord($&)/ge; print $_' < "$1"
  EOF
  } >dump
  chmod +x dump

More code outside.

== t/t4042-diff-textconv-caching.sh ==

  cat >helper <<'EOF'
  #!/bin/sh
  sed 's/^/converted: /' "$@" >helper.out
  cat helper.out
  EOF
  chmod +x helper

== t/t5401-update-hooks.sh ==

  cat >victim.git/hooks/pre-receive <<'EOF'
  #!/bin/sh
  printf %s "$@" >>$GIT_DIR/pre-receive.args
  cat - >$GIT_DIR/pre-receive.stdin
  echo STDOUT pre-receive
  echo STDERR pre-receive >&2
  EOF
  chmod u+x victim.git/hooks/pre-receive

Would you look at that? This is actually a hook test that is changing the hook
*outside* the cage.

== t/t5402-post-merge-hook.sh ==

  for clone in 1 2; do
      cat >clone${clone}/.git/hooks/post-merge <<'EOF'
  #!/bin/sh
  echo $@ >> $GIT_DIR/post-merge.args
  EOF
      chmod u+x clone${clone}/.git/hooks/post-merge
  done

Another hook test with code outside.

== t/t5403-post-checkout-hook.sh ==

Doing the same.

== t/t5516-fetch-push.sh ==

  mk_test_with_hooks() {
	  repo_name=$1
	  mk_test "$@" &&
	  (
		  cd "$repo_name" &&
		  mkdir .git/hooks &&
		  cd .git/hooks &&

		  cat >pre-receive <<-'EOF' &&
		  #!/bin/sh
		  cat - >>pre-receive.actual
		  EOF

		  cat >update <<-'EOF' &&
		  #!/bin/sh
		  printf "%s %s %s\n" "$@" >>update.actual
		  EOF

		  cat >post-receive <<-'EOF' &&
		  #!/bin/sh
		  cat - >>post-receive.actual
		  EOF

		  cat >post-update <<-'EOF' &&
		  #!/bin/sh
		  for ref in "$@"
		  do
			  printf "%s\n" "$ref" >>post-update.actual
		  done
		  EOF

		  chmod +x pre-receive update post-receive post-update
	  )
  }

This one is using a function, just like I am. It's not run outside, but we can
do the same.

== t/t5571-pre-push-hook.sh ==

  write_script "$HOOK" <<'EOF'
  echo "$1" >actual
  echo "$2" >>actual
  cat >>actual
  EOF

Anhoter hook test with code outside.

== t/t7004-tag.sh ==

  cat >fakeeditor <<'EOF'
  #!/bin/sh
  test -n "$1" && exec >"$1"
  echo A signed tag message
  echo from a fake editor.
  EOF
  chmod +x fakeeditor

== t/t7008-grep-binary.sh ==

  cat >nul_to_q_textconv <<'EOF'
  #!/bin/sh
  "$PERL_PATH" -pe 'y/\000/Q/' < "$1"
  EOF
  chmod +x nul_to_q_textconv

== t/t7504-commit-msg-hook.sh ==
== t/t8006-blame-textconv.sh ==
== t/t8007-cat-file-textconv.sh ==
== t/t9138-git-svn-authors-prog.sh ==

Very similar: scripts outside the cage.


In fact my version is actually cleaner than these, because the code that is run
outside the cage is clearly delimited by a function.

-- 
Felipe Contreras

Re: [RTC/PATCH] Add 'update-branch' hook

[Felipe Contreras]

Junio C Hamano wrote:
> Ilya Bobyr <ilya.bobyr@gmail.com> writes:
> 
> > On 4/21/2014 2:17 PM, Felipe Contreras wrote:
> >> Ilya Bobyr wrote:
> >>
> >>> Also, most have names that start with either "pre-" or "post-".
> >>> It seems reasonable for both "pre-update-branch" and
> >>> "post-update-branch" to exist.
> >> I don't see what would be the point in that.
> >
> > Do you see the point in the other hooks doing that?
> 
> pre- and post- are primarily so that people can tell that "pre-
> happens before the operation and its primary motivation is to stop
> an operation from happening" as opposed to "post- is called after
> the fact and there is no way for it to intervene---it is too late;
> it is primarily for things like logging" easily.
> 
> As long as you can tell what you can use it for and when it is
> called from the name of the hook, there is no fundamental reason why
> you need to have pre- or post- prefix in your hook names, but unless
> there is no other strong reason not to, it is probably a good idea
> to follow suit.  There is not much value in trying to be "original"
> in naming things, just to be different; it will only confuse the
> users.

It's not original; there are _already_ hooks without pre/post. And it's not
confusing, "update-branch" doesn't tell much, not any hook name could, that's
what the documentation is for.

-- 
Felipe Contreras

Re: [RTC/PATCH] Add 'update-branch' hook

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> ... there are _already_ hooks without pre/post.

Like commit-msg?  Yes, it would have been nicer if it were named
verify-commit-message or something.

Old mistakes are harder to change because of inertia.  It is not a
good excuse to knowingly make a new mistake to add new exceptions
that the users need to check documentations for, is it?

> And it's not confusing,

A simple fact that Ilya asked the question tells us otherwise ;-)

I personally do not see an immediate need for post-update-branch,
but if the new hook is about intervening an operation, it would be a
good idea to name the hook with "pre-" like other "before doing
something, validate the operation and forbid" hooks.  Otherwise it
would be impossible to later add "post-update-branch" for whatever
reason without inviting "why does pre-update-branch hook is misnamed
as just update-branch, when other validation and post-action pair
are named pre-something and post-something?".

Re: [RTC/PATCH] Add 'update-branch' hook

[Ilya Bobyr]

On 4/21/2014 3:24 PM, Felipe Contreras wrote:
> Ilya Bobyr wrote:
>> On Mon, Apr 21, 2014 at 2:35 PM, Felipe Contreras <
>> felipe.contreras@gmail.com> wrote:
>>> Ilya Bobyr wrote:
>>>> test_expect_success 'setup' "
>>>>       mkdir -p .git/hooks &&
>>>>       cat > .git/hooks/update-branch <<-\\EOF &&
>>>>       #!/bin/sh
>>>>       echo \$@ > .git/update-branch.args
>>>>       EOF
>>>>       chmod +x .git/hooks/update-branch &&
>>>>       echo one > content &&
>>>>       git add content &&
>>>>       git commit -a -m one
>>>> "
>>> That is not maintainable at all.
>> Maybe you could explain how is this less maintainable, compared to a separate
>> function?
> Do I really have to explain that manually escaping a shell script is not
> maintainable?

This is rude.

Here is how you can do it without escaping:

test_expect_success 'setup' '
	mkdir -p .git/hooks &&
	cat > .git/hooks/update-branch <<-\EOF &&
	#!/bin/sh
	echo $@ > .git/update-branch.args
	EOF
	chmod +x .git/hooks/update-branch &&
	echo one > content &&
	git add content &&
	git commit -a -m one
'

It is not different from most of the tests, I think.

>> This is how it is suggested by t/README and how it is done in the other
>> test suites.
>> I can not see how your case is different, but I might be missing something.
> Let's take a cursoy look at `git grep -l "'EOF'" t`.
>
> [...]

So the point is that some existing tests violate best practices?
I do not think this is a good justification to do the same for new tests.

> In fact my version is actually cleaner than these, because the code that is run
> outside the cage is clearly delimited by a function.

It depends on the perspective.
If it fails, the failure would be missed regardless of if it is in a
function or not.
Most examples that you quoted only create files outside test_expect_success.
Even that is not necessary.

I am not telling you how you should write it.
I am just saying that you are breaking one of the recommendations on how
to write tests.
There are different options that adhere to the suggestions in t/README.

Re: [RTC/PATCH] Add 'update-branch' hook

[Ilya Bobyr]

On 4/20/2014 7:23 PM, Felipe Contreras wrote:
> [...]
>
> diff --git a/branch.c b/branch.c
> index 660097b..c2058d1 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;
> @@ -304,6 +305,11 @@ void create_branch(const char *head,
>  	if (real_ref && track)
>  		setup_tracking(ref.buf + 11, real_ref, track, quiet);
>  
> +	if (run_hook_le(NULL, "update-branch", ref.buf + 11, sha1_to_hex(sha1), NULL)) {
> +		unlock_ref(lock);

lock is NULL if dont_change_ref is true.  unlock_ref() would crash in
that case.
You may want to add a test for that.

> +		die("hook 'update-branch' returned error");
> +	}
> +
>  	if (!dont_change_ref)
>  		if (write_ref_sha1(lock, sha1, msg) < 0)
>  			die_errno(_("Failed to write ref"));
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 9b3c04d..6ec96e5 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -581,9 +581,10 @@ 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)
>  {
> +	int err = 0;
>  	if (our && starts_with(our->name, "refs/heads/")) {
>  		/* Local default branch link */
>  		create_symref("HEAD", our->name, NULL);
> @@ -591,6 +592,7 @@ static void update_head(const struct ref *our, const struct ref *remote,
>  			const char *head = skip_prefix(our->name, "refs/heads/");
>  			update_ref(msg, "HEAD", our->old_sha1, NULL, 0, DIE_ON_ERR);
>  			install_branch_config(0, head, option_origin, our->name);
> +			err = run_hook_le(NULL, "update-branch", head, sha1_to_hex(our->old_sha1), NULL);

This is happening after the branch is updated and a config section for
it is created.

>  		}
>  	} else if (our) {
>  		struct commit *c = lookup_commit_reference(our->old_sha1);
> @@ -606,6 +608,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 err;
>  }
>  
>  static int checkout(void)
> @@ -987,7 +990,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/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 1c41cbd..084dc36 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -631,7 +631,11 @@ do_next () {
>  		git update-ref -m "$message" $head_name $newhead $orig_head &&
>  		git symbolic-ref \
>  		  -m "$GIT_REFLOG_ACTION: returning to $head_name" \
> -		  HEAD $head_name
> +		  HEAD $head_name &&
> +		if test -x "$GIT_DIR"/hooks/update-branch; then
> +			"$GIT_DIR"/hooks/update-branch $branch_name \
> +				$newhead $onto
> +		fi

It looks like this is also after the branch was already updated.

>  		;;
>  	esac && {
>  		test ! -f "$state_dir"/verbose ||
> diff --git a/git-rebase.sh b/git-rebase.sh
> index 2c75e9f..ededa32 100755
> --- a/git-rebase.sh
> +++ b/git-rebase.sh
> @@ -149,7 +149,11 @@ move_to_original_branch () {
>  			$head_name $(git rev-parse HEAD) $orig_head &&
>  		git symbolic-ref \
>  			-m "rebase finished: returning to $head_name" \
> -			HEAD $head_name ||
> +			HEAD $head_name &&
> +		if test -x "$GIT_DIR"/hooks/update-branch; then
> +			"$GIT_DIR"/hooks/update-branch $branch_name \
> +				$(git rev-parse HEAD) $onto
> +		fi ||

Same here.

>  		die "$(gettext "Could not move back to $head_name")"
>  		;;
>  	esac
> diff --git a/t/t5408-update-branch-hook.sh b/t/t5408-update-branch-hook.sh
> new file mode 100755
> index 0000000..d921c0e
> --- /dev/null
> +++ b/t/t5408-update-branch-hook.sh
> @@ -0,0 +1,39 @@
> +#!/bin/sh
> +
> +test_description='Test the update-branch hook'
> +
> +. ./test-lib.sh
> +
> +setup () {
> +	mkdir -p .git/hooks &&
> +	cat > .git/hooks/update-branch <<-'EOF' &&
> +	#!/bin/sh
> +	echo $@ > .git/update-branch.args
> +	EOF
> +	chmod +x .git/hooks/update-branch &&
> +	echo one > content &&
> +	git add content &&
> +	git commit -a -m one
> +}
> +
> +setup
> +
> +test_expect_success 'creating a branch' '
> +	git checkout -b test master &&
> +	echo two > new &&
> +	git add new &&
> +	git commit -a -m two
> +	echo "test $(git rev-parse master)" > expected &&
> +	test_cmp expected .git/update-branch.args
> +'
> +
> +test_expect_success 'doing a rebase' '
> +	git checkout -b next master &&
> +	echo three > content &&
> +	git commit -a -m three &&
> +	git rebase --onto next master test &&
> +	echo "test $(git rev-parse HEAD) $(git rev-parse next)" > expected &&
> +	test_cmp expected .git/update-branch.args
> +'
> +
> +test_done

Re: [RTC/PATCH] Add 'update-branch' hook

[Ilya Bobyr]

On 4/21/2014 1:49 PM, Felipe Contreras wrote:
> Ilya Bobyr wrote:
>> On 4/20/2014 7:23 PM, Felipe Contreras wrote:
>>> This hook is invoked whenever 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 two parameters; the name of the branch, and the
>>> SHA-1 of the latest commit, additionally, if there was a base commit the
>>> branch was rebased onto, a third parameter contains it.
>> And the old branch SHA could be found from in the reflog, correct?
> Actually the old branch SHA-1 is actually the current one, since the branch
> hasn't been updated at that point. Personally I don't see much value in adding
> something the script can easily find out.

If the hook is about a branch update, I would expect it to provide both
old and new points for the branch, along with the name.

The fact that for rebases it also provides new base SHA is very
convenient.  As it is an optional argument it may make further extension
of the interface a bit awkward.
So, is seems reasonable to provide both from the very beginning.
I was looking for hooks like that, to maintain certain meta-data about
the branches.
Old SHA would be very useful in that case.

I am not sure if both SHAs are easily available at the point where the
hook is called.

Re: [RTC/PATCH] Add 'update-branch' hook

[Felipe Contreras]

Ilya Bobyr wrote:
> On 4/21/2014 3:24 PM, Felipe Contreras wrote:
> > Ilya Bobyr wrote:
> >> On Mon, Apr 21, 2014 at 2:35 PM, Felipe Contreras <
> >> felipe.contreras@gmail.com> wrote:
> >>> Ilya Bobyr wrote:
> >>>> test_expect_success 'setup' "
> >>>>       mkdir -p .git/hooks &&
> >>>>       cat > .git/hooks/update-branch <<-\\EOF &&
> >>>>       #!/bin/sh
> >>>>       echo \$@ > .git/update-branch.args
> >>>>       EOF
> >>>>       chmod +x .git/hooks/update-branch &&
> >>>>       echo one > content &&
> >>>>       git add content &&
> >>>>       git commit -a -m one
> >>>> "
> >>> That is not maintainable at all.
> >> Maybe you could explain how is this less maintainable, compared to a separate
> >> function?
> > Do I really have to explain that manually escaping a shell script is not
> > maintainable?
> 
> This is rude.

So? I really don't see the need to explain that such a monstrosity would be
unmaintainable, that's a given.

> Here is how you can do it without escaping:
> 
> test_expect_success 'setup' '
> 	mkdir -p .git/hooks &&
> 	cat > .git/hooks/update-branch <<-\EOF &&
> 	#!/bin/sh
> 	echo $@ > .git/update-branch.args
> 	EOF
> 	chmod +x .git/hooks/update-branch &&
> 	echo one > content &&
> 	git add content &&
> 	git commit -a -m one
> '
> 
> It is not different from most of the tests, I think.

This is what I originally asked for.

> >> This is how it is suggested by t/README and how it is done in the other
> >> test suites.
> >> I can not see how your case is different, but I might be missing something.
> > Let's take a cursoy look at `git grep -l "'EOF'" t`.
> >
> > [...]
> 
> So the point is that some existing tests violate best practices?

I don't know what you mean by "best practices", but these are Git's best practices.

> I do not think this is a good justification to do the same for new tests.

It is not a justification to reject a patch either, specially if no better
alternative has been put forward.

Fortunately a better alternative has been put forward, so this is moot.
 
-- 
Felipe Contreras

Re: [RTC/PATCH] Add 'update-branch' hook

[Felipe Contreras]

Ilya Bobyr wrote:
> On 4/21/2014 11:45 PM, Felipe Contreras wrote:
> > [...]
> >>>> This is how it is suggested by t/README and how it is done in the other
> >>>> test suites.
> >>>> I can not see how your case is different, but I might be missing something.
> >>> Let's take a cursoy look at `git grep -l "'EOF'" t`.
> >>>
> >>> [...]
> >> So the point is that some existing tests violate best practices?
> > I don't know what you mean by "best practices", but these are Git's best practices.
> 
> I am talking about recommendations in t/README that I quoted.

Those are *guidelines*, best practices are defined as things you actually do,
as in "actually practice".

-- 
Felipe Contreras

Re: [RTC/PATCH] Add 'update-branch' hook

[Ilya Bobyr]

On 4/21/2014 11:45 PM, Felipe Contreras wrote:
> [...]
>>>> This is how it is suggested by t/README and how it is done in the other
>>>> test suites.
>>>> I can not see how your case is different, but I might be missing something.
>>> Let's take a cursoy look at `git grep -l "'EOF'" t`.
>>>
>>> [...]
>> So the point is that some existing tests violate best practices?
> I don't know what you mean by "best practices", but these are Git's best practices.

I am talking about recommendations in t/README that I quoted.

Re: [RTC/PATCH] Add 'update-branch' hook

[Stephen Leake]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> Ilya Bobyr wrote:
>> On 4/21/2014 2:17 PM, Felipe Contreras wrote:
>> > Ilya Bobyr wrote:
>> >
>> >> Also, most have names that start with either "pre-" or "post-".
>> >> It seems reasonable for both "pre-update-branch" and
>> >> "post-update-branch" to exist.
>> > I don't see what would be the point in that.
>> 
>> Do you see the point in the other hooks doing that?
>
> Yes, there a reason for the existance of those hooks. Now tell me why would
> anybody use post-update-branch instead of pre-update-branch?

I have a branch which should always be recompiled on update;
post-update-branch would be a good place for that.

-- 
-- Stephe

Re: [RTC/PATCH] Add 'update-branch' hook

[Felipe Contreras]

Ilya Bobyr wrote:
> On 4/20/2014 7:23 PM, Felipe Contreras wrote:
> > [...]
> >
> > diff --git a/branch.c b/branch.c
> > index 660097b..c2058d1 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;
> > @@ -304,6 +305,11 @@ void create_branch(const char *head,
> >  	if (real_ref && track)
> >  		setup_tracking(ref.buf + 11, real_ref, track, quiet);
> >  
> > +	if (run_hook_le(NULL, "update-branch", ref.buf + 11, sha1_to_hex(sha1), NULL)) {
> > +		unlock_ref(lock);
> 
> lock is NULL if dont_change_ref is true.  unlock_ref() would crash in
> that case.
> You may want to add a test for that.

That should be easy to fix.
 
> > +		die("hook 'update-branch' returned error");
> > +	}
> > +
> >  	if (!dont_change_ref)
> >  		if (write_ref_sha1(lock, sha1, msg) < 0)
> >  			die_errno(_("Failed to write ref"));
> > diff --git a/builtin/clone.c b/builtin/clone.c
> > index 9b3c04d..6ec96e5 100644
> > --- a/builtin/clone.c
> > +++ b/builtin/clone.c
> > @@ -581,9 +581,10 @@ 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)
> >  {
> > +	int err = 0;
> >  	if (our && starts_with(our->name, "refs/heads/")) {
> >  		/* Local default branch link */
> >  		create_symref("HEAD", our->name, NULL);
> > @@ -591,6 +592,7 @@ static void update_head(const struct ref *our, const struct ref *remote,
> >  			const char *head = skip_prefix(our->name, "refs/heads/");
> >  			update_ref(msg, "HEAD", our->old_sha1, NULL, 0, DIE_ON_ERR);
> >  			install_branch_config(0, head, option_origin, our->name);
> > +			err = run_hook_le(NULL, "update-branch", head, sha1_to_hex(our->old_sha1), NULL);
> 
> This is happening after the branch is updated and a config section for
> it is created.

I see that now, however, I cannot find where in builtin/clone.c is the branch
ref actually updated.

Worst, I don't see how I could possibly configure a hook to be triggered when
cloning, so I cannot test.
 
> >  		}
> >  	} else if (our) {
> >  		struct commit *c = lookup_commit_reference(our->old_sha1);
> > @@ -606,6 +608,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 err;
> >  }
> >  
> >  static int checkout(void)
> > @@ -987,7 +990,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/git-rebase--interactive.sh b/git-rebase--interactive.sh
> > index 1c41cbd..084dc36 100644
> > --- a/git-rebase--interactive.sh
> > +++ b/git-rebase--interactive.sh
> > @@ -631,7 +631,11 @@ do_next () {
> >  		git update-ref -m "$message" $head_name $newhead $orig_head &&
> >  		git symbolic-ref \
> >  		  -m "$GIT_REFLOG_ACTION: returning to $head_name" \
> > -		  HEAD $head_name
> > +		  HEAD $head_name &&
> > +		if test -x "$GIT_DIR"/hooks/update-branch; then
> > +			"$GIT_DIR"/hooks/update-branch $branch_name \
> > +				$newhead $onto
> > +		fi
> 
> It looks like this is also after the branch was already updated.

This and the one below should be easy to fix.

-- 
Felipe Contreras

Re: [RTC/PATCH] Add 'update-branch' hook

[Felipe Contreras]

Ilya Bobyr wrote:
> On 4/21/2014 1:49 PM, Felipe Contreras wrote:
> > Ilya Bobyr wrote:
> >> On 4/20/2014 7:23 PM, Felipe Contreras wrote:
> >>> This hook is invoked whenever 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 two parameters; the name of the branch, and the
> >>> SHA-1 of the latest commit, additionally, if there was a base commit the
> >>> branch was rebased onto, a third parameter contains it.
> >> And the old branch SHA could be found from in the reflog, correct?
> > Actually the old branch SHA-1 is actually the current one, since the branch
> > hasn't been updated at that point. Personally I don't see much value in adding
> > something the script can easily find out.
> 
> If the hook is about a branch update, I would expect it to provide both
> old and new points for the branch, along with the name.

Again, I don't see the the point of passing something that is easy to find out:
`git rev-parse $branch` gives you that information.

> The fact that for rebases it also provides new base SHA is very
> convenient.  As it is an optional argument it may make further extension
> of the interface a bit awkward.
> So, is seems reasonable to provide both from the very beginning.

So basically `git branch` would send the same SHA-1 twice.

-- 
Felipe Contreras

Re: [RTC/PATCH] Add 'update-branch' hook

[Felipe Contreras]

Stephen Leake wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > Ilya Bobyr wrote:
> >> On 4/21/2014 2:17 PM, Felipe Contreras wrote:
> >> > Ilya Bobyr wrote:
> >> >
> >> >> Also, most have names that start with either "pre-" or "post-".
> >> >> It seems reasonable for both "pre-update-branch" and
> >> >> "post-update-branch" to exist.
> >> > I don't see what would be the point in that.
> >> 
> >> Do you see the point in the other hooks doing that?
> >
> > Yes, there a reason for the existance of those hooks. Now tell me why would
> > anybody use post-update-branch instead of pre-update-branch?
> 
> I have a branch which should always be recompiled on update;
> post-update-branch would be a good place for that.

And why would pre-update-branch not serve that purpose?

-- 
Felipe Contreras

Re: [RTC/PATCH] Add 'update-branch' hook

[Ilya Bobyr]

On 4/22/2014 9:31 AM, Felipe Contreras wrote:
> Stephen Leake wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>
>>> Ilya Bobyr wrote:
>>>> On 4/21/2014 2:17 PM, Felipe Contreras wrote:
>>>>> Ilya Bobyr wrote:
>>>>>
>>>>>> Also, most have names that start with either "pre-" or "post-".
>>>>>> It seems reasonable for both "pre-update-branch" and
>>>>>> "post-update-branch" to exist.
>>>>> I don't see what would be the point in that.
>>>> Do you see the point in the other hooks doing that?
>>> Yes, there a reason for the existance of those hooks. Now tell me why would
>>> anybody use post-update-branch instead of pre-update-branch?
>> I have a branch which should always be recompiled on update;
>> post-update-branch would be a good place for that.
> And why would pre-update-branch not serve that purpose?

"pre-" hook could be used, but if the hooks is not supposed to prevent
the operation, it seems reasonable to put it in the "post-" hook should
one be available.
For example, for clone and branch that would mean that that the branch
sections are already created in .git/config, but for "pre-" hooks,
should be find the right spot, configuration could probably be absent
just yet.

I do not think that someone is objecting adding just the "pre-" hook first.
But it seems unlikely that one can envision all the possible use cases
to say that "post-" hook would never be useful.

Re: [RTC/PATCH] Add 'update-branch' hook

[Felipe Contreras]

Ilya Bobyr wrote:
> On 4/22/2014 9:31 AM, Felipe Contreras wrote:
> > Stephen Leake wrote:
> >> Felipe Contreras <felipe.contreras@gmail.com> writes:
> >>> Yes, there a reason for the existance of those hooks. Now tell me why would
> >>> anybody use post-update-branch instead of pre-update-branch?
> >> 
> >> I have a branch which should always be recompiled on update;
> >> post-update-branch would be a good place for that.
> >> 
> > And why would pre-update-branch not serve that purpose?
> 
> "pre-" hook could be used, but if the hooks is not supposed to prevent
> the operation, it seems reasonable to put it in the "post-" hook should
> one be available.

If 'pre-update-branch' can be used, then you are pretty much agreeing to the fact
that the 'post-udpate-branch' hook would be *useless*.

Such a script would work both as 'pre-update-branch' and 'post-update-branch',
therefore a single 'update-branch' would serve.

So I ask again:

Tell me why would anybody need 'post-update-branch' instead of
'pre-update-branch'?

-- 
Felipe Contreras

Re: [RTC/PATCH] Add 'update-branch' hook

[Stephen Leake]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> Stephen Leake wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>> 
>> > Ilya Bobyr wrote:
>> >> On 4/21/2014 2:17 PM, Felipe Contreras wrote:
>> >> > Ilya Bobyr wrote:
>> >> >
>> >> >> Also, most have names that start with either "pre-" or "post-".
>> >> >> It seems reasonable for both "pre-update-branch" and
>> >> >> "post-update-branch" to exist.
>> >> > I don't see what would be the point in that.
>> >> 
>> >> Do you see the point in the other hooks doing that?
>> >
>> > Yes, there a reason for the existance of those hooks. Now tell me why would
>> > anybody use post-update-branch instead of pre-update-branch?
>> 
>> I have a branch which should always be recompiled on update;
>> post-update-branch would be a good place for that.
>
> And why would pre-update-branch not serve that purpose?

Because the code that needs to be compiled is not yet in the workspace

-- 
-- Stephe

Re: [RTC/PATCH] Add 'update-branch' hook

[Felipe Contreras]

Stephen Leake wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > Stephen Leake wrote:
> >> Felipe Contreras <felipe.contreras@gmail.com> writes:
> >> 
> >> > Ilya Bobyr wrote:
> >> >> On 4/21/2014 2:17 PM, Felipe Contreras wrote:
> >> >> > Ilya Bobyr wrote:
> >> >> >
> >> >> >> Also, most have names that start with either "pre-" or "post-".
> >> >> >> It seems reasonable for both "pre-update-branch" and
> >> >> >> "post-update-branch" to exist.
> >> >> > I don't see what would be the point in that.
> >> >> 
> >> >> Do you see the point in the other hooks doing that?
> >> >
> >> > Yes, there a reason for the existance of those hooks. Now tell me why would
> >> > anybody use post-update-branch instead of pre-update-branch?
> >> 
> >> I have a branch which should always be recompiled on update;
> >> post-update-branch would be a good place for that.
> >
> > And why would pre-update-branch not serve that purpose?
> 
> Because the code that needs to be compiled is not yet in the workspace

And it won't be in 'post-update-branch' either.

 % git checkout master
 % git branch feature-a stable
 <- update-branch hook will be called here

The hook will get 'feature-a' as the first argument, but the code in the
workspace would correspond to 'master'; the checked out branch (pre or post).

-- 
Felipe Contreras

Re: [RTC/PATCH] Add 'update-branch' hook

[Felipe Contreras]

Junio C Hamano wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > ... there are _already_ hooks without pre/post.
> 
> Like commit-msg?  Yes, it would have been nicer if it were named
> verify-commit-message or something.

No it wouldn't. I can use the commit-msg hook to change the commit message and
to absolutely no verification, so verify-commit-message would be misleading.

Maybe you would like modify-and-or-verify-commit-message which would be
correct, but I wouldn't, I like short-and-sweet, and commit-msg is just that.

> Old mistakes are harder to change because of inertia.  It is not a
> good excuse to knowingly make a new mistake to add new exceptions
> that the users need to check documentations for, is it?

That's a nifty trick; label something a mistake, and then it suddenly becomes
one.

No, it's not a mistake, first it has to be proven to be mistake and I haven't
seen any arguments that try to do so.

Besides it's a red herring, you said such a name would be original and I've
just proved that it's not original, so the originality is not a concern.

> > And it's not confusing,
> 
> A simple fact that Ilya asked the question tells us otherwise ;-)

It's not any more confusing than these:

applypatch-msg:

When does this happen? Can I return an error?

pre-applypatch:

Again when does it happen? What does the input contains? The whole patch? Including the message?

post-applypatch:

Totally confused.

pre-commit:
prepare-commit-msg:
commit-msg:

What is the difference between these? Doesn't pre-commit contains the message already?

pre-receive:

Before receiving what?

update:

Updating what? When is it called? Can I cancel something?

The fact that somebody asked a question doesn't make a name confusing.

> I personally do not see an immediate need for post-update-branch,
> but if the new hook is about intervening an operation,

It's not about that, I can remove that feature if it would make you happier.

> Otherwise it would be impossible to later add "post-update-branch"

Which is never going to happen.

I'm still waiting for anybody to imagine any reason why we might want
post-udpate-branch.

-- 
Felipe Contreras

Re: [RTC/PATCH] 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:
>> 
>> > ... there are _already_ hooks without pre/post.
>> 
>> Like commit-msg?  Yes, it would have been nicer if it were named
>> verify-commit-message or something.
>
> No it wouldn't. I can use the commit-msg hook to change the commit message and
> to absolutely no verification, so verify-commit-message would be misleading.

You are confused (and please do not spread the confusion).  If you
read the first paragraph of the documentation on the hook and think
for 5 seconds why "--no-verify" countermands it, you would realize
that the hook is primarily meant for verification.  We also allow
the hook to edit the message, but that is not even "a useful feature
added as an afterthought"; the documentation mentions it because the
implementation did not bother to make sure the hook did not touch
the message file.

It was a mistake not to call it with a clear name that tells
verification happens there.

>> Old mistakes are harder to change because of inertia.  It is not a
>> good excuse to knowingly make a new mistake to add new exceptions
>> that the users need to check documentations for, is it?

I see no reason to waste more time on this point.

Re: [RTC/PATCH] Add 'update-branch' hook

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

>> >> I have a branch which should always be recompiled on update;
>> >> post-update-branch would be a good place for that.
>> >
>> > And why would pre-update-branch not serve that purpose?
>> 
>> Because the code that needs to be compiled is not yet in the workspace
>
> And it won't be in 'post-update-branch' either.
>
>  % git checkout master
>  % git branch feature-a stable
>  <- update-branch hook will be called here
>
> The hook will get 'feature-a' as the first argument, but the code in the
> workspace would correspond to 'master'; the checked out branch (pre or post).

The whole point of a pre- hook is to run _before_ the externally
observable state changes due to the operation.

If Stephen has a separate build-tree that fetches from the branch
every time the tip of the branch changes in this repository to
produce build artifacts for the branch to be shared in his network,
perhaps via NFS or something.  "git fetch" that will be run from
that build-tree repository will *not* see the tip of the branch, and
running such a hook will not be possible from a pre-update-branch
hook.

We can certainly argue that such a hook could instead push to the
build-tree repository using the commit object name, but I tend to
think such an argument is merely sidestepping the real issue.  Some
hooks do want to observe the state _after_ the operation [*1*],
while some hooks can do without seeing exactly the state after the
operation.

So while I am generally not very supportive towards post-anything
hook, I would reject a claim that says "pre-anything can be used
without inventing post-anything---do the same thing and allow the
operation and you are done".  That is not simply true.


[Footnote]

*1* A trivial example: send out an e-mail that contains the output
    from "git branch -l -v" or "git log --oneline --decorate --all"
    to a logger and expect to see the branch tip pointing at the
    commit _after_ the operation.

Re: [RTC/PATCH] 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:
> >> 
> >> > ... there are _already_ hooks without pre/post.
> >> 
> >> Like commit-msg?  Yes, it would have been nicer if it were named
> >> verify-commit-message or something.
> >
> > No it wouldn't. I can use the commit-msg hook to change the commit message and
> > to absolutely no verification, so verify-commit-message would be misleading.
> 
> You are confused (and please do not spread the confusion).  If you
> read the first paragraph of the documentation on the hook and think
> for 5 seconds why "--no-verify" countermands it, you would realize
> that the hook is primarily meant for verification.

I do not care what the hook is "primarily for", it's for more than just
verification.

> We also allow the hook to edit the message, but that is not even "a useful
> feature added as an afterthought"; the documentation mentions it because the
> implementation did not bother to make sure the hook did not touch the message
> file.

Indeed it's too late now, and now the hook does more than just verification,
therefore verify-commit-message wouldn't be an appropriate name.

> It was a mistake not to call it with a clear name that tells
> verification happens there.

No, the name is fine for what the hook does, if you would want the script to do
something different, *and* change the name of the script, that's a different
issue.

> >> Old mistakes are harder to change because of inertia.  It is not a
> >> good excuse to knowingly make a new mistake to add new exceptions
> >> that the users need to check documentations for, is it?
> 
> I see no reason to waste more time on this point.

You haven't proved it's a mistake.

The only thing you have showed is that letting the 'commit-msg' modify the
message was a mistake, not that the name is wrong for what it currently does.

-- 
Felipe Contreras

Re: [RTC/PATCH] Add 'update-branch' hook

[Felipe Contreras]

Junio C Hamano wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> >> >> I have a branch which should always be recompiled on update;
> >> >> post-update-branch would be a good place for that.
> >> >
> >> > And why would pre-update-branch not serve that purpose?
> >> 
> >> Because the code that needs to be compiled is not yet in the workspace
> >
> > And it won't be in 'post-update-branch' either.
> >
> >  % git checkout master
> >  % git branch feature-a stable
> >  <- update-branch hook will be called here
> >
> > The hook will get 'feature-a' as the first argument, but the code in the
> > workspace would correspond to 'master'; the checked out branch (pre or post).
> 
> The whole point of a pre- hook is to run _before_ the externally
> observable state changes due to the operation.
> 
> If Stephen has a separate build-tree that fetches from the branch
> every time the tip of the branch changes in this repository to
> produce build artifacts for the branch to be shared in his network,
> perhaps via NFS or something.  "git fetch" that will be run from
> that build-tree repository will *not* see the tip of the branch, and
> running such a hook will not be possible from a pre-update-branch
> hook.
> 
> We can certainly argue that such a hook could instead push to the
> build-tree repository using the commit object name,

Exactly, it could do that.

> but I tend to think such an argument is merely sidestepping the real issue.

So you grant that there is no reason anybody can think of why we would ever
want a post-update-branch?

> Some hooks do want to observe the state _after_ the operation [*1*], while
> some hooks can do without seeing exactly the state after the operation.

Yes, and when the operation is updating a branch, nobody can think of why we
would want the former.

> So while I am generally not very supportive towards post-anything
> hook, I would reject a claim that says "pre-anything can be used
> without inventing post-anything---do the same thing and allow the
> operation and you are done".  That is not simply true.

Let's make a bet, we go for 'pre-update-branch' and five years from now, if
there's no 'post-update-branch', you will publicly accept thta I was right.

Deal?

-- 
Felipe Contreras

Re: [RTC/PATCH] Add 'update-branch' hook

[Stephen Leake]

Felipe Contreras <felipe.contreras@gmail.com> writes:

>> >> I have a branch which should always be recompiled on update;
>> >> post-update-branch would be a good place for that.
>> >
>> > And why would pre-update-branch not serve that purpose?
>> 
>> Because the code that needs to be compiled is not yet in the workspace
>
> And it won't be in 'post-update-branch' either.

Then you are using a very odd definition of "post update"

>  % git checkout master
>  % git branch feature-a stable
>  <- update-branch hook will be called here
>
> The hook will get 'feature-a' as the first argument, but the code in the
> workspace would correspond to 'master'; the checked out branch (pre or post).

Then the hooks should be called 'pre-branch', 'post-branch'; there is no
"update" involved.

The hook I need is actually "post-merge", since "merge" is the command that
updates the workspace.

Sorry for the noise.
-- 
-- Stephe

Re: [RTC/PATCH] Add 'update-branch' hook

[Felipe Contreras]

Stephen Leake wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> >> >> I have a branch which should always be recompiled on update;
> >> >> post-update-branch would be a good place for that.
> >> >
> >> > And why would pre-update-branch not serve that purpose?
> >> 
> >> Because the code that needs to be compiled is not yet in the workspace
> >
> > And it won't be in 'post-update-branch' either.
> 
> Then you are using a very odd definition of "post update"

It's not. The branch was updated, not the workspace.

> >  % git checkout master
> >  % git branch feature-a stable
> >  <- update-branch hook will be called here
> >
> > The hook will get 'feature-a' as the first argument, but the code in the
> > workspace would correspond to 'master'; the checked out branch (pre or post).
> 
> Then the hooks should be called 'pre-branch', 'post-branch'; there is no
> "update" involved.

Of course there is. A 'branch' hook would be triggered when you create a new
branch (e.g. `git branch`), however, it should not be triggered when you update
a branch (e.g. `git rebase`).

> The hook I need is actually "post-merge", since "merge" is the command that
> updates the workspace.

I'd say it's probably 'post-checkout'.

-- 
Felipe Contreras

Re: [RTC/PATCH] Add 'update-branch' hook

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> So you grant that there is no reason anybody can think of why we would ever
> want a post-update-branch?

No, it only shows that you (and I) are not imaginative enough
(and/or we didn't bother spending enough brain cycles) to come up
with an example.  Your lack of imagination and foresight does not
give you any right to close the door to those who come after you who
have real needs, or make it awkward to add it later for them.

> Let's make a bet, we go for 'pre-update-branch' and five years from now, if
> there's no 'post-update-branch', you will publicly accept thta I was right.
>
> Deal?

Let me get this straight.  You spent a lot of effort to argue that
naming it update-branch is the right thing, but now you want me to
name it pre-update-branch, only so that you can prove you are right?

Playing a silly bet among friends may be fun from time to time.  But
I do not like using Git as a plaything, I am not your friend, and I
never felt it fun having to interact with you.  I am not interested
in proving you right or wrong.  You are not that interesting.

What you said however shows clearly the reason why it is not fun to
work with you, and I think that is a lot more important point.  Your
priorities are screwed up.  For the rest of us, making Git better is
the primary reason why we are here.  You seem to be saying that it
is more important to you, however, to "win" your little argument,
and you are willing to even sacrifice a better Git (in your mind,
with the hook named as update-branch) in order to "win".

With a person with such screwed-up priorities, nobody whose first
objective is to make Git better can have a sane conversation.  Ask
those who said they do not want to work with you.  In the list
archive, there are plenty of examples to choose from, and I think
they will agree with me.

It is a pity that in all of these long flamefest, you may have meant
well to improve Git when you brought up something that needs to be
solved in your first few messages.  The rest of us may even have
agreed that it is good to address that issue on many of them.  But
the time "something needs to be done" and "the way Felipe proposes
to solve it is good" turns out to be different, i.e. when those who
agree with the former do not agree with the latter, the discussions
with you go downhill quick.  Each and every time.  See your "index
is hard to learn for people---can we do something?" topic, if you
want another example, where you try to twist words by Peff and
others and caught in doing so.

Now, I know you are going to say "that is what *you* think, and even
if they agree, that is only what *they* think. it is not true! my
priorities are right and they are wrong!".

I'd freely give you that they are only *impressions* we have on you,
that we were forced to form by observing your past and present
behaviours.  It may not be "true you".  You may be a loving an
wonderful person in reality, and you are not showing your true self
when you are on this list.  But you know something?  The project
advances by humans working together, and without telepathy, these
impression are the only thing we humans can go by.

I also know that you are going to say "that is what *you* think".  I
have nothing more to say to you at that point.

It could be that your "bet" is a way for you to finally admitting
that naming the hook with "pre-" prefix will result in a better Git
than without, without you having to say "Yes, you are right, let's
change it" (which I rarely if ever saw you doing).  But still that
shows the same screwed-up priorities---winning your little argument
(or not losing it) matters more to you than working well with
others.  I do not think I want to work with such a person.

Re: [RTC/PATCH] Add 'update-branch' hook

[Felipe Contreras]

Junio C Hamano wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > So you grant that there is no reason anybody can think of why we would ever
> > want a post-update-branch?
> 
> No, it only shows that you (and I) are not imaginative enough
> (and/or we didn't bother spending enough brain cycles) to come up
> with an example.

That is the same thing; nobody can think of a reason.

> Your lack of imagination and foresight does not give you any right to close
> the door to those who come after you who have real needs, or make it awkward
> to add it later for them.

No, but rationality and evidence are the only things we can use to make
decisions, and there is no reason to think something is going to happen, I
don't see why any rational person would think that it would.

> > Let's make a bet, we go for 'pre-update-branch' and five years from now, if
> > there's no 'post-update-branch', you will publicly accept thta I was right.
> >
> > Deal?
> 
> Let me get this straight.  You spent a lot of effort to argue that
> naming it update-branch is the right thing, but now you want me to
> name it pre-update-branch, only so that you can prove you are right?

That is right.

It's impossible to convince you with logic and evidence, and even when you are
shown to be wrong, you don't accept it.

There is literally nothing anybody can do to convince you that imaginary fears
are just imaginary. At the end of the day your conclusion will be that the
improbable is still possible, well anything is possible, so saying "I don't see
how A could happen, but it's possible" is really not saying anything at all.

So yes, the only thing I can do is give up, however if you have any stakes in
progress of Git you would put your money where your mouth is.

If you had already done your job you should have some certainty on whether a
'post-update-branch' is going to happen or not. If you don't have such
certainty enough to make a bet, then why should anybody trust your conclusion?

> For the rest of us, making Git better is the primary reason why we are here.
> You seem to be saying that it is more important to you, however, to "win"
> your little argument, and you are willing to even sacrifice a better Git (in
> your mind, with the hook named as update-branch) in order to "win".

I can't make Git better if you don't humble yourself. You need to accept when
you are wrong.

If I say "A is not going to happen" and I provide evidence, but you say it
would, and indeed it doesn't happen, maybe when I say "B is not going to
happen", maybe you would actually listen, and if not, maybe when I say
"C is not going to happen" you would.

But if you are only willing to accept you were wrong when it's safe, then how
is anything going to change for the future.

> With a person with such screwed-up priorities, nobody whose first
> objective is to make Git better can have a sane conversation.

You are the one with the screwed priorities.

Time and time again the #1 issue people have raised about Git is the
user-interface. We even had Git user surveys to try to find out what people
wanted.

In these surveys the last thing people wanted was better performance, yet most
Git developers are still focusing on performance.

What people said needed improvement was the user-interface and documentation,
yet *nothing* has changed in these two areas. It's not a wonder no more surveys
are launched; because the results of such surveys are ignored anyway.

If a project has screwed-up priorities, it's when the areas of improvements
users say are needed get ignored.

> if you want another example, where you try to twist words by Peff and others
> and caught in doing so.

This is plainly intellectually dishonest. One year ago I made a summary of what
others said[1], I tried to keep it verbatim, CC'ed them, and invited them to
clarify if their position was misrepresented. Nobody, not even Jeff complained
about that.

Now, my mistake was thinking that "A is better" meant "we should go for A",
however, that wasn't the case for Jeff. I didn't twist any words, I made a
wrong assumption.

However, I bet most people in that list agree that "we should go for A", and if
you want, I can ask them all again (except Jeff, because we know his answer).
But I bet you are not interested in what they (or for that matter anyone) think
on the matter.

And you know it was an easy mistake to make, to accuse me of twisting words is
just dishonest.

> It could be that your "bet" is a way for you to finally admitting
> that naming the hook with "pre-" prefix will result in a better Git
> than without, without you having to say "Yes, you are right, let's
> change it" (which I rarely if ever saw you doing).  But still that
> shows the same screwed-up priorities---winning your little argument
> (or not losing it) matters more to you than working well with
> others.  I do not think I want to work with such a person.

That is not it at all.

How am I going to convince you of anything controversial in the future, if you
are never willing to admit that you were wrong, and pay a small public face
price?

The fact of the matter is that you don't want to be wrong, you don't want to be
shown to be wrong, and you don't want to accept you were wrong. Therefore you
reject any experiment in tha direction, and you ignore things like the user
survey that shows your priorities are wrong.

*This* is what hurts the project, not my "bet".

Now, if you had any certainty on what you are saying, you would say "Sure", win
the bet, and nothing bad would happen, users would have "pre-update-branch" and
"post-update-branch", and everybody would be happy (except me a little bit
because I was wrong). But the fact of the matter is that at some level you
already know there won't be any "post-update-branch", I guess that's why you
decided to attack me personally instead of dealing with your cognitive
dissonance and accept that fact.

[1] http://article.gmane.org/gmane.comp.version-control.git/233469

-- 
Felipe Contreras