Re: [PATCH] contrib/hooks: add post-update hook for updating working copy
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 30, 2007, 17:19 UTC
- Message-ID
- <7vejjte4wp.fsf@assigned-by-dhcp.cox.net>
- In-Reply-To
- <11831937823588-git-send-email-sam.vilain@catalyst.net.nz>
Sam Vilain <sam.vilain@catalyst.net.nz> writes:
Show 12 quoted lines
> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt > index 665f6dc..9f5fbc7 100644 > --- a/Documentation/git-push.txt > +++ b/Documentation/git-push.txt > @@ -20,7 +20,9 @@ necessary to complete the given refs. > > You can make interesting things happen to a repository > every time you push into it, by setting up 'hooks' there. See > -documentation for gitlink:git-receive-pack[1]. > +documentation for gitlink:git-receive-pack[1]. One commonly > +requested feature, updating the working copy of the target > +repository, must be enabled in this way.
That is more like "could be", not "must be", and it is not the manpage's job to pass judgement on if a feature is often requested.
Show 24 quoted lines
> diff --git a/templates/hooks--post-update b/templates/hooks--post-update > index bcba893..b5d490c 100644 > --- a/templates/hooks--post-update > +++ b/templates/hooks--post-update > @@ -1,8 +1,78 @@ > #!/bin/sh > # > -# An example hook script to prepare a packed repository for use over > -# dumb transports. > +# This hook does two things: > # > -# To enable this hook, make this file executable by "chmod +x post-update". > +# 1. update the "info" files that allow the list of references to be > +# queries over dumb transports such as http > +# > +# 2. if this repository looks like it is a non-bare repository, and > +# the checked-out branch is pushed to, then update the working copy. > +# This makes "push" and "pull" symmetric operations, as in darcs and > +# bzr. > + > +git-update-server-info > + > +export GIT_DIR=`cd $GIT_DIR; pwd` > +[ `expr "$GIT_DIR" : '.*/\.git'` = 0 ] && exit 0
That's convoluted. If you use 'expr', probably
expr "$GIT_DIR" : '.*/\.git' >/dev/null || exit 0
but I would probably do without an extra fork, like this:
case "$GIT_DIR" in */.git) : happy ;; *) exit 0 ;; esac
Also you can exit early if $GIT_DIR/index does not exist.
> +
> +tree_in_revlog() {revlog? Since when are we Hg?
Show 14 quoted lines
> + ref=$1
> + tree=$2
> + found=$(
> + tail logs/$ref | while read commit rubbish
> + do
> + this_tree=`git-rev-parse commit $commit^{tree}`
> + if [ "$this_tree" = "$tree" ]
> + then
> + echo $commit
> + fi
> + done
> + )
> + [ -n "$found" ] && true
> +}I would imagine that "$some_command && true" would always give the same result as "$some_command" alone. I'd just write this as:
test -n "$found"
if I were you.
> + > +for ref > +do > +active=`git-symbolic-ref HEAD`
- You do not want to do this inside "for ref" loop as this is constant expression.
- When the HEAD is detached, this will give you an error message, and an empty string. But you do not care about detached HEAD case anyway I would imagine.
Perhaps...
active=$(git symbolic-ref -q HEAD) || exit 0
for ref
do
...Show 6 quoted lines
> +if [ "$ref" = "$active" ] > +then > + echo "Pushing to checked out branch - updating working copy" >&2 > + success= > + if ! (cd ..; git-diff-files) | grep -q . > + then
Trying to see if there is any difference from the index, aka
git diff-files --quiet
?
> + # save the current index just in case > + current_tree=`git-write-tree`
What happens if the user is in the middle of a merge? write-tree would fail and you should error out.
> + if tree_in_revlog $ref $current_tree > + then
Why should it behave differently depending on whether the index matches one of the arbitrary (i.e. taken from "tail" default) number of commits the user happened to be at in the recent past? If the check were "does it match with the HEAD", there could be a valid justification but this check does not make any sense to me.
> + cd .. > + if git-diff-index -R --name-status HEAD >&2 && > + git-diff-index -z --name-only --diff-filter=A HEAD | xargs -0r rm && > + git-reset --hard HEAD
I do not understand the first two lines at all. Are you trying to lose working files for the paths that were added to the index since HEAD? "git reset --hard HEAD" should take care of that already. To test:
$ >a-new-file
$ git add a-new-file
$ git reset --hard HEAD
$ ls -l a-new-file
ls: a-new-file: No such file or directoryBut more importantly, why is it justified to throw away such files to begin with?
Show 11 quoted lines
> + then > + success=1 > + else > + echo "E:unexpected error during update" >&2 > + fi > + else > + echo "E:uncommitted, staged changes found" >&2 > + fi > + else > + echo "E:unstaged changes found" >&2 > + fi
I think this part is a good demonstration why pushing into a live branch should not attempt to update the working tree. It sometimes happens, and it sometimes cannot (which is not your fault at all), but the indication of what happened (or did not happen) goes to the person who pushed the changes, not to the person who gets confusing behaviour if the index/worktree suddenly goes out of sync with respect to the updated HEAD.
The longer I look at this patch, the more inclined I become to say that the only part that is worth saving is the next hunk.
Show 19 quoted lines
> -exec git-update-server-info > + if [ -z "$success" ] > + then > + ( > + echo "Non-bare repository checkout is not clean - not updating it" > + echo "However I AM going to update the index. Any half-staged commit" > + echo "in that checkout will be thrown away, but on the bright side" > + echo "this is probably the least confusing thing for us to do and at" > + echo "least we're not throwing any files somebody has changed away" > + git-reset --mixed HEAD > + echo > + echo "This is the new status of the upstream working copy:" > + git-status > + ) >&2 > + fi > +fi > +done > -- > 1.5.2.1.1131.g3b90