RFC patch, 2 partsTeach receive-pack not to run update hook for corrupt/non existent ref

> Signed-off-by: Pang Yan Han <pangyanhan@gmail.com>
> ---
>  url.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
>
> diff --git a/url.c b/url.c
> index 3e06fd3..d2e17e6 100644
> --- a/url.c
> +++ b/url.c
> @@ -22,7 +22,6 @@ int is_url(const char *url)
>
>        if (!url)
>                return 0;
> -       url2 = url;
>        first_slash = strchr(url, '/');
>
>        /* Input with no slash at all or slash first can't be URL. */

> On Sun, Sep 25, 2011 at 1:06 PM, Pang Yan Han <pangyanhan@gmail.com> wrote:
>> Signed-off-by: Pang Yan Han <pangyanhan@gmail.com>
>> ---
>>  url.c |    1 -
>>  1 files changed, 0 insertions(+), 1 deletions(-)
>>
>> diff --git a/url.c b/url.c
>> index 3e06fd3..d2e17e6 100644
>> --- a/url.c
>> +++ b/url.c
>> @@ -22,7 +22,6 @@ int is_url(const char *url)
>>
>>        if (!url)
>>                return 0;
>> -       url2 = url;
>>        first_slash = strchr(url, '/');
>>
>>        /* Input with no slash at all or slash first can't be URL. */
>
> Looks correct. Perhaps you could mention in the patch message that
>
>   There are no operations on url2 until another assignment to it later
> at line 41.

> Tay Ray Chuan <rctay89@gmail.com> writes:
> 
> > On Sun, Sep 25, 2011 at 1:06 PM, Pang Yan Han <pangyanhan@gmail.com> wrote:
> >> Signed-off-by: Pang Yan Han <pangyanhan@gmail.com>
> >> ---
> >>  url.c |    1 -
> >>  1 files changed, 0 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/url.c b/url.c
> >> index 3e06fd3..d2e17e6 100644
> >> --- a/url.c
> >> +++ b/url.c
> >> @@ -22,7 +22,6 @@ int is_url(const char *url)
> >>
> >>        if (!url)
> >>                return 0;
> >> -       url2 = url;
> >>        first_slash = strchr(url, '/');
> >>
> >>        /* Input with no slash at all or slash first can't be URL. */
> >
> > Looks correct. Perhaps you could mention in the patch message that
> >
> >   There are no operations on url2 until another assignment to it later
> > at line 41.
> 
> The looks correct, so I'll queue it, but it looks like that the function
> is implemented in an overly complicated way.
> 
> Why aren't we checking from left to right in a single pass, perhaps like
> this?
> 
> 	/* Make sure it is of form "scheme://something" */
> 	int is_url(const char *url)
> 	{
> 		/* Is "scheme" part reasonable? */
> 		if (!url || !is_urlschemechar(1, *url++))
> 	        	return 0;
> 		while (*url && *url != ':') {
> 			if (!is_urlschemechar(0, *url++))
> 				return 0;
> 		}
> 		/* We've seen "scheme"; we want colon-slash-slash */
> 		return (url[0] == ':' && url[1] == '/' && url[2] == '/');
> 	}

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

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

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

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

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

> From a philosophical point of view, update and pre-receive *check*
> things to make sure everything is OK.  IMO they should be allowed to
> run even if the ref being deleted doesn't exist -- that could well be
> an error condition that the guy who owns the repo wants to trap and
> alert himself to in some special way.  I would *not* like them
> disabled.

> Sitaram Chamarty <sitaramc@gmail.com> writes:
>
>> From a philosophical point of view, update and pre-receive *check*
>> things to make sure everything is OK.  IMO they should be allowed to
>> run even if the ref being deleted doesn't exist -- that could well be
>> an error condition that the guy who owns the repo wants to trap and
>> alert himself to in some special way.  I would *not* like them
>> disabled.
>
> I think this is a sane thing to do.
>
>> Post-{update,receive} are for *after* a successful push.  My
>> suggestion would be to make sure the inputs supplied to those hooks
>> (via STDIN for post-receive, and as arguments in case of post-update)
>> reflect this -- only successfully updated refs are sent in as args.
>
> Perhaps sane.
>
>> This might mean that in the case of 'git push origin
>> :refs/heads/non-existent-ref' the post-receive hook would run but
>> STDIN would be empty, and post-update would run but have no arguments.
>
> Hmm?
>
> In that case (if "non-existent-ref" was indeed non-existent, and not just
> pointing at a dangling commit), I would say the post anything hook should
> not be called for that ref.  These hooks of course need to run if there
> are _other_ refs that were updated, though, to handle these _other_ refs,
> but I do not think they should be told about the no-op.

>> In that case (if "non-existent-ref" was indeed non-existent, and not just
>> pointing at a dangling commit), I would say the post anything hook should
>> not be called for that ref. These hooks of course need to run if there
>> are _other_ refs that were updated, though, to handle these _other_ refs,
>> but I do not think they should be told about the no-op.
>
> Question is what happens if none of them existed.  It's a difference
> between not calling the hook at all, versus calling it with no
> arguments/empty stdin (as the case may be) -- which would you do?

> Sitaram Chamarty <sitaramc@gmail.com> writes:
>
>>> In that case (if "non-existent-ref" was indeed non-existent, and not just
>>> pointing at a dangling commit), I would say the post anything hook should
>>> not be called for that ref. These hooks of course need to run if there
>>> are _other_ refs that were updated, though, to handle these _other_ refs,
>>> but I do not think they should be told about the no-op.
>>
>> Question is what happens if none of them existed.  It's a difference
>> between not calling the hook at all, versus calling it with no
>> arguments/empty stdin (as the case may be) -- which would you do?
>
> In case it was unclear, I was trying to say the hooks should not run with
> empty input.

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Sitaram Chamarty <sitaramc@gmail.com> writes:
> >
> >>> In that case (if "non-existent-ref" was indeed non-existent, and not just
> >>> pointing at a dangling commit), I would say the post anything hook should
> >>> not be called for that ref. These hooks of course need to run if there
> >>> are _other_ refs that were updated, though, to handle these _other_ refs,
> >>> but I do not think they should be told about the no-op.
> >>
> >> Question is what happens if none of them existed.  It's a difference
> >> between not calling the hook at all, versus calling it with no
> >> arguments/empty stdin (as the case may be) -- which would you do?
> >
> > In case it was unclear, I was trying to say the hooks should not run with
> > empty input.
> 
> If the purpose of "post-update" (or "post-receive") hooks were to trigger
> every time anybody attempted to push into the repository, then it would
> make perfect sense for them to trigger when "push origin :no-such-branch"
> were attempted. But if that were the purpose of these hooks, they should
> also trigger when "push origin master" is run and "master" is already at
> the right commit, as that is the same kind of no-op -- the pushed into
> repository was already up-to-date with respect to the wish of the pusher.
> 
> I do not mind, and I do prefer, these hooks to run when somebody deleted
> an existing ref that points at a corrupt or non-existent object, as that
> is _not_ a no-op but is a meaningful event that has an effect that is
> observable from the outside world (e.g. ls-remote).

> Should I reroll this patch with this behaviour:
>
> - Everything as usual for valid ref updates and deletes
> - For deleting corrupt (dangling?) ref, post-receive and post-update hooks
>   also receive the same args as per valid update / delete

> 
> Pang Yan Han <pangyanhan@gmail.com> writes:
> 
> > Should I reroll this patch with this behaviour:
> >
> > - Everything as usual for valid ref updates and deletes
> > - For deleting corrupt (dangling?) ref, post-receive and post-update hooks
> >   also receive the same args as per valid update / delete
> 
> Suonds sensible.
> 
> > - For deleting non-existent refs:
> >   - post-receive shall have empty stdin for those refs
> >   - post-update shall have an empty arg for those refs
> 
> I do not think these hooks should see names of refs that ended up being a
> no-op. If the push is only about attempting to delete a ref that did not
> exist, these hooks should not even get called. If there were other refs
> that got updated, these hooks have to be called, but they should not be
> told about the no-op.  IOW
> 
>     $ git push $there :no-such-ref master:refs/remotes/origin/master
> 
> should:
> 
>  (1) not call the post-* hooks if the refs/remotes/origin/master was
>      already pointing at the same commit; or
> 
>  (2) invoke the post-* hooks if refs/remotes/origin/master is updated, but
>      should tell hooks only about the update of refs/remotes/origin/master.
> 
> That is pretty much in line with how a normal attempt to push the same
> commit to an already up-to-date ref works.  For example, if you:
> 
>     $ git push $there master next
> 
> when 'master' is lagging and 'next' is already up-to-date, post-update and
> post-receive hooks run and told only about 'master' and not 'next'.

> Sitaram Chamarty <sitaramc@gmail.com> writes:
>
>>> In that case (if "non-existent-ref" was indeed non-existent, and not just
>>> pointing at a dangling commit), I would say the post anything hook should
>>> not be called for that ref. These hooks of course need to run if there
>>> are _other_ refs that were updated, though, to handle these _other_ refs,
>>> but I do not think they should be told about the no-op.
>>
>> Question is what happens if none of them existed.  It's a difference
>> between not calling the hook at all, versus calling it with no
>> arguments/empty stdin (as the case may be) -- which would you do?
>
> In case it was unclear, I was trying to say the hooks should not run with
> empty input.