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

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 [off-list ref] wrote:

quoted

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.

quoted

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

quoted

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