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

> 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

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

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

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

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

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

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

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

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

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

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

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

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

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