Re: [PATCH] contrib/hooks: add post-update hook for updating working copy

[Sam Vilain <sam@vilain.net>]

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

Ok, I'll just remove that clause.  Or do you think it perhaps belongs in
a NOTES or HINTS section?

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

Ok, well first we check that the index matches the working copy.  But if
there are staged changes which have not been committed, then the written
tree will (probably) not exist anywhere in the reflog for the current
branch, and we can stop.

Basically I'm trying to figure out "does the current index have any
uncommitted changes".  If it matches the tree from the previous (handful
of) ref(s), then the answer is "no".  If we can't find it anywhere then
it's probably got staged changes, and short of trying to move the
changes forward, we should stop.

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

The first one simply displays what is happening to the working copy for
the benefit of the user.

The second is implementing something I for some reason thought git-reset
wasn't doing.

> But more importantly, why is it justified to throw away such
> files to begin with?

Because we've already previously decided that they are safely stowed in
a previous (via time/reflog) revision of the current branch.

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

One counter-argument to this is that you indicate that is the behaviour
that you want when you chmod +x the hook.  It should gracefully step out
of the way when people who currently set that hook active keep doing it.

But this problem exists without this hook, in fact it is far worse.  The
indication of what happened goes nowhere, and the person gets extremely
confusing behaviour when they commit.

Perhaps it would make sense to do this check in the "update" hook as
well, thereby chmod +x refuses to allow a push that touches the
currently checked out branch.  The check would then be run twice if both
hooks are enabled, unless the first one can signal success/verification
to the second somehow.

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

I disagree; I think any half-measure is going to leave new users
horribly surprised by what happens, and if you just reset the index then
the staged commit is lost.

Sam.