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

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

[Pang Yan Han]

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

[PATCH/RFC 1/2] is_url: Remove redundant assignment

[Pang Yan Han]

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. */
-- 
1.7.7.rc3.2.g29f2e6

Re: [PATCH/RFC 1/2] is_url: Remove redundant assignment

[Tay Ray Chuan]

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.

-- 
Cheers,
Ray Chuan

Re: [PATCH/RFC 1/2] is_url: Remove redundant assignment

[Junio C Hamano]

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] == '/');
	}

Re: [PATCH/RFC 1/2] is_url: Remove redundant assignment

[Jeff King]

[+cc Ilari, who wrote the code originally]

On Mon, Sep 26, 2011 at 09:52:54AM -0700, Junio C Hamano wrote:

> 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] == '/');
> 	}

That looks right to me, and is way more readable.

-Peff

[PATCH/RFC 2/2] receive-pack: Don't run update hook for corrupt or nonexistent ref

[Pang Yan Han]

Teach receive-pack not to run update hook for a corrupt or nonexistent ref.

In addition, update the warning message for deleting a corrupt or nonexistent
ref from:

"Allowing deletion of corrupt ref"

to:

"Allowing deletion of corrupt/nonexistent ref"

Noticed-by: Sitaram Chamarty <sitaramc@gmail.com>
Signed-off-by: Pang Yan Han <pangyanhan@gmail.com>
---
 builtin/receive-pack.c |   50 +++++++++++++++++++++++++++--------------------
 t/t5516-fetch-push.sh  |   33 +++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+), 21 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index ae164da..a9385cf 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -346,6 +346,17 @@ static void refuse_unconfigured_deny_delete_current(void)
 		rp_error("%s", refuse_unconfigured_deny_delete_current_msg[i]);
 }
 
+static const char *delete_null_sha1_ref(const char *namespaced_name,
+										const char *name,
+										unsigned char *old_sha1)
+{
+	if (delete_ref(namespaced_name, old_sha1, 0)) {
+		rp_error("failed to delete %s", name);
+		return "failed to delete";
+	}
+	return NULL; /* good */
+}
+
 static const char *update(struct command *cmd)
 {
 	const char *name = cmd->ref_name;
@@ -438,33 +449,30 @@ static const char *update(struct command *cmd)
 			return "non-fast-forward";
 		}
 	}
+
+	/* Don't run update hook for corrupt/nonexistent ref */
+	if (is_null_sha1(new_sha1) && !parse_object(old_sha1)) {
+		rp_warning("Allowing deletion of corrupt/nonexistent ref.");
+		old_sha1 = NULL;
+		return delete_null_sha1_ref(namespaced_name, name, old_sha1);
+	}
+
 	if (run_update_hook(cmd)) {
 		rp_error("hook declined to update %s", name);
 		return "hook declined";
 	}
 
-	if (is_null_sha1(new_sha1)) {
-		if (!parse_object(old_sha1)) {
-			rp_warning("Allowing deletion of corrupt ref.");
-			old_sha1 = NULL;
-		}
-		if (delete_ref(namespaced_name, old_sha1, 0)) {
-			rp_error("failed to delete %s", name);
-			return "failed to delete";
-		}
-		return NULL; /* good */
-	}
-	else {
-		lock = lock_any_ref_for_update(namespaced_name, old_sha1, 0);
-		if (!lock) {
-			rp_error("failed to lock %s", name);
-			return "failed to lock";
-		}
-		if (write_ref_sha1(lock, new_sha1, "push")) {
-			return "failed to write"; /* error() already called */
-		}
-		return NULL; /* good */
+	if (is_null_sha1(new_sha1))
+		return delete_null_sha1_ref(namespaced_name, name, old_sha1);
+
+	lock = lock_any_ref_for_update(namespaced_name, old_sha1, 0);
+	if (!lock) {
+		rp_error("failed to lock %s", name);
+		return "failed to lock";
 	}
+	if (write_ref_sha1(lock, new_sha1, "push"))
+		return "failed to write"; /* error() already called */
+	return NULL; /* good */
 }
 
 static char update_post_hook[] = "hooks/post-update";
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 3abb290..b7a74e3 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -40,6 +40,20 @@ mk_test () {
 	)
 }
 
+mk_test_with_update_hook () {
+	mk_test "$@" &&
+	(
+		cd testrepo &&
+		mkdir .git/hooks &&
+		cd .git/hooks &&
+		cat >update <<'EOF'
+#!/bin/sh
+printf %s "$@" >>update.args
+EOF
+		chmod u+x update
+	)
+}
+
 mk_child() {
 	rm -rf "$1" &&
 	git clone testrepo "$1"
@@ -559,6 +573,25 @@ test_expect_success 'allow deleting an invalid remote ref' '
 
 '
 
+test_expect_success 'deleting existing ref triggers update hook' '
+	mk_test_with_update_hook heads/master &&
+	(
+	cd testrepo &&
+	git rev-parse --verify refs/heads/master &&
+	git config receive.denyDeleteCurrent warn
+	) &&
+	git push testrepo :refs/heads/master &&
+	(cd testrepo && test_must_fail git rev-parse --verify refs/heads/master) &&
+	(cd testrepo/.git && test -s update.args)
+'
+
+test_expect_success 'deleting nonexistent ref does not trigger update hook' '
+	mk_test_with_update_hook heads/master &&
+	rm -f testrepo/.git/objects/??/* &&
+	git push testrepo :refs/heads/master &&
+	(cd testrepo/.git && ! test -f update.args)
+'
+
 test_expect_success 'allow deleting a ref using --delete' '
 	mk_test heads/master &&
 	(cd testrepo && git config receive.denyDeleteCurrent warn) &&
-- 
1.7.7.rc3.2.g29f2e6

[PATCH/RFCv2 2/2] run post-receive and post-update hooks with empty stdin/no args for invalid ref deletion

[Pang Yan Han]

The post-receive and post-update hooks are triggered with invalid input on
stdin and invalid args respectively during the deletion of corrupt or
non-existent refs during a push.

Teach receive-pack to run post-receive hook with empty stdin and post-update
hook with empty args in the event of an invalid ref deletion.

Signed-off-by: Pang Yan Han <pangyanhan@gmail.com>
---
This has been updated with the suggestions from Sitaram Chamarty, as stated in
http://thread.gmane.org/gmane.comp.version-control.git/182056/focus=182065

 builtin/receive-pack.c |   35 +++++++++++++-
 t/t5516-fetch-push.sh  |  118 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 150 insertions(+), 3 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index ae164da..28d0b09 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -153,6 +153,26 @@ struct command {
 	char ref_name[FLEX_ARRAY]; /* more */
 };
 
+/* For invalid refs */
+static struct command **invalid_delete;
+static size_t invalid_delete_nr;
+static size_t invalid_delete_alloc;
+
+static void invalid_delete_append(struct command *cmd)
+{
+	ALLOC_GROW(invalid_delete, invalid_delete_nr + 1, invalid_delete_alloc);
+	invalid_delete[invalid_delete_nr++] = cmd;
+}
+
+static int is_invalid_delete(struct command *cmd)
+{
+	size_t i;
+	for (i = 0; i < invalid_delete_nr; i++)
+		if (invalid_delete[i] == cmd)
+			return 1;
+	return 0;
+}
+
 static const char pre_receive_hook[] = "hooks/pre-receive";
 static const char post_receive_hook[] = "hooks/post-receive";
 
@@ -248,7 +268,8 @@ static int run_receive_hook(struct command *commands, const char *hook_name)
 	}
 
 	for (cmd = commands; cmd; cmd = cmd->next) {
-		if (!cmd->error_string) {
+		/* Run with empty stdin for invalid ref deletion */
+		if (!cmd->error_string && !is_invalid_delete(cmd)) {
 			size_t n = snprintf(buf, sizeof(buf), "%s %s %s\n",
 				sha1_to_hex(cmd->old_sha1),
 				sha1_to_hex(cmd->new_sha1),
@@ -447,6 +468,7 @@ static const char *update(struct command *cmd)
 		if (!parse_object(old_sha1)) {
 			rp_warning("Allowing deletion of corrupt ref.");
 			old_sha1 = NULL;
+			invalid_delete_append(cmd);
 		}
 		if (delete_ref(namespaced_name, old_sha1, 0)) {
 			rp_error("failed to delete %s", name);
@@ -490,8 +512,14 @@ static void run_update_post_hook(struct command *commands)
 		char *p;
 		if (cmd->error_string)
 			continue;
-		p = xmalloc(strlen(cmd->ref_name) + 1);
-		strcpy(p, cmd->ref_name);
+		if (is_invalid_delete(cmd)) {
+			/* Run with empty args for invalid ref deletion */
+			p = xmalloc(1);
+			p[0] = '\0';
+		} else {
+			p = xmalloc(strlen(cmd->ref_name) + 1);
+			strcpy(p, cmd->ref_name);
+		}
 		argv[argc] = p;
 		argc++;
 	}
@@ -866,5 +894,6 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 	}
 	if (use_sideband)
 		packet_flush(1);
+	free(invalid_delete);
 	return 0;
 }
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 3abb290..038a3b3 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -40,6 +40,39 @@ mk_test () {
 	)
 }
 
+mk_test_with_hooks() {
+	mk_test "$@" &&
+	(
+	cd testrepo &&
+	mkdir .git/hooks &&
+	cd .git/hooks &&
+
+	cat >pre-receive <<'EOF' &&
+#!/bin/sh
+cat - >>pre-receive.actual
+EOF
+
+	cat >update <<'EOF' &&
+#!/bin/sh
+printf "%s %s %s\n" "$@" >>update.actual
+EOF
+	cat >post-receive <<'EOF' &&
+#!/bin/sh
+cat - >>post-receive.actual
+EOF
+
+	cat >post-update <<'EOF' &&
+#!/bin/sh
+for ref in "$@"
+do
+	printf "%s\n" "$ref" >>post-update.actual
+done
+EOF
+
+	chmod u+x pre-receive update post-receive post-update
+	)
+}
+
 mk_child() {
 	rm -rf "$1" &&
 	git clone testrepo "$1"
@@ -559,6 +592,91 @@ test_expect_success 'allow deleting an invalid remote ref' '
 
 '
 
+test_expect_success 'pushing valid refs triggers post-receive and post-update hooks' '
+	mk_test_with_hooks heads/master &&
+	orgmaster=$(cd testrepo && git show-ref -s --verify refs/heads/master) &&
+	newmaster=$(git show-ref -s --verify refs/heads/master) &&
+	git push testrepo refs/heads/master:refs/heads/master :refs/heads/nonexistent &&
+	(cd testrepo/.git &&
+	cat >pre-receive.expect <<'EOF' &&
+$orgmaster $newmaster refs/heads/master
+$_z40 $_z40 refs/heads/nonexistent
+EOF
+
+	cat >update.expect <<'EOF' &&
+refs/heads/master $orgmaster $newmaster
+refs/heads/nonexistent $_z40 $_z40
+EOF
+
+	cat >post-receive.expect <<'EOF' &&
+$orgmaster $newmaster refs/heads/master
+EOF
+
+	cat >post-update.expect <<'EOF' &&
+refs/heads/master
+
+EOF
+
+	test_cmp pre-receive.expect pre-receive.actual &&
+	test_cmp update.expect update.actual &&
+	test_cmp post-receive.expect post-receive.actual &&
+	test_cmp post-update.expect post-update.actual
+	)
+'
+
+test_expect_success 'deleting non-existent ref triggers post-receive and post-update hooks with empty args' '
+	mk_test_with_hooks heads/master &&
+	git push testrepo :refs/heads/nonexistent &&
+	(cd testrepo/.git &&
+	cat >pre-receive.expect <<'EOF' &&
+$_z40 $_z40 refs/heads/nonexistent
+EOF
+
+	cat >update.expect <<'EOF' &&
+refs/heads/nonexistent $_z40 $_z40
+EOF
+
+	cat >post-receive.expect <<'EOF' &&
+EOF
+
+	cat >post-update.expect <<'EOF' &&
+
+EOF
+
+	test_cmp pre-receive.expect pre-receive.actual &&
+	test_cmp update.expect update.actual &&
+	test_cmp post-receive.expect post-receive.actual &&
+	test_cmp post-update.expect post-update.actual
+	)
+'
+
+test_expect_success 'deleting invalid ref triggers post-receive and post-update hooks with empty args' '
+	mk_test_with_hooks heads/master &&
+	rm -f testrepo/.git/objects/??/* &&
+	git push testrepo :refs/heads/master &&
+	(cd testrepo/.git &&
+	cat >pre-receive.expect <<'EOF' &&
+$_z40 $_z40 refs/heads/master
+EOF
+
+	cat >update.expect <<'EOF' &&
+refs/heads/master $_z40 $_z40
+EOF
+
+	cat >post-receive.expect <<'EOF' &&
+EOF
+
+	cat >post-update.expect <<'EOF' &&
+
+EOF
+
+	test_cmp pre-receive.expect pre-receive.actual &&
+	test_cmp update.expect update.actual &&
+	test_cmp post-receive.expect post-receive.actual &&
+	test_cmp post-update.expect post-update.actual
+	)
+'
+
 test_expect_success 'allow deleting a ref using --delete' '
 	mk_test heads/master &&
 	(cd testrepo && git config receive.denyDeleteCurrent warn) &&
-- 
1.7.7.rc3.2.gd706f.dirty

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

[Sitaram Chamarty]

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

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

[Pang Yan Han]

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

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

[Sitaram Chamarty]

On Sun, Sep 25, 2011 at 3:18 PM, Pang Yan Han <pangyanhan@gmail.com> wrote:
> On Sun, Sep 25, 2011 at 01:28:31PM +0530, Sitaram Chamarty wrote:

[snip]

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

Sorry if I sounded aggressive; I was going to brevity, and levity suffered :-)

[snip lots of stuff about new approach]

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

I'm sorry again but it's been almost 2 decades since I did any serious
C and I've never dug into git internals, so I can't tell you if you're
even on the right track.  You should wait for one of the other folks
you cc-d to weigh in with their opinions.

Personally, anytime someone says "disable the update hook" I get very
worried -- I've got a heck of a lot invested in update hooks ;-)

I wasn't even *asking* about disabling that; I was asking about
*post*-update, which you didn't even address in your code.

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.

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.

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.

That is, IMO, the correct way to deal with this.

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

[Junio C Hamano]

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.

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

[Sitaram Chamarty]

On Tue, Sep 27, 2011 at 4:53 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

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?  I
say the hook *should* always run, and the code inside the hook should
take care of the fact that no arguments/no input means nothing
actually happened.

-- 
Sitaram

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

[Junio C Hamano]

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.

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

[Junio C Hamano]

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

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

[Pang Yan Han]

Hi Junio,

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
- For deleting non-existent refs:
  - post-receive shall have empty stdin for those refs
  - post-update shall have an empty arg for those refs

Thanks.

On Mon, Sep 26, 2011 at 05:04:24PM -0700, Junio C Hamano wrote:
> 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).

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

[Junio C Hamano]

[offtopic: where does that annoying M-F-T header come from? It even seems
to be pointless in this case as it lists the same people as are already on
To/Cc/From of the message.]

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

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

[Pang Yan Han]

On Tue, Sep 27, 2011 at 09:56:52AM -0700, Junio C Hamano wrote:
> [offtopic: where does that annoying M-F-T header come from? It even seems
> to be pointless in this case as it lists the same people as are already on
> To/Cc/From of the message.]

Sorry, it's due to my lack of familiarity with mutt.

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

Thanks, I will reroll this later.

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

[Sitaram Chamarty]

On Tue, Sep 27, 2011 at 5:19 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

I saw "should not be called for that ref" and I did get confused;
thanks for clarifying.

I perfectly fine with it for post-{update,receive}.  My interest is in
making sure the *update* hook runs, which (in an earlier email in the
thread) I explained why and you agreed it made sense.

Thanks,

-- 
Sitaram