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

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

Signed-off-by: Pang Yan Han <redacted>
---
 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

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 [off-list ref]
Signed-off-by: Pang Yan Han <redacted>
---
 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

On Sun, Sep 25, 2011 at 10:36 AM, Pang Yan Han [off-list ref] 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

On Sun, Sep 25, 2011 at 1:06 PM, Pang Yan Han [off-list ref] wrote:

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

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

On Sun, Sep 25, 2011 at 3:18 PM, Pang Yan Han [off-list ref] wrote:

On Sun, Sep 25, 2011 at 01:28:31PM +0530, Sitaram Chamarty wrote:

[snip]

quoted

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.

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