Merge branch 'ph/push-to-delete-nothing'

* ph/push-to-delete-nothing:
  receive-pack: don't pass non-existent refs to post-{receive,update} hooks

Conflicts:
	builtin/receive-pack.c

2 files changed, 215 insertions, 8 deletions

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index e2d3f94661..261b610d24 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -154,7 +154,8 @@ 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 */
@@ -264,6 +265,7 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed, void *feed_sta
 
 struct receive_hook_feed_state {
 	struct command *cmd;
+	int skip_broken;
 	struct strbuf buf;
 };
 
@@ -272,7 +274,8 @@ static int feed_receive_hook(void *state_, const char **bufp, size_t *sizep)
 	struct receive_hook_feed_state *state = state_;
 	struct command *cmd = state->cmd;
 
-	while (cmd && cmd->error_string)
+	while (cmd &&
+	       state->skip_broken && (cmd->error_string || cmd->did_not_exist))
 		cmd = cmd->next;
 	if (!cmd)
 		return -1; /* EOF */
@@ -288,13 +291,15 @@ static int feed_receive_hook(void *state_, const char **bufp, size_t *sizep)
 	return 0;
 }
 
-static int run_receive_hook(struct command *commands, const char *hook_name)
+static int run_receive_hook(struct command *commands, const char *hook_name,
+			    int skip_broken)
 {
 	struct receive_hook_feed_state state;
 	int status;
 
 	strbuf_init(&state.buf, 0);
 	state.cmd = commands;
+	state.skip_broken = skip_broken;
 	if (feed_receive_hook(&state, NULL, NULL))
 		return 0;
 	state.cmd = commands;
@@ -485,8 +490,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(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);
@@ -517,7 +527,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 || cmd->did_not_exist)
 			continue;
 		argc++;
 	}
@@ -528,7 +538,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 || cmd->did_not_exist)
 			continue;
 		p = xmalloc(strlen(cmd->ref_name) + 1);
 		strcpy(p, cmd->ref_name);
@@ -672,7 +682,7 @@ static void execute_commands(struct command *commands, const char *unpacker_erro
 				       0, &cmd))
 		set_connectivity_errors(commands);
 
-	if (run_receive_hook(commands, pre_receive_hook)) {
+	if (run_receive_hook(commands, pre_receive_hook, 0)) {
 		for (cmd = commands; cmd; cmd = cmd->next)
 			cmd->error_string = "pre-receive hook declined";
 		return;
@@ -940,7 +950,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 			unlink_or_warn(pack_lockfile);
 		if (report_status)
 			report(commands, unpack_status);
-		run_receive_hook(commands, post_receive_hook);
+		run_receive_hook(commands, post_receive_hook, 1);
 		run_update_post_hook(commands);
 		if (auto_gc) {
 			const char *argv_gc_auto[] = {
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 3abb2907ea..b69cf574d7 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -40,6 +40,40 @@ 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 +x pre-receive update post-receive post-update
+	)
+}
+
 mk_child() {
 	rm -rf "$1" &&
 	git clone testrepo "$1"
@@ -559,6 +593,169 @@ 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 'deletion of a non-existent ref is not fed to 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 'deletion of a non-existent ref alone does trigger post-receive and post-update hooks' '
+	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
+
+		test_cmp pre-receive.expect pre-receive.actual &&
+		test_cmp update.expect update.actual &&
+		test_path_is_missing post-receive.actual &&
+		test_path_is_missing 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) &&