Re: [PATCH/RFC 0/2] Teach receive-pack not to run update hook for corrupt/non existent ref

[Pang Yan Han <pangyanhan@gmail.com>]

On Sun, Sep 25, 2011 at 01:28:31PM +0530, Sitaram Chamarty wrote:
> On Sun, Sep 25, 2011 at 10:36 AM, Pang Yan Han <pangyanhan@gmail.com> wrote:
> > Hi list,
> >
> > Currently, receive-pack runs the pre-receive, update, post-receive and
> > post-update hooks during a push to delete corrupt or non-existent refs, eg:
> >
> >        git push origin :refs/heads/foo
> >
> > where refs/heads/foo is missing from the remote origin.
> >
> > The issue is reported here [1]
> 
> I did not report an issue.  I asked if this was expected and could be
> relied upon.  I'm actually happy with the current behaviour because it
> solves a problem very neatly for me, but before documenting it I
> wanted to make sure it would not one day disappear.
> 
> > This is a patch series which teaches receive-pack not to run update hook for
> > corrupt or non existent refs during a push.
> >
> > Patch 1/2 isn't really relevant to the topic. It's just something I stumbled
> > across while reading the code. It removes a redundant assignment in the is_url
> > function.
> >
> > Patch 2/2 teaches receive-pack not to run update hook for corrupt or non
> > existent refs. In summary, I reordered the statements in the update function
> > so that the update hook is not run for corrupt / non existent refs.
> >
> > Perhaps this isn't a good enough solution since the pre-receive, post-receive
> > and post-update hooks are still run. Also the tests aren't exactly good looking.
> 
> It doesn't make sense to disable only the update hook.  And although I
> did not come right out and say it, it is the post-update that I care
> about.  If that still runs, my "issue" still exists.

Um I'm rather new to Git and the reason why I didn't reply this initially was
because I didn't know what to reply. Sorry but you sound rather aggressive and
I was really taken aback by this.

I've taken a look at the code again and here's another approach which will
result in heavier changes to builtin/receive-pack and may possibly work:

Check through the list of refs to be updated before even executing the
pre receive hook. Ensure that there is at least one "valid" ref update.
Essentially this is kind of like "dry running" the update function.

One way to do it is to shift the bulk of work determining valid and invalid
ref updates from the update function to a separate function.
We maintain 2 lists, one to store valid refs to be updated and another to
store non-existent/corrupt refs which will be deleted. Specifically, we will
be storing 'struct command'.

The update function can be cut down to these parts:
- determining namespaced_name
- lock_any_ref_for_update
- write_ref_sha1

The pre receive hook will only be executed if the list containing valid ref
pushes is non-empty. Same goes for the post receive and post update hooks.

What do you think of this approach (if it's even correct)?

> 
> > Any advice is highly appreciated. Thanks!
> >
> > [1] http://thread.gmane.org/gmane.comp.version-control.git/181942
> >
> > Pang Yan Han (2):
> >  is_url: Remove redundant assignment
> >  receive-pack: Don't run update hook for corrupt or nonexistent ref
> >
> >  builtin/receive-pack.c |   50 +++++++++++++++++++++++++++--------------------
> >  t/t5516-fetch-push.sh  |   33 +++++++++++++++++++++++++++++++
> >  url.c                  |    1 -
> >  3 files changed, 62 insertions(+), 22 deletions(-)
> >
> > --
> > 1.7.7.rc3.2.g29f2e6
> >
> >
> 
> 
> 
> -- 
> Sitaram