[PATCH/RFCv3 2/2] receive-pack: don't pass non-existent refs to post-{receive,update} hooks in push deletions

[PATCH/RFCv3 2/2] receive-pack: don't pass non-existent refs to post-{receive,update} hooks in push deletions

[Pang Yan Han]

When a push specifies deletion of non-existent refs, the post post-receive and
post-update hooks receive as input/arguments the non-existent refs.

For instance, for the following push, where refs/heads/nonexistent is a ref
which does not exist on the remote side:

	git push origin :refs/heads/nonexistent

the post-receive hook receives from standard input:

	<null-sha1> SP <null-sha1> SP refs/heads/nonexistent

and the post-update hook receives as arguments:

	refs/heads/nonexistent

which does not make sense since it is a no-op.

Teach receive-pack to not pass non-existent refs as input / arguments to the
post-receive and post-update hooks in the event of a push involving
non-existent ref deletion.

Signed-off-by: Pang Yan Han <pangyanhan@gmail.com>
---
 builtin/receive-pack.c |   31 ++++++++-
 t/t5516-fetch-push.sh  |  170 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 197 insertions(+), 4 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index ae164da..8a0a9d2 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";
 
@@ -215,7 +235,7 @@ static int run_receive_hook(struct command *commands, const char *hook_name)
 	int have_input = 0, code;
 
 	for (cmd = commands; !have_input && cmd; cmd = cmd->next) {
-		if (!cmd->error_string)
+		if (!cmd->error_string && !is_invalid_delete(cmd))
 			have_input = 1;
 	}
 
@@ -248,7 +268,7 @@ static int run_receive_hook(struct command *commands, const char *hook_name)
 	}
 
 	for (cmd = commands; cmd; cmd = cmd->next) {
-		if (!cmd->error_string) {
+		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 +467,8 @@ static const char *update(struct command *cmd)
 		if (!parse_object(old_sha1)) {
 			rp_warning("Allowing deletion of corrupt ref.");
 			old_sha1 = NULL;
+			if (!ref_exists((char *) name))
+				invalid_delete_append(cmd);
 		}
 		if (delete_ref(namespaced_name, old_sha1, 0)) {
 			rp_error("failed to delete %s", name);
@@ -477,7 +499,7 @@ static void run_update_post_hook(struct command *commands)
 	struct child_process proc;
 
 	for (argc = 0, cmd = commands; cmd; cmd = cmd->next) {
-		if (cmd->error_string)
+		if (cmd->error_string || is_invalid_delete(cmd))
 			continue;
 		argc++;
 	}
@@ -488,7 +510,7 @@ static void run_update_post_hook(struct command *commands)
 
 	for (argc = 1, cmd = commands; cmd; cmd = cmd->next) {
 		char *p;
-		if (cmd->error_string)
+		if (cmd->error_string || is_invalid_delete(cmd))
 			continue;
 		p = xmalloc(strlen(cmd->ref_name) + 1);
 		strcpy(p, cmd->ref_name);
@@ -866,5 +888,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..c0d8a0e 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,143 @@ 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 heads/next &&
+	orgmaster=$(cd testrepo && git show-ref -s --verify refs/heads/master) &&
+	newmaster=$(git show-ref -s --verify refs/heads/master) &&
+	orgnext=$(cd testrepo && git show-ref -s --verify refs/heads/next) &&
+	newnext=$_z40 &&
+	git push testrepo refs/heads/master:refs/heads/master :refs/heads/next &&
+	(cd testrepo/.git &&
+	cat >pre-receive.expect <<'EOF' &&
+$orgmaster $newmaster refs/heads/master
+$orgnext $newnext refs/heads/next
+EOF
+
+	cat >update.expect <<'EOF' &&
+refs/heads/master $orgmaster $newmaster
+refs/heads/next $orgnext $newnext
+EOF
+
+	cat >post-receive.expect <<'EOF' &&
+$orgmaster $newmaster refs/heads/master
+$orgnext $newnext refs/heads/next
+EOF
+
+	cat >post-update.expect <<'EOF' &&
+refs/heads/master
+refs/heads/next
+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 dangling ref triggers hooks with correct 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' &&
+$_z40 $_z40 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 does not trigger 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 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 'mixed ref updates, deletes, invalid deletes trigger hooks with correct input' '
+	mk_test_with_hooks heads/master heads/next heads/pu &&
+	orgmaster=$(cd testrepo && git show-ref -s --verify refs/heads/master) &&
+	newmaster=$(git show-ref -s --verify refs/heads/master) &&
+	orgnext=$(cd testrepo && git show-ref -s --verify refs/heads/next) &&
+	newnext=$_z40 &&
+	orgpu=$(cd testrepo && git show-ref -s --verify refs/heads/pu) &&
+	newpu=$(git show-ref -s --verify refs/heads/master) &&
+	git push testrepo refs/heads/master:refs/heads/master refs/heads/master:refs/heads/pu :refs/heads/next :refs/heads/nonexistent &&
+	(cd testrepo/.git &&
+	cat >pre-receive.expect <<'EOF' &&
+$orgmaster $newmaster refs/heads/master
+$orgnext $newnext refs/heads/next
+$orgpu $newpu refs/heads/pu
+$_z40 $_z40 refs/heads/nonexistent
+EOF
+
+	cat >update.expect <<'EOF' &&
+refs/heads/master $orgmaster $newmaster
+refs/heads/next $orgnext $newnext
+refs/heads/pu $orgpu $newpu
+refs/heads/nonexistent $_z40 $_z40
+EOF
+
+	cat >post-receive.expect <<'EOF' &&
+$orgmaster $newmaster refs/heads/master
+$orgnext $newnext refs/heads/next
+$orgpu $newpu refs/heads/pu
+EOF
+
+	cat >post-update.expect <<'EOF' &&
+refs/heads/master
+refs/heads/next
+refs/heads/pu
+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.g6bf07

Re: [PATCH/RFCv3 2/2] receive-pack: don't pass non-existent refs to post-{receive,update} hooks in push deletions

[Junio C Hamano]

Pang Yan Han <pangyanhan@gmail.com> writes:

> +/* For invalid refs */
> +static struct command **invalid_delete;
> +static size_t invalid_delete_nr;
> +static size_t invalid_delete_alloc;

Do you have to have these separately only to keep track of the corner case
errors?  I would have expected that it would be more natural to mark them
by adding a single bitfield to "struct command".

> @@ -447,6 +467,8 @@ static const char *update(struct command *cmd)
>  		if (!parse_object(old_sha1)) {
>  			rp_warning("Allowing deletion of corrupt ref.");
>  			old_sha1 = NULL;
> +			if (!ref_exists((char *) name))
> +				invalid_delete_append(cmd);

This is not an "invalid" delete but deleting a non-existing ref.  Perhaps
you would want to move the warning and optionally reword it as well, e.g.

	if (!parse_object(old_sha1)) {
        	old_sha1 = NULL;
                if (ref_exists(name)) {
			rp_warning("Allowing deletion of corrupt ref.");
		} else {
			rp_warning("Deleting a ref that does not exist.");
			cmd->did_not_exist = 1;
		}
		...

Re: [PATCH/RFCv3 2/2] receive-pack: don't pass non-existent refs to post-{receive,update} hooks in push deletions

[Pang Yan Han]

On Wed, Sep 28, 2011 at 03:37:32PM -0700, Junio C Hamano wrote:
> Pang Yan Han <pangyanhan@gmail.com> writes:
> 
> > +/* For invalid refs */
> > +static struct command **invalid_delete;
> > +static size_t invalid_delete_nr;
> > +static size_t invalid_delete_alloc;
> 
> Do you have to have these separately only to keep track of the corner case
> errors?  I would have expected that it would be more natural to mark them
> by adding a single bitfield to "struct command".

Yes they are purely for keeping track of deleting non-existent refs.
Ok I will add the bitfield to struct command.

> 
> > @@ -447,6 +467,8 @@ static const char *update(struct command *cmd)
> >  		if (!parse_object(old_sha1)) {
> >  			rp_warning("Allowing deletion of corrupt ref.");
> >  			old_sha1 = NULL;
> > +			if (!ref_exists((char *) name))
> > +				invalid_delete_append(cmd);
> 
> This is not an "invalid" delete but deleting a non-existing ref.  Perhaps
> you would want to move the warning and optionally reword it as well, e.g.
> 
> 	if (!parse_object(old_sha1)) {
>         	old_sha1 = NULL;
>                 if (ref_exists(name)) {
> 			rp_warning("Allowing deletion of corrupt ref.");
> 		} else {
> 			rp_warning("Deleting a ref that does not exist.");
> 			cmd->did_not_exist = 1;
> 		}
> 		...

Sure thing.

I am unable to reply this until much later, but are the tests in this patch ok?
It's the first time I'm writing test cases for git.

Thanks for the feedback!

Re: [PATCH/RFCv3 2/2] receive-pack: don't pass non-existent refs to post-{receive,update} hooks in push deletions

[Junio C Hamano]

Here is a quick fix-up on top.

Points of interest:

 - Get rid of an unnecessary list; just a single bitfield is sufficient;
   while at it, make skip_update also a bitfield.

 - ref_exists(refname) calls resolve_ref() which wants "const char *",
   and does not touch refname itself. Make it to take "const char *",
   instead of casting const away in the new caller.

 - Properly indent test script.  Also:

   - Use "cmd <<-EOF" to allow indenting the here text with leading tabs;

   - Strip useless use of single quote around "cmd <<'EOF'" inside
     test_expect_success; they were only stepping out of the sq context
     while the third argument to test_expect_success was being parsed. A
     construct like this:

	cat >pre-receive.expect <<'EOF'
        $orgmaster $newmaster refs/heads/master
        EOF

     only misleads the readers as if the file is getting literal variable
     names without substitution, which is not what is going on.

I didn't fix this in this fix-up patch, but you also need to test the case
where _ONLY_ deletion of a non-existent ref is requested, in which case,
pre-receive and update should be told about it, but post-receive and
post-update should not be even run (i.e. test the absense of these
post-*.actual files).

 builtin/receive-pack.c |   41 ++++++---------------
 refs.c                 |    2 +-
 refs.h                 |    2 +-
 t/t5516-fetch-push.sh  |   90 ++++++++++++++++++++++++-----------------------
 4 files changed, 60 insertions(+), 75 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 8a0a9d2..b73f18a 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -147,32 +147,13 @@ static void write_head_info(void)
 struct command {
 	struct command *next;
 	const char *error_string;
-	unsigned int skip_update;
+	unsigned int skip_update:1,
+		     did_not_exist:1;
 	unsigned char old_sha1[20];
 	unsigned char new_sha1[20];
 	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";
 
@@ -235,7 +216,7 @@ static int run_receive_hook(struct command *commands, const char *hook_name)
 	int have_input = 0, code;
 
 	for (cmd = commands; !have_input && cmd; cmd = cmd->next) {
-		if (!cmd->error_string && !is_invalid_delete(cmd))
+		if (!cmd->error_string && !cmd->did_not_exist)
 			have_input = 1;
 	}
 
@@ -268,7 +249,7 @@ static int run_receive_hook(struct command *commands, const char *hook_name)
 	}
 
 	for (cmd = commands; cmd; cmd = cmd->next) {
-		if (!cmd->error_string && !is_invalid_delete(cmd)) {
+		if (!cmd->error_string && !cmd->did_not_exist) {
 			size_t n = snprintf(buf, sizeof(buf), "%s %s %s\n",
 				sha1_to_hex(cmd->old_sha1),
 				sha1_to_hex(cmd->new_sha1),
@@ -465,10 +446,13 @@ static const char *update(struct command *cmd)
 
 	if (is_null_sha1(new_sha1)) {
 		if (!parse_object(old_sha1)) {
-			rp_warning("Allowing deletion of corrupt ref.");
 			old_sha1 = NULL;
-			if (!ref_exists((char *) name))
-				invalid_delete_append(cmd);
+			if (ref_exists(name)) {
+				rp_warning("Allowing deletion of corrupt ref.");
+			} else {
+				rp_warning("Deleting a non-existent ref.");
+				cmd->did_not_exist = 1;
+			}
 		}
 		if (delete_ref(namespaced_name, old_sha1, 0)) {
 			rp_error("failed to delete %s", name);
@@ -499,7 +483,7 @@ static void run_update_post_hook(struct command *commands)
 	struct child_process proc;
 
 	for (argc = 0, cmd = commands; cmd; cmd = cmd->next) {
-		if (cmd->error_string || is_invalid_delete(cmd))
+		if (cmd->error_string || cmd->did_not_exist)
 			continue;
 		argc++;
 	}
@@ -510,7 +494,7 @@ static void run_update_post_hook(struct command *commands)
 
 	for (argc = 1, cmd = commands; cmd; cmd = cmd->next) {
 		char *p;
-		if (cmd->error_string || is_invalid_delete(cmd))
+		if (cmd->error_string || cmd->did_not_exist)
 			continue;
 		p = xmalloc(strlen(cmd->ref_name) + 1);
 		strcpy(p, cmd->ref_name);
@@ -888,6 +872,5 @@ 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/refs.c b/refs.c
index a615043..43e9225 100644
--- a/refs.c
+++ b/refs.c
@@ -1851,7 +1851,7 @@ int update_ref(const char *action, const char *refname,
 	return 0;
 }
 
-int ref_exists(char *refname)
+int ref_exists(const char *refname)
 {
 	unsigned char sha1[20];
 	return !!resolve_ref(refname, sha1, 1, NULL);
diff --git a/refs.h b/refs.h
index dfb086e..4431205 100644
--- a/refs.h
+++ b/refs.h
@@ -57,7 +57,7 @@ extern void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char *refn
  */
 extern void add_extra_ref(const char *refname, const unsigned char *sha1, int flags);
 extern void clear_extra_refs(void);
-extern int ref_exists(char *);
+extern int ref_exists(const char *);
 
 extern int peel_ref(const char *, unsigned char *);
 
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index c0d8a0e..47db4b1 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -43,33 +43,34 @@ mk_test () {
 mk_test_with_hooks() {
 	mk_test "$@" &&
 	(
-	cd testrepo &&
-	mkdir .git/hooks &&
-	cd .git/hooks &&
+		cd testrepo &&
+		mkdir .git/hooks &&
+		cd .git/hooks &&
 
-	cat >pre-receive <<'EOF' &&
-#!/bin/sh
-cat - >>pre-receive.actual
-EOF
+		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 >update <<-'EOF' &&
+		#!/bin/sh
+		printf "%s %s %s\n" "$@" >>update.actual
+		EOF
 
-	cat >post-update <<'EOF' &&
-#!/bin/sh
-for ref in "$@"
-do
-	printf "%s\n" "$ref" >>post-update.actual
-done
-EOF
+		cat >post-receive <<-'EOF' &&
+		#!/bin/sh
+		cat - >>post-receive.actual
+		EOF
 
-	chmod u+x pre-receive update post-receive post-update
+		cat >post-update <<-'EOF' &&
+		#!/bin/sh
+		for ref in "$@"
+		do
+			printf "%s\n" "$ref" >>post-update.actual
+		done
+		EOF
+
+		chmod +x pre-receive update post-receive post-update
 	)
 }
 
@@ -599,31 +600,32 @@ test_expect_success 'pushing valid refs triggers post-receive and post-update ho
 	orgnext=$(cd testrepo && git show-ref -s --verify refs/heads/next) &&
 	newnext=$_z40 &&
 	git push testrepo refs/heads/master:refs/heads/master :refs/heads/next &&
-	(cd testrepo/.git &&
-	cat >pre-receive.expect <<'EOF' &&
-$orgmaster $newmaster refs/heads/master
-$orgnext $newnext refs/heads/next
-EOF
+	(
+		cd testrepo/.git &&
+		cat >pre-receive.expect <<-EOF &&
+		$orgmaster $newmaster refs/heads/master
+		$orgnext $newnext refs/heads/next
+		EOF
 
-	cat >update.expect <<'EOF' &&
-refs/heads/master $orgmaster $newmaster
-refs/heads/next $orgnext $newnext
-EOF
+		cat >update.expect <<-EOF &&
+		refs/heads/master $orgmaster $newmaster
+		refs/heads/next $orgnext $newnext
+		EOF
 
-	cat >post-receive.expect <<'EOF' &&
-$orgmaster $newmaster refs/heads/master
-$orgnext $newnext refs/heads/next
-EOF
+		cat >post-receive.expect <<-EOF &&
+		$orgmaster $newmaster refs/heads/master
+		$orgnext $newnext refs/heads/next
+		EOF
 
-	cat >post-update.expect <<'EOF' &&
-refs/heads/master
-refs/heads/next
-EOF
+		cat >post-update.expect <<-EOF &&
+		refs/heads/master
+		refs/heads/next
+		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_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
 	)
 '
 
-- 
1.7.7.rc3.4.g8d714

Re: [PATCH/RFCv3 2/2] receive-pack: don't pass non-existent refs to post-{receive,update} hooks in push deletions

[Pang Yan Han]

Sorry Junio, disregard my earlier message. I didn't see this email.

On Wed, Sep 28, 2011 at 04:09:07PM -0700, Junio C Hamano wrote:
> Here is a quick fix-up on top.
> 
> Points of interest:
> 
>  - Get rid of an unnecessary list; just a single bitfield is sufficient;
>    while at it, make skip_update also a bitfield.
> 
>  - ref_exists(refname) calls resolve_ref() which wants "const char *",
>    and does not touch refname itself. Make it to take "const char *",
>    instead of casting const away in the new caller.
> 
>  - Properly indent test script.  Also:
> 
>    - Use "cmd <<-EOF" to allow indenting the here text with leading tabs;
> 
>    - Strip useless use of single quote around "cmd <<'EOF'" inside
>      test_expect_success; they were only stepping out of the sq context
>      while the third argument to test_expect_success was being parsed. A
>      construct like this:
> 
> 	cat >pre-receive.expect <<'EOF'
>         $orgmaster $newmaster refs/heads/master
>         EOF
> 
>      only misleads the readers as if the file is getting literal variable
>      names without substitution, which is not what is going on.
> 
> I didn't fix this in this fix-up patch, but you also need to test the case
> where _ONLY_ deletion of a non-existent ref is requested, in which case,
> pre-receive and update should be told about it, but post-receive and
> post-update should not be even run (i.e. test the absense of these
> post-*.actual files).
> 
>  builtin/receive-pack.c |   41 ++++++---------------
>  refs.c                 |    2 +-
>  refs.h                 |    2 +-
>  t/t5516-fetch-push.sh  |   90 ++++++++++++++++++++++++-----------------------
>  4 files changed, 60 insertions(+), 75 deletions(-)
> 
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 8a0a9d2..b73f18a 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -147,32 +147,13 @@ static void write_head_info(void)
>  struct command {
>  	struct command *next;
>  	const char *error_string;
> -	unsigned int skip_update;
> +	unsigned int skip_update:1,
> +		     did_not_exist:1;
>  	unsigned char old_sha1[20];
>  	unsigned char new_sha1[20];
>  	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";
>  
> @@ -235,7 +216,7 @@ static int run_receive_hook(struct command *commands, const char *hook_name)
>  	int have_input = 0, code;
>  
>  	for (cmd = commands; !have_input && cmd; cmd = cmd->next) {
> -		if (!cmd->error_string && !is_invalid_delete(cmd))
> +		if (!cmd->error_string && !cmd->did_not_exist)
>  			have_input = 1;
>  	}
>  
> @@ -268,7 +249,7 @@ static int run_receive_hook(struct command *commands, const char *hook_name)
>  	}
>  
>  	for (cmd = commands; cmd; cmd = cmd->next) {
> -		if (!cmd->error_string && !is_invalid_delete(cmd)) {
> +		if (!cmd->error_string && !cmd->did_not_exist) {
>  			size_t n = snprintf(buf, sizeof(buf), "%s %s %s\n",
>  				sha1_to_hex(cmd->old_sha1),
>  				sha1_to_hex(cmd->new_sha1),
> @@ -465,10 +446,13 @@ static const char *update(struct command *cmd)
>  
>  	if (is_null_sha1(new_sha1)) {
>  		if (!parse_object(old_sha1)) {
> -			rp_warning("Allowing deletion of corrupt ref.");
>  			old_sha1 = NULL;
> -			if (!ref_exists((char *) name))
> -				invalid_delete_append(cmd);
> +			if (ref_exists(name)) {
> +				rp_warning("Allowing deletion of corrupt ref.");
> +			} else {
> +				rp_warning("Deleting a non-existent ref.");
> +				cmd->did_not_exist = 1;
> +			}
>  		}
>  		if (delete_ref(namespaced_name, old_sha1, 0)) {
>  			rp_error("failed to delete %s", name);
> @@ -499,7 +483,7 @@ static void run_update_post_hook(struct command *commands)
>  	struct child_process proc;
>  
>  	for (argc = 0, cmd = commands; cmd; cmd = cmd->next) {
> -		if (cmd->error_string || is_invalid_delete(cmd))
> +		if (cmd->error_string || cmd->did_not_exist)
>  			continue;
>  		argc++;
>  	}
> @@ -510,7 +494,7 @@ static void run_update_post_hook(struct command *commands)
>  
>  	for (argc = 1, cmd = commands; cmd; cmd = cmd->next) {
>  		char *p;
> -		if (cmd->error_string || is_invalid_delete(cmd))
> +		if (cmd->error_string || cmd->did_not_exist)
>  			continue;
>  		p = xmalloc(strlen(cmd->ref_name) + 1);
>  		strcpy(p, cmd->ref_name);
> @@ -888,6 +872,5 @@ 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/refs.c b/refs.c
> index a615043..43e9225 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1851,7 +1851,7 @@ int update_ref(const char *action, const char *refname,
>  	return 0;
>  }
>  
> -int ref_exists(char *refname)
> +int ref_exists(const char *refname)
>  {
>  	unsigned char sha1[20];
>  	return !!resolve_ref(refname, sha1, 1, NULL);
> diff --git a/refs.h b/refs.h
> index dfb086e..4431205 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -57,7 +57,7 @@ extern void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char *refn
>   */
>  extern void add_extra_ref(const char *refname, const unsigned char *sha1, int flags);
>  extern void clear_extra_refs(void);
> -extern int ref_exists(char *);
> +extern int ref_exists(const char *);
>  
>  extern int peel_ref(const char *, unsigned char *);
>  
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index c0d8a0e..47db4b1 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -43,33 +43,34 @@ mk_test () {
>  mk_test_with_hooks() {
>  	mk_test "$@" &&
>  	(
> -	cd testrepo &&
> -	mkdir .git/hooks &&
> -	cd .git/hooks &&
> +		cd testrepo &&
> +		mkdir .git/hooks &&
> +		cd .git/hooks &&
>  
> -	cat >pre-receive <<'EOF' &&
> -#!/bin/sh
> -cat - >>pre-receive.actual
> -EOF
> +		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 >update <<-'EOF' &&
> +		#!/bin/sh
> +		printf "%s %s %s\n" "$@" >>update.actual
> +		EOF
>  
> -	cat >post-update <<'EOF' &&
> -#!/bin/sh
> -for ref in "$@"
> -do
> -	printf "%s\n" "$ref" >>post-update.actual
> -done
> -EOF
> +		cat >post-receive <<-'EOF' &&
> +		#!/bin/sh
> +		cat - >>post-receive.actual
> +		EOF
>  
> -	chmod u+x pre-receive update post-receive post-update
> +		cat >post-update <<-'EOF' &&
> +		#!/bin/sh
> +		for ref in "$@"
> +		do
> +			printf "%s\n" "$ref" >>post-update.actual
> +		done
> +		EOF
> +
> +		chmod +x pre-receive update post-receive post-update
>  	)
>  }
>  
> @@ -599,31 +600,32 @@ test_expect_success 'pushing valid refs triggers post-receive and post-update ho
>  	orgnext=$(cd testrepo && git show-ref -s --verify refs/heads/next) &&
>  	newnext=$_z40 &&
>  	git push testrepo refs/heads/master:refs/heads/master :refs/heads/next &&
> -	(cd testrepo/.git &&
> -	cat >pre-receive.expect <<'EOF' &&
> -$orgmaster $newmaster refs/heads/master
> -$orgnext $newnext refs/heads/next
> -EOF
> +	(
> +		cd testrepo/.git &&
> +		cat >pre-receive.expect <<-EOF &&
> +		$orgmaster $newmaster refs/heads/master
> +		$orgnext $newnext refs/heads/next
> +		EOF
>  
> -	cat >update.expect <<'EOF' &&
> -refs/heads/master $orgmaster $newmaster
> -refs/heads/next $orgnext $newnext
> -EOF
> +		cat >update.expect <<-EOF &&
> +		refs/heads/master $orgmaster $newmaster
> +		refs/heads/next $orgnext $newnext
> +		EOF
>  
> -	cat >post-receive.expect <<'EOF' &&
> -$orgmaster $newmaster refs/heads/master
> -$orgnext $newnext refs/heads/next
> -EOF
> +		cat >post-receive.expect <<-EOF &&
> +		$orgmaster $newmaster refs/heads/master
> +		$orgnext $newnext refs/heads/next
> +		EOF
>  
> -	cat >post-update.expect <<'EOF' &&
> -refs/heads/master
> -refs/heads/next
> -EOF
> +		cat >post-update.expect <<-EOF &&
> +		refs/heads/master
> +		refs/heads/next
> +		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_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
>  	)
>  '
>  
> -- 
> 1.7.7.rc3.4.g8d714
> 

Re: [PATCH/RFCv3 2/2] receive-pack: don't pass non-existent refs to post-{receive,update} hooks in push deletions

[Junio C Hamano]

Pang Yan Han <pangyanhan@gmail.com> writes:

> I am unable to reply this until much later, but are the tests in this patch ok?
> It's the first time I'm writing test cases for git.

I'll squash in a bit more updates and later publish the result.
Thanks.

Re: [PATCH/RFCv3 2/2] receive-pack: don't pass non-existent refs to post-{receive,update} hooks in push deletions

[Pang Yan Han]

On Wed, Sep 28, 2011 at 04:28:11PM -0700, Junio C Hamano wrote:
> Pang Yan Han <pangyanhan@gmail.com> writes:
> 
> > I am unable to reply this until much later, but are the tests in this patch ok?
> > It's the first time I'm writing test cases for git.
> 
> I'll squash in a bit more updates and later publish the result.
> Thanks.

Hi Junio,

Thank you for correcting my many amateur mistakes!

Sorry for asking, but do I need to reroll this with the fixup in
origin/ph/push-to-delete-nothing ? Is the commit message fine especially
in light of the changes in the fixup?

Thanks.

Re: [PATCH/RFCv3 2/2] receive-pack: don't pass non-existent refs to post-{receive,update} hooks in push deletions

[Junio C Hamano]

Pang Yan Han <pangyanhan@gmail.com> writes:

> Sorry for asking, but do I need to reroll this with the fixup in
> origin/ph/push-to-delete-nothing ? Is the commit message fine especially
> in light of the changes in the fixup?

If you think that the result of squashing the fix-up commit into your
patch looks OK, I do not think there is a need to reroll. The patch title
may need to be shortened, but other than that I do not see anything
glaringly wrong in the commit log message that I cannot amend out.

Thanks.