[PATCH] post-checkout hook, and related docs and tests

[PATCH] post-checkout hook, and related docs and tests

[root]

Signed-off-by: Josh England <jjengla@sandia.gov>
---
 Documentation/hooks.txt       |   10 +++++++
 git-checkout.sh               |    5 +++
 t/t5403-post-checkout-hook.sh |   61 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 76 insertions(+), 0 deletions(-)
 create mode 100755 t/t5403-post-checkout-hook.sh

diff --git a/Documentation/hooks.txt b/Documentation/hooks.txt
index c39edc5..e78f91a 100644
--- a/Documentation/hooks.txt
+++ b/Documentation/hooks.txt
@@ -87,6 +87,16 @@ parameter, and is invoked after a commit is made.
 This hook is meant primarily for notification, and cannot affect
 the outcome of `git-commit`.
 
+post-checkout
+-----------
+
+This hook is invoked when a `git-checkout` is run on a local repository.
+The hook is given two parameters: the ref of the previous HEAD, and the ref of 
+the new HEAD.  This hook cannot affect the outcome of `git-checkout`.
+
+This hook can be used to perform repository validity checks, auto-display
+differences from the previous HEAD, or set working dir metadata properties.
+
 [[pre-receive]]
 pre-receive
 -----------
diff --git a/git-checkout.sh b/git-checkout.sh
index 17f4392..0cff36c 100755
--- a/git-checkout.sh
+++ b/git-checkout.sh
@@ -284,3 +284,8 @@ if [ "$?" -eq 0 ]; then
 else
 	exit 1
 fi
+
+# Run a post-checkout hook
+if test -x "$GIT_DIR"/hooks/post-checkout; then
+        "$GIT_DIR"/hooks/post-checkout $old $new
+fi
diff --git a/t/t5403-post-checkout-hook.sh b/t/t5403-post-checkout-hook.sh
new file mode 100755
index 0000000..aa0216a
--- /dev/null
+++ b/t/t5403-post-checkout-hook.sh
@@ -0,0 +1,61 @@
+#!/bin/sh
+#
+# Copyright (c) 2006 Josh England
+#
+
+test_description='Test the post-checkout hook.'
+. ./test-lib.sh
+
+test_expect_success setup '
+	echo Data for commit0. >a &&
+	git update-index --add a &&
+	tree0=$(git write-tree) &&
+	commit0=$(echo setup | git commit-tree $tree0) &&
+        git update-ref refs/heads/master $commit0 &&
+	git-clone ./. clone1 &&
+	git-clone ./. clone2 &&
+        GIT_DIR=clone2/.git git branch -a new2 &&
+       	echo Data for commit1. >clone2/b &&
+	GIT_DIR=clone2/.git git add clone2/b &&
+	GIT_DIR=clone2/.git git commit -m new2
+'
+
+for clone in 1 2; do 
+    cat >clone${clone}/.git/hooks/post-checkout <<'EOF'
+#!/bin/sh
+echo $@ > $GIT_DIR/post-checkout.args
+EOF
+    chmod u+x clone${clone}/.git/hooks/post-checkout
+done
+
+test_expect_success 'post-checkout runs as expected ' '
+        GIT_DIR=clone1/.git git checkout master &&
+ 	test -e clone1/.git/post-checkout.args
+'
+
+test_expect_success 'post-checkout receives the right arguments with HEAD unchanged ' '
+         old=$(awk "{print \$1}" clone1/.git/post-checkout.args) &&
+         new=$(awk "{print \$2}" clone1/.git/post-checkout.args) &&         
+         test $old = $new
+'
+
+test_expect_success 'post-checkout runs as expected ' '
+        GIT_DIR=clone1/.git git checkout master &&
+ 	test -e clone1/.git/post-checkout.args
+'
+
+test_expect_success 'post-checkout args are correct with git checkout -b ' '
+        GIT_DIR=clone1/.git git checkout -b new1 &&
+        old=$(awk "{print \$1}" clone1/.git/post-checkout.args) &&
+        new=$(awk "{print \$2}" clone1/.git/post-checkout.args) &&         
+        test $old = $new
+'
+
+test_expect_success 'post-checkout receives the right arguments with HEAD changed ' '
+        GIT_DIR=clone2/.git git checkout new2 &&
+        old=$(awk "{print \$1}" clone2/.git/post-checkout.args) &&
+        new=$(awk "{print \$2}" clone2/.git/post-checkout.args) &&         
+        test $old != $new
+'
+
+test_done
-- 
1.5.3.1.143.gf417e3-dirty

Re: [PATCH] post-checkout hook, and related docs and tests

[Josh England]

Junk.  Sorry about the address.  My mailer went retarded.

-JE

On Fri, 2007-09-21 at 14:27 -0600, root wrote:
> Signed-off-by: Josh England <jjengla@sandia.gov>
> ---
>  Documentation/hooks.txt       |   10 +++++++
>  git-checkout.sh               |    5 +++
>  t/t5403-post-checkout-hook.sh |   61 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 76 insertions(+), 0 deletions(-)
>  create mode 100755 t/t5403-post-checkout-hook.sh
> 
> diff --git a/Documentation/hooks.txt b/Documentation/hooks.txt
> index c39edc5..e78f91a 100644
> --- a/Documentation/hooks.txt
> +++ b/Documentation/hooks.txt
> @@ -87,6 +87,16 @@ parameter, and is invoked after a commit is made.
>  This hook is meant primarily for notification, and cannot affect
>  the outcome of `git-commit`.
>  
> +post-checkout
> +-----------
> +
> +This hook is invoked when a `git-checkout` is run on a local repository.
> +The hook is given two parameters: the ref of the previous HEAD, and the ref of 
> +the new HEAD.  This hook cannot affect the outcome of `git-checkout`.
> +
> +This hook can be used to perform repository validity checks, auto-display
> +differences from the previous HEAD, or set working dir metadata properties.
> +
>  [[pre-receive]]
>  pre-receive
>  -----------
> diff --git a/git-checkout.sh b/git-checkout.sh
> index 17f4392..0cff36c 100755
> --- a/git-checkout.sh
> +++ b/git-checkout.sh
> @@ -284,3 +284,8 @@ if [ "$?" -eq 0 ]; then
>  else
>  	exit 1
>  fi
> +
> +# Run a post-checkout hook
> +if test -x "$GIT_DIR"/hooks/post-checkout; then
> +        "$GIT_DIR"/hooks/post-checkout $old $new
> +fi
> diff --git a/t/t5403-post-checkout-hook.sh b/t/t5403-post-checkout-hook.sh
> new file mode 100755
> index 0000000..aa0216a
> --- /dev/null
> +++ b/t/t5403-post-checkout-hook.sh
> @@ -0,0 +1,61 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2006 Josh England
> +#
> +
> +test_description='Test the post-checkout hook.'
> +. ./test-lib.sh
> +
> +test_expect_success setup '
> +	echo Data for commit0. >a &&
> +	git update-index --add a &&
> +	tree0=$(git write-tree) &&
> +	commit0=$(echo setup | git commit-tree $tree0) &&
> +        git update-ref refs/heads/master $commit0 &&
> +	git-clone ./. clone1 &&
> +	git-clone ./. clone2 &&
> +        GIT_DIR=clone2/.git git branch -a new2 &&
> +       	echo Data for commit1. >clone2/b &&
> +	GIT_DIR=clone2/.git git add clone2/b &&
> +	GIT_DIR=clone2/.git git commit -m new2
> +'
> +
> +for clone in 1 2; do 
> +    cat >clone${clone}/.git/hooks/post-checkout <<'EOF'
> +#!/bin/sh
> +echo $@ > $GIT_DIR/post-checkout.args
> +EOF
> +    chmod u+x clone${clone}/.git/hooks/post-checkout
> +done
> +
> +test_expect_success 'post-checkout runs as expected ' '
> +        GIT_DIR=clone1/.git git checkout master &&
> + 	test -e clone1/.git/post-checkout.args
> +'
> +
> +test_expect_success 'post-checkout receives the right arguments with HEAD unchanged ' '
> +         old=$(awk "{print \$1}" clone1/.git/post-checkout.args) &&
> +         new=$(awk "{print \$2}" clone1/.git/post-checkout.args) &&         
> +         test $old = $new
> +'
> +
> +test_expect_success 'post-checkout runs as expected ' '
> +        GIT_DIR=clone1/.git git checkout master &&
> + 	test -e clone1/.git/post-checkout.args
> +'
> +
> +test_expect_success 'post-checkout args are correct with git checkout -b ' '
> +        GIT_DIR=clone1/.git git checkout -b new1 &&
> +        old=$(awk "{print \$1}" clone1/.git/post-checkout.args) &&
> +        new=$(awk "{print \$2}" clone1/.git/post-checkout.args) &&         
> +        test $old = $new
> +'
> +
> +test_expect_success 'post-checkout receives the right arguments with HEAD changed ' '
> +        GIT_DIR=clone2/.git git checkout new2 &&
> +        old=$(awk "{print \$1}" clone2/.git/post-checkout.args) &&
> +        new=$(awk "{print \$2}" clone2/.git/post-checkout.args) &&         
> +        test $old != $new
> +'
> +
> +test_done

Re: [PATCH] post-checkout hook, and related docs and tests

[Junio C Hamano]

"root" <root@sandia.gov> writes:

> +post-checkout
> +-----------
> +
> +This hook is invoked when a `git-checkout` is run on a local repository.
> +The hook is given two parameters: the ref of the previous HEAD, and the ref of 
> +the new HEAD.  This hook cannot affect the outcome of `git-checkout`.
> +
> +This hook can be used to perform repository validity checks, auto-display
> +differences from the previous HEAD, or set working dir metadata properties.
> +

People may wonder why this is not run when they do "git checkout
otherbranch path.c"; the second sentence from the above
description implies why it shouldn't, but the first sentence
probably should state it more clearly.

What's the _semantics_ you are trying to achieve?

Why does the hook run every time git-bisect suggests the next
revision to try?

Why does the hook run when rebase starts its work?

When "git pull" or "git merge" results in a fast forward, the
situation is no different from checking out a new revision.  Why
doesn't the hook run in these cases?

Re: [PATCH] post-checkout hook, and related docs and tests

[Josh England]

On Fri, 2007-09-21 at 17:15 -0700, Junio C Hamano wrote:
> "root" <root@sandia.gov> writes:
> 
> > +post-checkout
> > +-----------
> > +
> > +This hook is invoked when a `git-checkout` is run on a local repository.
> > +The hook is given two parameters: the ref of the previous HEAD, and the ref of 
> > +the new HEAD.  This hook cannot affect the outcome of `git-checkout`.
> > +
> > +This hook can be used to perform repository validity checks, auto-display
> > +differences from the previous HEAD, or set working dir metadata properties.
> > +
> 
> People may wonder why this is not run when they do "git checkout
> otherbranch path.c"; the second sentence from the above
> description implies why it shouldn't, but the first sentence
> probably should state it more clearly.
> 
> What's the _semantics_ you are trying to achieve?

I'd like to get a hook that runs whenever the working dir gets
updated.  The 'git-checkout otherbranch path.c' case should run it also, so I view that as a bug.

> Why does the hook run every time git-bisect suggests the next
> revision to try?
> Why does the hook run when rebase starts its work?

It may be inserted in a ad place or maybe it needs some intelligence in
there to know when *not* to run.

> When "git pull" or "git merge" results in a fast forward, the
> situation is no different from checking out a new revision.  Why
> doesn't the hook run in these cases?

This is actually what I'd like to do.  I submitted the post-merge patch
some time ago to serve that purpose.  Do you think they should both be
rolled into a single post-checkout hook?  It would seem to make sense to
me.

Re: [PATCH] post-checkout hook, and related docs and tests

[Josh England]

On Fri, 2007-09-21 at 17:15 -0700, Junio C Hamano wrote:
> "root" <root@sandia.gov> writes:
> 
> > +post-checkout
> > +-----------
> > +
> > +This hook is invoked when a `git-checkout` is run on a local repository.
> > +The hook is given two parameters: the ref of the previous HEAD, and the ref of 
> > +the new HEAD.  This hook cannot affect the outcome of `git-checkout`.
> > +
> > +This hook can be used to perform repository validity checks, auto-display
> > +differences from the previous HEAD, or set working dir metadata properties.
> > +
> 
> People may wonder why this is not run when they do "git checkout
> otherbranch path.c"; the second sentence from the above
> description implies why it shouldn't, but the first sentence
> probably should state it more clearly.
> 
> What's the _semantics_ you are trying to achieve?
> 
> Why does the hook run every time git-bisect suggests the next
> revision to try?

Its being run since git-bisect calls git-checkout internally, but since
the 'git-checkout $branch' could potentially update the working tree it
may be desirable to have the hook run.  Since one stated purpose of the
hook is maintain repository validity or update metadata, running the
hook at this time may be the right thing to do.

> Why does the hook run when rebase starts its work?

I think this case is actually desirable.  If the rebase changes some
aspects of the working dir that the hook cares about (eg: metadata),
then the hook will be able handle the situation correctly.  Not running
the hook for a rebase operation could result in the working dir being
left in an inconsistent state.

-JE

Re: [PATCH] post-checkout hook, and related docs and tests

[Junio C Hamano]

"Josh England" <jjengla@sandia.gov> writes:

>> What's the _semantics_ you are trying to achieve?
>
> I'd like to get a hook that runs whenever the working dir gets
> updated.  The 'git-checkout otherbranch path.c' case should
> run it also, so I view that as a bug.

I think that _is_ INSANE.  Do you run the hook for these then?

	$ edit path.c
        $ git-cat-file otherbranch:path.c >path.c

Why "git checkout otherbranch path.c" should be any different?

Re: [PATCH] post-checkout hook, and related docs and tests

[Josh England]

On Mon, 2007-09-24 at 11:34 -0700, Junio C Hamano wrote:
> "Josh England" <jjengla@sandia.gov> writes:
> 
> >> What's the _semantics_ you are trying to achieve?
> >
> > I'd like to get a hook that runs whenever the working dir gets
> > updated.  The 'git-checkout otherbranch path.c' case should
> > run it also, so I view that as a bug.
> 
> I think that _is_ INSANE.  Do you run the hook for these then?
> 
> 	$ edit path.c
>         $ git-cat-file otherbranch:path.c >path.c
> 
> Why "git checkout otherbranch path.c" should be any different?

It is different because the file is being updated through the 'git
checkout' interface.  The user is not copying the file over by hand,
he/she is asking git to do it for them via 'git checkout'.  Granted, the
branch (and HEAD) does not change for this operation, but that shouldn't
matter.  It is somewhat in line with the principle of 'least-surprise':
if the hook runs for 'git checkout otherbranch', but not 'git checkout
otherbranch path.c', this could cause confusion and distress to the
user.  IMO, it is a 'checkout' so the post-checkout hook should run.
Why is that so insane?  

Look at it from the perspective of the intended use of this hook.  I'm
trying to use this hook to keep working dir metadata (ownership/perms)
in a consistent state.  When I do a 'git checkout otherbranch', the hook
runs, updating perms as needed, and all is well.  As is, if I 'git
checkout otherbranch path.c', the file is created with the default
umask, the hook is not run, and path.c potentially (likely) has
incorrect perms.  The working dir is now in an inconsistent state and
the worst part is that the next commit will propagate the faulty
metadata for that file.

-JE

Re: [PATCH] post-checkout hook, and related docs and tests

[Junio C Hamano]

"Josh England" <jjengla@sandia.gov> writes:

> ...  Granted, the
> branch (and HEAD) does not change for this operation, but that shouldn't
> matter.  It is somewhat in line with the principle of 'least-surprise':
> if the hook runs for 'git checkout otherbranch', but not 'git checkout
> otherbranch path.c', this could cause confusion and distress to the
> user.  IMO, it is a 'checkout' so the post-checkout hook should run.
> Why is that so insane?  

Because I find it would be surprising if the following commands
behave differently:

	$ git cat-file blob otherbranch:path.c >path.c
        $ git show otherbranch:path.c >path.c
        $ git diff -R otherbranch path.c | git apply
        $ git checkout otherbranch path.c

These are all talking about various ways to _edit_ working tree
files, and not about switching between revisions.

That's why I said I found that what the second sentence from
your original description implied ("the hook gets old and new
commit object name" which means we are talking about switching
between revisions) was sensible, but it needs to be stressed a
bit.

If you want to spacial case 

        $ git checkout otherbranch path.c

it raises another issue.  Which commit should supply the
"extended attribute description" for path.c?  Should it be taken
from the current commit (aka HEAD), otherbranch, or the index?

Re: [PATCH] post-checkout hook, and related docs and tests

[Josh England]

On Mon, 2007-09-24 at 14:07 -0700, Junio C Hamano wrote:
> "Josh England" <jjengla@sandia.gov> writes:
> 
> > ...  Granted, the
> > branch (and HEAD) does not change for this operation, but that shouldn't
> > matter.  It is somewhat in line with the principle of 'least-surprise':
> > if the hook runs for 'git checkout otherbranch', but not 'git checkout
> > otherbranch path.c', this could cause confusion and distress to the
> > user.  IMO, it is a 'checkout' so the post-checkout hook should run.
> > Why is that so insane?  
> 
> Because I find it would be surprising if the following commands
> behave differently:
> 
> 	$ git cat-file blob otherbranch:path.c >path.c
>         $ git show otherbranch:path.c >path.c
>         $ git diff -R otherbranch path.c | git apply
>         $ git checkout otherbranch path.c

For all intents and purposes, these would still behave the same.  The
existence of a post-checkout hook does not at all affect the outcome of
the checkout.  Sure there is potential for someone to do something
stupid inside the hook script, but that is true of any hook.

Most git users would never even enable the hook, but for those that do I
would assume that they'd want the hook to run for all 'git-checkout'
variants -- as I do.

> These are all talking about various ways to _edit_ working tree
> files, and not about switching between revisions.
> 
> That's why I said I found that what the second sentence from
> your original description implied ("the hook gets old and new
> commit object name" which means we are talking about switching
> between revisions) was sensible, but it needs to be stressed a
> bit.
> 
> If you want to spacial case 
> 
>         $ git checkout otherbranch path.c
> 
> it raises another issue.  Which commit should supply the
> "extended attribute description" for path.c?  Should it be taken
> from the current commit (aka HEAD), otherbranch, or the index?

This already is a special case and your question is valid but not one
that git should necessary care about.  Since extended attributes are not
built into git the only way to handle them is through hooks.  A such, it
is up to the hook to worry about these kinds of issues.  The hook I have
written handles this by updating path.c attributes to match whatever
exists in the (tracked) attributes file of the current HEAD.

This 'git-checkout otherbranch path.c' is a corner case, but one that
can result in broken behavior when handling metadata unless the hook
runs.  I just want to close all the holes.  I can change the description
of the hook to try to dispel any confusion if that would help.

-JE

Re: [PATCH] post-checkout hook, and related docs and tests

[Junio C Hamano]

"Josh England" <jjengla@sandia.gov> writes:

> On Mon, 2007-09-24 at 14:07 -0700, Junio C Hamano wrote:
> ...
>> If you want to spacial case 
>> 
>>         $ git checkout otherbranch path.c
>> 
>> it raises another issue.  Which commit should supply the
>> "extended attribute description" for path.c?  Should it be taken
>> from the current commit (aka HEAD), otherbranch, or the index?
>
> This already is a special case and your question is valid but not one
> that git should necessary care about.  Since extended attributes are not
> built into git the only way to handle them is through hooks.  A such, it
> is up to the hook to worry about these kinds of issues.

The fear I have is that that kind of thinking would necessitate
your hook to be called after the user edits paths.c in any other
way not to confuse users.

What I am questioning is where we should stop, in order to keep
things simpler to explain, and I happen to think that it is far
easier if we can teach that "git checkout other path.c" is
equivalent to "git cat-file blob other:path.c >path.c" followed
by "git add path.c", than saying "checkout is magical and if you
have external hook it can do far more than editing the file
yourself to arrive at the same contents".

But I am obviously not the one who is interested in tracking
extended attributes attached to git contents, and I do not feel
too strongly about one way or the other.  I am Ok with it if you
think "checkout is magical" is easier to teach [*1*].

I just wanted to make sure we know what semantics this is
bringing in, and get it clearly documented.  That's all.


[Footnote]

*1* I actually suspect this might be the case. I consider that
per-path checkout from a commit is just a fancy and handy way to
edit individual files but that probably comes from knowing how
git works too much and I lost my git virginity too long ago.  A
pure "user" who types "git checkout commit path" may actively
expect "checkout" command to do something more magical than
simply updating the index and the work tree files to a random
state that happens to match the state recorded in one commit.

Re: [PATCH] post-checkout hook, and related docs and tests

[Andreas Ericsson]

Junio C Hamano wrote:
> "Josh England" <jjengla@sandia.gov> writes:
> 
> But I am obviously not the one who is interested in tracking
> extended attributes attached to git contents, and I do not feel
> too strongly about one way or the other.  I am Ok with it if you
> think "checkout is magical" is easier to teach [*1*].
> 
> [Footnote]
> 
> *1* I actually suspect this might be the case.

I think so too, if nothing else than for the simple reason than that
the hook is called 'post-checkout', so the explanation is likely to
go something like 'the checkout program activates it after having
updated the worktree'.


> I consider that
> per-path checkout from a commit is just a fancy and handy way to
> edit individual files but that probably comes from knowing how
> git works too much and I lost my git virginity too long ago.  A
> pure "user" who types "git checkout commit path" may actively
> expect "checkout" command to do something more magical than
> simply updating the index and the work tree files to a random
> state that happens to match the state recorded in one commit.

Like, run the post-checkout hook? I should think it wouldn't be
too hard to believe it will.

I imagine the people using this feature will be either git-fanatics
that use it for everything and only in their own environment, or
sysadmins that get a handy tool for managing config in a corporate
environment. I wouldn't be surprised if those sysadmins weren't
all too keen on learning the 1001 ways there is to create a file
from a special revision in git (and personally I only knew about 2
of the 4 you listed), so for them there'll most likely *only* be
git-checkout to edit the work-tree.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Re: [PATCH] post-checkout hook, and related docs and tests

[Josh England]

On Mon, 2007-09-24 at 16:54 -0700, Junio C Hamano wrote:
> "Josh England" <jjengla@sandia.gov> writes:
> 
> > On Mon, 2007-09-24 at 14:07 -0700, Junio C Hamano wrote:
> > ...
> >> If you want to spacial case 
> >> 
> >>         $ git checkout otherbranch path.c
> >> 
> >> it raises another issue.  Which commit should supply the
> >> "extended attribute description" for path.c?  Should it be taken
> >> from the current commit (aka HEAD), otherbranch, or the index?
> >
> > This already is a special case and your question is valid but not one
> > that git should necessary care about.  Since extended attributes are not
> > built into git the only way to handle them is through hooks.  A such, it
> > is up to the hook to worry about these kinds of issues.
> 
> The fear I have is that that kind of thinking would necessitate
> your hook to be called after the user edits paths.c in any other
> way not to confuse users.
> 
> What I am questioning is where we should stop, in order to keep
> things simpler to explain, and I happen to think that it is far
> easier if we can teach that "git checkout other path.c" is
> equivalent to "git cat-file blob other:path.c >path.c" followed
> by "git add path.c", than saying "checkout is magical and if you
> have external hook it can do far more than editing the file
> yourself to arrive at the same contents".
> 
> But I am obviously not the one who is interested in tracking
> extended attributes attached to git contents, and I do not feel
> too strongly about one way or the other.  I am Ok with it if you
> think "checkout is magical" is easier to teach [*1*].
> 
> I just wanted to make sure we know what semantics this is
> bringing in, and get it clearly documented.  That's all.

OK.  I'll try to come up with some good wording for the documentation.

So this leads to my next question:  Should the post-merge patch be
brought in under this same umbrella to form a single post-checkout hook,
or should it stay a separate hook?

-JE

Re: [PATCH] post-checkout hook, and related docs and tests

[Junio C Hamano]

"Josh England" <jjengla@sandia.gov> writes:

> So this leads to my next question:  Should the post-merge patch be
> brought in under this same umbrella to form a single post-checkout hook,
> or should it stay a separate hook?

I think it is called would be inconvenient for the callee if you
call the same hook without telling the hook script why it is
called, so if you go in the unification route the caller of the
unified hook needs to supply an extra parameter and existing
hooks if any need to be updated --- neither sounds like a very
idea.  The writer of the hooks however can choose to call one
from the other if he wants the same action for both hooks, so it
looks to me that separate hooks for separate purposes is the way
to go.

Re: [PATCH] post-checkout hook, and related docs and tests

[Josh England]

On Tue, 2007-09-25 at 14:29 -0700, Junio C Hamano wrote:
> "Josh England" <jjengla@sandia.gov> writes:
> 
> > So this leads to my next question:  Should the post-merge patch be
> > brought in under this same umbrella to form a single post-checkout hook,
> > or should it stay a separate hook?
> 
> I think it is called would be inconvenient for the callee if you
> call the same hook without telling the hook script why it is
> called, so if you go in the unification route the caller of the
> unified hook needs to supply an extra parameter and existing
> hooks if any need to be updated --- neither sounds like a very
> idea.  The writer of the hooks however can choose to call one
> from the other if he wants the same action for both hooks, so it
> looks to me that separate hooks for separate purposes is the way
> to go.

Yeah, I agree.  I'll rework post-checkout and send in both again.

-JE

Re: [PATCH] post-checkout hook, and related docs and tests

[Dmitry Potapov]

On Mon, Sep 24, 2007 at 02:07:36PM -0700, Junio C Hamano wrote:
> "Josh England" <jjengla@sandia.gov> writes:
> 
> > ...  Granted, the
> > branch (and HEAD) does not change for this operation, but that shouldn't
> > matter.  It is somewhat in line with the principle of 'least-surprise':
> > if the hook runs for 'git checkout otherbranch', but not 'git checkout
> > otherbranch path.c', this could cause confusion and distress to the
> > user.  IMO, it is a 'checkout' so the post-checkout hook should run.
> > Why is that so insane?  
> 
> Because I find it would be surprising if the following commands
> behave differently:
> 
> 	$ git cat-file blob otherbranch:path.c >path.c
>         $ git show otherbranch:path.c >path.c
>         $ git diff -R otherbranch path.c | git apply
>         $ git checkout otherbranch path.c

Actually, they already act differently even without any hook.
If path.c is a symbol link then 1 and 2 will give a different
result than commands 3 and 4.

On the other hand, while the difference in above commands
understandable (in case 1 and 2, the shell creates path.c; and
in 3 and 4, git creates it), I really dislike the idea of 
"checkout is magical." I believe that command 3 and 4 should
always give the same result or Git is broken.

Another reason, why I dislike the post-checkout hook is that it
is prone to abuse like as not so smart user trying to put some
content modification here. Moreover, it appears to be excessive
to me, because if you want to run something after git-checkout,
you can write a simple shell script for that that first runs
git-checkout with the given arguments and then run whatever you
want. I don't see why we should modify Git for that.

Perhaps, it would be better to have a hook on modification,
which is invoked every time when Git wants to try to change
anything in the working directory. The hook could receives on
the input something that looks like 'git-diff --name-status'
output and can do any work on creation files, etc. It is much
more flexible, because you can do additional stuff here like
creating one directory in the path as a symbol link somewhere
else or something like that. But what is much more important
is that everything work _consistently_ and you get the same
results whether you type:
git diff -R otherbranch path.c | git apply
or
git checkout otherbranch path.c

If you start with one "magical interface" then eventually you
will end up with everything being so magical that no one can
make sense of it. Please, stay consistent.

Dmitry Potapov

Re: [PATCH] post-checkout hook, and related docs and tests

[Josh England]

On Wed, 2007-09-26 at 18:52 +0400, Dmitry Potapov wrote:
> On Mon, Sep 24, 2007 at 02:07:36PM -0700, Junio C Hamano wrote:
> > "Josh England" <jjengla@sandia.gov> writes:
> > 
> > > ...  Granted, the
> > > branch (and HEAD) does not change for this operation, but that shouldn't
> > > matter.  It is somewhat in line with the principle of 'least-surprise':
> > > if the hook runs for 'git checkout otherbranch', but not 'git checkout
> > > otherbranch path.c', this could cause confusion and distress to the
> > > user.  IMO, it is a 'checkout' so the post-checkout hook should run.
> > > Why is that so insane?  
> > 
> > Because I find it would be surprising if the following commands
> > behave differently:
> > 
> > 	$ git cat-file blob otherbranch:path.c >path.c
> >         $ git show otherbranch:path.c >path.c
> >         $ git diff -R otherbranch path.c | git apply
> >         $ git checkout otherbranch path.c
> 
> Actually, they already act differently even without any hook.
> If path.c is a symbol link then 1 and 2 will give a different
> result than commands 3 and 4.

Moroever, with respect to permissions, the first 2 retain the
permissions of the file if it already exists in the worktree, whereas
the other variations actually recreate the file with a default umask and
wipe out existing permissions.

> On the other hand, while the difference in above commands
> understandable (in case 1 and 2, the shell creates path.c; and
> in 3 and 4, git creates it), I really dislike the idea of 
> "checkout is magical." I believe that command 3 and 4 should
> always give the same result or Git is broken.
> 
> Another reason, why I dislike the post-checkout hook is that it
> is prone to abuse like as not so smart user trying to put some
> content modification here.

Content modification is not among the intended uses for this hook.  Any
and all hooks can be abused/misused in this way.  I just want to give
the user a tool -- if he wants to hit himself in the face with it that's
his prerogative.

>  Moreover, it appears to be excessive
> to me, because if you want to run something after git-checkout,
> you can write a simple shell script for that that first runs
> git-checkout with the given arguments and then run whatever you
> want. I don't see why we should modify Git for that.

The same could be said for pre-commit and other hooks.  The whole
reason for the hook system is to provide a useful interface so that
users are *not* required to write their own wrapper scripts to get the
job done.  In this case, providing the hook is by far the *more*
consistent way of doing things.

> Perhaps, it would be better to have a hook on modification,
> which is invoked every time when Git wants to try to change
> anything in the working directory. The hook could receives on
> the input something that looks like 'git-diff --name-status'
> output and can do any work on creation files, etc. It is much
> more flexible, because you can do additional stuff here like
> creating one directory in the path as a symbol link somewhere
> else or something like that. But what is much more important
> is that everything work _consistently_ and you get the same
> results whether you type:
> git diff -R otherbranch path.c | git apply
> or
> git checkout otherbranch path.c
> 
> If you start with one "magical interface" then eventually you
> will end up with everything being so magical that no one can
> make sense of it. Please, stay consistent.

I don't know why you think this is so magical.  git-checkout can run a
post-checkout hook, if enabled.  Plain and simple.  No magic here.  As
for the universal 'worktree-updated' hook, I look forward to seeing a
sane implementation, but in the meantime post-merge and post-checkout
suit my needs just fine.

-JE