[PATCH v2 1/2] receive-pack: switch global variable 'commands' to a parameter

[PATCH v2 1/2] receive-pack: switch global variable 'commands' to a parameter

[Jay Soffian]

Receive-pack is inconsistent in its usage of the 'commands'
variable; though it is setup as a global and accessed that way by
execute_commands(), report(), and run_receive_hook(), it is also
passed as a parameter to delete_only() and run_update_post_hook().

For consistency, make it local to cmd_receive_pack and pass it as a
parameter. As long as we're cleaning up, also make our use of the
names 'commands' and 'cmd' consistent.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
 builtin/receive-pack.c |   60 +++++++++++++++++++++--------------------------
 1 files changed, 27 insertions(+), 33 deletions(-)

Changes from v1:

- Minor reword of commit message
- Removed two extraneous whitespace changes

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 0559fcc..fffb6ea 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -134,8 +134,6 @@ struct command {
 	char ref_name[FLEX_ARRAY]; /* more */
 };
 
-static struct command *commands;
-
 static const char pre_receive_hook[] = "hooks/pre-receive";
 static const char post_receive_hook[] = "hooks/post-receive";
 
@@ -188,7 +186,7 @@ static int copy_to_sideband(int in, int out, void *arg)
 	return 0;
 }
 
-static int run_receive_hook(const char *hook_name)
+static int run_receive_hook(struct command *commands, const char *hook_name)
 {
 	static char buf[sizeof(commands->old_sha1) * 2 + PATH_MAX + 4];
 	struct command *cmd;
@@ -447,15 +445,15 @@ static const char *update(struct command *cmd)
 
 static char update_post_hook[] = "hooks/post-update";
 
-static void run_update_post_hook(struct command *cmd)
+static void run_update_post_hook(struct command *commands)
 {
-	struct command *cmd_p;
+	struct command *cmd;
 	int argc;
 	const char **argv;
 	struct child_process proc;
 
-	for (argc = 0, cmd_p = cmd; cmd_p; cmd_p = cmd_p->next) {
-		if (cmd_p->error_string)
+	for (argc = 0, cmd = commands; cmd; cmd = cmd->next) {
+		if (cmd->error_string)
 			continue;
 		argc++;
 	}
@@ -464,12 +462,12 @@ static void run_update_post_hook(struct command *cmd)
 	argv = xmalloc(sizeof(*argv) * (2 + argc));
 	argv[0] = update_post_hook;
 
-	for (argc = 1, cmd_p = cmd; cmd_p; cmd_p = cmd_p->next) {
+	for (argc = 1, cmd = commands; cmd; cmd = cmd->next) {
 		char *p;
-		if (cmd_p->error_string)
+		if (cmd->error_string)
 			continue;
-		p = xmalloc(strlen(cmd_p->ref_name) + 1);
-		strcpy(p, cmd_p->ref_name);
+		p = xmalloc(strlen(cmd->ref_name) + 1);
+		strcpy(p, cmd->ref_name);
 		argv[argc] = p;
 		argc++;
 	}
@@ -488,37 +486,32 @@ static void run_update_post_hook(struct command *cmd)
 	}
 }
 
-static void execute_commands(const char *unpacker_error)
+static void execute_commands(struct command *commands, const char *unpacker_error)
 {
-	struct command *cmd = commands;
+	struct command *cmd;
 	unsigned char sha1[20];
 
 	if (unpacker_error) {
-		while (cmd) {
+		for (cmd = commands; cmd; cmd = cmd->next)
 			cmd->error_string = "n/a (unpacker error)";
-			cmd = cmd->next;
-		}
 		return;
 	}
 
-	if (run_receive_hook(pre_receive_hook)) {
-		while (cmd) {
+	if (run_receive_hook(commands, pre_receive_hook)) {
+		for (cmd = commands; cmd; cmd = cmd->next)
 			cmd->error_string = "pre-receive hook declined";
-			cmd = cmd->next;
-		}
 		return;
 	}
 
 	head_name = resolve_ref("HEAD", sha1, 0, NULL);
 
-	while (cmd) {
+	for (cmd = commands; cmd; cmd = cmd->next)
 		cmd->error_string = update(cmd);
-		cmd = cmd->next;
-	}
 }
 
-static void read_head_info(void)
+static struct command *read_head_info(void)
 {
+	struct command *commands = NULL;
 	struct command **p = &commands;
 	for (;;) {
 		static char line[1000];
@@ -557,6 +550,7 @@ static void read_head_info(void)
 		*p = cmd;
 		p = &cmd->next;
 	}
+	return commands;
 }
 
 static const char *parse_pack_header(struct pack_header *hdr)
@@ -643,7 +637,7 @@ static const char *unpack(void)
 	}
 }
 
-static void report(const char *unpack_status)
+static void report(struct command *commands, const char *unpack_status)
 {
 	struct command *cmd;
 	struct strbuf buf = STRBUF_INIT;
@@ -667,12 +661,12 @@ static void report(const char *unpack_status)
 	strbuf_release(&buf);
 }
 
-static int delete_only(struct command *cmd)
+static int delete_only(struct command *commands)
 {
-	while (cmd) {
+	struct command *cmd;
+	for (cmd = commands; cmd; cmd = cmd->next) {
 		if (!is_null_sha1(cmd->new_sha1))
 			return 0;
-		cmd = cmd->next;
 	}
 	return 1;
 }
@@ -722,6 +716,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 	int stateless_rpc = 0;
 	int i;
 	char *dir = NULL;
+	struct command *commands;
 
 	argv++;
 	for (i = 1; i < argc; i++) {
@@ -772,18 +767,17 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 	if (advertise_refs)
 		return 0;
 
-	read_head_info();
-	if (commands) {
+	if ((commands = read_head_info()) != NULL) {
 		const char *unpack_status = NULL;
 
 		if (!delete_only(commands))
 			unpack_status = unpack();
-		execute_commands(unpack_status);
+		execute_commands(commands, unpack_status);
 		if (pack_lockfile)
 			unlink_or_warn(pack_lockfile);
 		if (report_status)
-			report(unpack_status);
-		run_receive_hook(post_receive_hook);
+			report(commands, unpack_status);
+		run_receive_hook(commands, post_receive_hook);
 		run_update_post_hook(commands);
 		if (auto_gc) {
 			const char *argv_gc_auto[] = {
-- 
1.7.0.3.436.g2b878

[PATCH v2 2/2] receive-pack: detect aliased updates which can occur with symrefs

[Jay Soffian]

When pushing to a remote repo the sending side filters out aliased
updates (e.g., foo:baz bar:baz). However, it is not possible for the
sender to know if two refs are aliased on the receiving side via
symrefs. Here is one such scenario:

  $ git init origin
  $ (cd origin && touch file && git add file && git commit -a -m intial)
  $ git clone --bare origin origin.git
  $ rm -rf origin

  $ git clone origin.git client

  $ git clone --mirror client backup.git &&
  $ (cd backup.git && git remote set-head origin --auto)

  $ (cd client &&
	git remote add --mirror backup ../backup.git &&
	echo change1 > file && git commit -a -m change1 &&
	git push origin &&
	git push backup
	)

The push to backup fails with:

  Counting objects: 5, done.
  Writing objects: 100% (3/3), 244 bytes, done.
  Total 3 (delta 0), reused 0 (delta 0)
  Unpacking objects: 100% (3/3), done.
  error: Ref refs/remotes/origin/master is at ef3... but expected 262...
  remote: error: failed to lock refs/remotes/origin/master
  To ../backup.git
     262cd57..ef307ff  master -> master
     262cd57..ef307ff  origin/HEAD -> origin/HEAD
   ! [remote rejected] origin/master -> origin/master (failed to lock)
  error: failed to push some refs to '../backup.git'

The reason is that refs/remotes/origin/HEAD is a symref to
refs/remotes/origin/master, but it is not possible for the sending side
to unambiguously know this.

This commit fixes the issue by having receive-pack ignore any update to
a symref whose target is being identically updated. If a symref and its
target are being updated inconsistently, then the update for both fails
with an error message ("refusing inconsistent update...") to help
diagnose the situation.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
 builtin/receive-pack.c |   62 +++++++++++++++++++++++++++++++++++++++++++++++-
 t/t5516-fetch-push.sh  |   45 ++++++++++++++++++++++++++++++++++
 2 files changed, 106 insertions(+), 1 deletions(-)

Changes from v1 (incorporating Junio's feedback):
- Reformatted commit message; minor rewording.
- Detect situation where there is an inconsistent aliased update and
  give a better diagnostic than "failed to lock"
- Add additional test case for inconsistent update situation

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index fffb6ea..414446b 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -9,6 +9,7 @@
 #include "object.h"
 #include "remote.h"
 #include "transport.h"
+#include "string-list.h"
 
 static const char receive_pack_usage[] = "git receive-pack <git-dir>";
 
@@ -129,6 +130,7 @@ static void write_head_info(void)
 struct command {
 	struct command *next;
 	const char *error_string;
+	unsigned int skip_update;
 	unsigned char old_sha1[20];
 	unsigned char new_sha1[20];
 	char ref_name[FLEX_ARRAY]; /* more */
@@ -486,6 +488,61 @@ static void run_update_post_hook(struct command *commands)
 	}
 }
 
+static void check_aliased_update(struct command *cmd, struct string_list *list)
+{
+	struct string_list_item *item;
+	struct command *dst_cmd;
+	unsigned char sha1[20];
+	char cmd_oldh[41], cmd_newh[41], dst_oldh[41], dst_newh[41];
+	int flag;
+
+	const char *dst_name = resolve_ref(cmd->ref_name, sha1, 0, &flag);
+
+	if (!(flag & REF_ISSYMREF))
+		return;
+
+	if ((item = string_list_lookup(dst_name, list)) == NULL)
+		return;
+
+	cmd->skip_update = 1;
+
+	dst_cmd = (struct command *) item->util;
+
+	if (!hashcmp(cmd->old_sha1, dst_cmd->old_sha1) &&
+	    !hashcmp(cmd->new_sha1, dst_cmd->new_sha1))
+		return;
+
+	strcpy(cmd_oldh, find_unique_abbrev(cmd->old_sha1, DEFAULT_ABBREV));
+	strcat(cmd_newh, find_unique_abbrev(cmd->new_sha1, DEFAULT_ABBREV));
+	strcpy(dst_oldh, find_unique_abbrev(dst_cmd->old_sha1, DEFAULT_ABBREV));
+	strcat(dst_newh, find_unique_abbrev(dst_cmd->new_sha1, DEFAULT_ABBREV));
+	rp_error("refusing inconsistent update between symref '%s' (%s..%s) and"
+		 " its target '%s' (%s..%s)",
+		 cmd->ref_name, cmd_oldh, cmd_newh,
+		 dst_cmd->ref_name, dst_oldh, dst_newh);
+
+	cmd->error_string = dst_cmd->error_string =
+		"inconsistent aliased update";
+}
+
+static void check_aliased_updates(struct command *commands)
+{
+	struct command *cmd;
+	struct string_list ref_list = { NULL, 0, 0, 0 };
+
+	for (cmd = commands; cmd; cmd = cmd->next) {
+		struct string_list_item *item =
+			string_list_append(cmd->ref_name, &ref_list);
+		item->util = (void *)cmd;
+	}
+	sort_string_list(&ref_list);
+
+	for (cmd = commands; cmd; cmd = cmd->next)
+		check_aliased_update(cmd, &ref_list);
+
+	string_list_clear(&ref_list, 0);
+}
+
 static void execute_commands(struct command *commands, const char *unpacker_error)
 {
 	struct command *cmd;
@@ -503,9 +560,11 @@ static void execute_commands(struct command *commands, const char *unpacker_erro
 		return;
 	}
 
+	check_aliased_updates(commands);
+
 	head_name = resolve_ref("HEAD", sha1, 0, NULL);
 
-	for (cmd = commands; cmd; cmd = cmd->next)
+	for (cmd = commands; cmd && !cmd->skip_update; cmd = cmd->next)
 		cmd->error_string = update(cmd);
 }
 
@@ -545,6 +604,7 @@ static struct command *read_head_info(void)
 		hashcpy(cmd->old_sha1, old_sha1);
 		hashcpy(cmd->new_sha1, new_sha1);
 		memcpy(cmd->ref_name, line + 82, len - 81);
+		cmd->skip_update = 0;
 		cmd->error_string = NULL;
 		cmd->next = NULL;
 		*p = cmd;
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 2de98e6..bc3f24f 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -660,6 +660,51 @@ test_expect_success 'push with branches containing #' '
 	git checkout master
 '
 
+test_expect_success 'push into aliased refs (consistent)' '
+	mk_test heads/master &&
+	mk_child child1 &&
+	mk_child child2 &&
+	(cd child1 &&
+		git branch foo &&
+		git symbolic-ref refs/heads/bar refs/heads/foo
+		git config receive.denyCurrentBranch false
+	) &&
+	(cd child2 &&
+		: >path2 &&
+		git add path2 &&
+		test_tick &&
+		git commit -a -m child2 &&
+		git branch foo &&
+		git branch bar &&
+		git push ../child1 foo bar
+	)
+'
+
+test_expect_success 'push into aliased refs (inconsistent)' '
+	mk_test heads/master &&
+	mk_child child1 &&
+	mk_child child2 &&
+	(cd child1 &&
+		git branch foo &&
+		git symbolic-ref refs/heads/bar refs/heads/foo
+		git config receive.denyCurrentBranch false
+	) &&
+	(cd child2 &&
+		: >path2 &&
+		git add path2 &&
+		test_tick &&
+		git commit -a -m child2 &&
+		git branch foo &&
+		: >path3 &&
+		git add path3 &&
+		test_tick &&
+		git commit -a -m child2 &&
+		git branch bar &&
+		test_must_fail git push ../child1 foo bar 2>stderr &&
+		grep "refusing inconsistent update" stderr
+	)
+'
+
 test_expect_success 'push --porcelain' '
 	mk_empty &&
 	echo >.git/foo  "To testrepo" &&
-- 
1.7.0.3.436.g2b878

Re: [PATCH v2 2/2] receive-pack: detect aliased updates which can occur with symrefs

[Jay Soffian]

On Mon, Apr 19, 2010 at 12:25 PM, Jay Soffian <jaysoffian@gmail.com> wrote:
> When pushing to a remote repo the sending side filters out aliased
> updates (e.g., foo:baz bar:baz). However, it is not possible for the
> sender to know if two refs are aliased on the receiving side via
> symrefs. Here is one such scenario:

Don't receive this. There's a logic error in it. I'll send a follow up shortly.

j.

[PATCH v2 2/2] receive-pack: detect aliased updates which can occur with symrefs

[Jay Soffian]

When pushing to a remote repo the sending side filters out aliased
updates (e.g., foo:baz bar:baz). However, it is not possible for the
sender to know if two refs are aliased on the receiving side via
symrefs. Here is one such scenario:

  $ git init origin
  $ (cd origin && touch file && git add file && git commit -a -m intial)
  $ git clone --bare origin origin.git
  $ rm -rf origin

  $ git clone origin.git client

  $ git clone --mirror client backup.git &&
  $ (cd backup.git && git remote set-head origin --auto)

  $ (cd client &&
	git remote add --mirror backup ../backup.git &&
	echo change1 > file && git commit -a -m change1 &&
	git push origin &&
	git push backup
	)

The push to backup fails with:

  Counting objects: 5, done.
  Writing objects: 100% (3/3), 244 bytes, done.
  Total 3 (delta 0), reused 0 (delta 0)
  Unpacking objects: 100% (3/3), done.
  error: Ref refs/remotes/origin/master is at ef3... but expected 262...
  remote: error: failed to lock refs/remotes/origin/master
  To ../backup.git
     262cd57..ef307ff  master -> master
     262cd57..ef307ff  origin/HEAD -> origin/HEAD
   ! [remote rejected] origin/master -> origin/master (failed to lock)
  error: failed to push some refs to '../backup.git'

The reason is that refs/remotes/origin/HEAD is a symref to
refs/remotes/origin/master, but it is not possible for the sending side
to unambiguously know this.

This commit fixes the issue by having receive-pack ignore any update to
a symref whose target is being identically updated. If a symref and its
target are being updated inconsistently, then the update for both fails
with an error message ("refusing inconsistent update...") to help
diagnose the situation.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
 builtin/receive-pack.c |   64 +++++++++++++++++++++++++++++++++++++++++++++++-
 t/t5516-fetch-push.sh  |   45 +++++++++++++++++++++++++++++++++
 2 files changed, 108 insertions(+), 1 deletions(-)

Changes from v1 (incorporating Junio's feedback):
- Reformatted commit message; minor rewording.
- Detect situation where there is an inconsistent aliased update and
  give a better diagnostic than "failed to lock"
- Add additional test case for inconsistent update situation

[
This one is correct, the interdiff from
<1271694343-31876-2-git-send-email-jaysoffian@gmail.com> is

  diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
  index 414446b..7b269d2 100644
  --- a/builtin/receive-pack.c
  +++ b/builtin/receive-pack.c
  @@ -512,6 +512,8 @@ static void check_aliased_update(struct command *cmd, struct string_list *list)
   	    !hashcmp(cmd->new_sha1, dst_cmd->new_sha1))
   		return;
   
  +	dst_cmd->skip_update = 1;
  +
   	strcpy(cmd_oldh, find_unique_abbrev(cmd->old_sha1, DEFAULT_ABBREV));
   	strcat(cmd_newh, find_unique_abbrev(cmd->new_sha1, DEFAULT_ABBREV));
   	strcpy(dst_oldh, find_unique_abbrev(dst_cmd->old_sha1, DEFAULT_ABBREV));
  )
]
  
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index fffb6ea..7b269d2 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -9,6 +9,7 @@
 #include "object.h"
 #include "remote.h"
 #include "transport.h"
+#include "string-list.h"
 
 static const char receive_pack_usage[] = "git receive-pack <git-dir>";
 
@@ -129,6 +130,7 @@ static void write_head_info(void)
 struct command {
 	struct command *next;
 	const char *error_string;
+	unsigned int skip_update;
 	unsigned char old_sha1[20];
 	unsigned char new_sha1[20];
 	char ref_name[FLEX_ARRAY]; /* more */
@@ -486,6 +488,63 @@ static void run_update_post_hook(struct command *commands)
 	}
 }
 
+static void check_aliased_update(struct command *cmd, struct string_list *list)
+{
+	struct string_list_item *item;
+	struct command *dst_cmd;
+	unsigned char sha1[20];
+	char cmd_oldh[41], cmd_newh[41], dst_oldh[41], dst_newh[41];
+	int flag;
+
+	const char *dst_name = resolve_ref(cmd->ref_name, sha1, 0, &flag);
+
+	if (!(flag & REF_ISSYMREF))
+		return;
+
+	if ((item = string_list_lookup(dst_name, list)) == NULL)
+		return;
+
+	cmd->skip_update = 1;
+
+	dst_cmd = (struct command *) item->util;
+
+	if (!hashcmp(cmd->old_sha1, dst_cmd->old_sha1) &&
+	    !hashcmp(cmd->new_sha1, dst_cmd->new_sha1))
+		return;
+
+	dst_cmd->skip_update = 1;
+
+	strcpy(cmd_oldh, find_unique_abbrev(cmd->old_sha1, DEFAULT_ABBREV));
+	strcat(cmd_newh, find_unique_abbrev(cmd->new_sha1, DEFAULT_ABBREV));
+	strcpy(dst_oldh, find_unique_abbrev(dst_cmd->old_sha1, DEFAULT_ABBREV));
+	strcat(dst_newh, find_unique_abbrev(dst_cmd->new_sha1, DEFAULT_ABBREV));
+	rp_error("refusing inconsistent update between symref '%s' (%s..%s) and"
+		 " its target '%s' (%s..%s)",
+		 cmd->ref_name, cmd_oldh, cmd_newh,
+		 dst_cmd->ref_name, dst_oldh, dst_newh);
+
+	cmd->error_string = dst_cmd->error_string =
+		"inconsistent aliased update";
+}
+
+static void check_aliased_updates(struct command *commands)
+{
+	struct command *cmd;
+	struct string_list ref_list = { NULL, 0, 0, 0 };
+
+	for (cmd = commands; cmd; cmd = cmd->next) {
+		struct string_list_item *item =
+			string_list_append(cmd->ref_name, &ref_list);
+		item->util = (void *)cmd;
+	}
+	sort_string_list(&ref_list);
+
+	for (cmd = commands; cmd; cmd = cmd->next)
+		check_aliased_update(cmd, &ref_list);
+
+	string_list_clear(&ref_list, 0);
+}
+
 static void execute_commands(struct command *commands, const char *unpacker_error)
 {
 	struct command *cmd;
@@ -503,9 +562,11 @@ static void execute_commands(struct command *commands, const char *unpacker_erro
 		return;
 	}
 
+	check_aliased_updates(commands);
+
 	head_name = resolve_ref("HEAD", sha1, 0, NULL);
 
-	for (cmd = commands; cmd; cmd = cmd->next)
+	for (cmd = commands; cmd && !cmd->skip_update; cmd = cmd->next)
 		cmd->error_string = update(cmd);
 }
 
@@ -545,6 +606,7 @@ static struct command *read_head_info(void)
 		hashcpy(cmd->old_sha1, old_sha1);
 		hashcpy(cmd->new_sha1, new_sha1);
 		memcpy(cmd->ref_name, line + 82, len - 81);
+		cmd->skip_update = 0;
 		cmd->error_string = NULL;
 		cmd->next = NULL;
 		*p = cmd;
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 2de98e6..bc3f24f 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -660,6 +660,51 @@ test_expect_success 'push with branches containing #' '
 	git checkout master
 '
 
+test_expect_success 'push into aliased refs (consistent)' '
+	mk_test heads/master &&
+	mk_child child1 &&
+	mk_child child2 &&
+	(cd child1 &&
+		git branch foo &&
+		git symbolic-ref refs/heads/bar refs/heads/foo
+		git config receive.denyCurrentBranch false
+	) &&
+	(cd child2 &&
+		: >path2 &&
+		git add path2 &&
+		test_tick &&
+		git commit -a -m child2 &&
+		git branch foo &&
+		git branch bar &&
+		git push ../child1 foo bar
+	)
+'
+
+test_expect_success 'push into aliased refs (inconsistent)' '
+	mk_test heads/master &&
+	mk_child child1 &&
+	mk_child child2 &&
+	(cd child1 &&
+		git branch foo &&
+		git symbolic-ref refs/heads/bar refs/heads/foo
+		git config receive.denyCurrentBranch false
+	) &&
+	(cd child2 &&
+		: >path2 &&
+		git add path2 &&
+		test_tick &&
+		git commit -a -m child2 &&
+		git branch foo &&
+		: >path3 &&
+		git add path3 &&
+		test_tick &&
+		git commit -a -m child2 &&
+		git branch bar &&
+		test_must_fail git push ../child1 foo bar 2>stderr &&
+		grep "refusing inconsistent update" stderr
+	)
+'
+
 test_expect_success 'push --porcelain' '
 	mk_empty &&
 	echo >.git/foo  "To testrepo" &&
-- 
1.7.0.3.436.g2b878

Re: [PATCH v2 2/2] receive-pack: detect aliased updates which can occur with symrefs

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

> @@ -503,9 +562,11 @@ static void execute_commands(struct command *commands, const char *unpacker_erro
>  		return;
>  	}
>  
> +	check_aliased_updates(commands);
> +
>  	head_name = resolve_ref("HEAD", sha1, 0, NULL);
>  
> -	for (cmd = commands; cmd; cmd = cmd->next)
> +	for (cmd = commands; cmd && !cmd->skip_update; cmd = cmd->next)
>  		cmd->error_string = update(cmd);
>  }

Do you really mean to have "skip-update" check as the loop termination
condition like this (i.e. "upon seeing any skip-update, abandon the rest
of the update queue"), or is this a typo of "skip this entry but keep
going, processing the rest" that should have been a separate "if" in the
body of the loop?

> @@ -545,6 +606,7 @@ static struct command *read_head_info(void)
>  		hashcpy(cmd->old_sha1, old_sha1);
>  		hashcpy(cmd->new_sha1, new_sha1);
>  		memcpy(cmd->ref_name, line + 82, len - 81);
> +		cmd->skip_update = 0;
>  		cmd->error_string = NULL;
>  		cmd->next = NULL;

It would make sense to do xcalloc(nmemb, size) of one member of that
length to allocate cmd at this point, instead of adding yet another
assignment like this.

It also would help me a slight bit if you compared what has been queued
with what you sent to catch minor differences between my expectation from
this series and what you have (e.g. I'd like to keep this as a fix that is
back-mergeable to 'maint' and also I have already done some style fixes to
the test).

Thanks.

Re: [PATCH v2 2/2] receive-pack: detect aliased updates which can occur with symrefs

[Jay Soffian]

On Mon, Apr 19, 2010 at 4:39 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> -     for (cmd = commands; cmd; cmd = cmd->next)
>> +     for (cmd = commands; cmd && !cmd->skip_update; cmd = cmd->next)
>>               cmd->error_string = update(cmd);
>>  }
>
> Do you really mean to have "skip-update" check as the loop termination
> condition like this (i.e. "upon seeing any skip-update, abandon the rest
> of the update queue"), or is this a typo of "skip this entry but keep
> going, processing the rest" that should have been a separate "if" in the
> body of the loop?

Oh wow, the perils of coding before coffee. I wish I could call it a
typo, it's a thinko which is much worse.

>> @@ -545,6 +606,7 @@ static struct command *read_head_info(void)
>>               hashcpy(cmd->old_sha1, old_sha1);
>>               hashcpy(cmd->new_sha1, new_sha1);
>>               memcpy(cmd->ref_name, line + 82, len - 81);
>> +             cmd->skip_update = 0;
>>               cmd->error_string = NULL;
>>               cmd->next = NULL;
>
> It would make sense to do xcalloc(nmemb, size) of one member of that
> length to allocate cmd at this point, instead of adding yet another
> assignment like this.

Okay.

> It also would help me a slight bit if you compared what has been queued
> with what you sent to catch minor differences between my expectation from
> this series and what you have (e.g. I'd like to keep this as a fix that is
> back-mergeable to 'maint' and also I have already done some style fixes to
> the test).

I didn't realize you'd queued it before I sent out v2. But I'm not
sure what you're asking for beyond what I already said in the email:

- Reformatted commit message; minor rewording.
- Detect situation where there is an inconsistent aliased update and
 give a better diagnostic than "failed to lock"
- Add additional test case for inconsistent update situation

j.

[PATCH v3 0/3] js/maint-receive-pack-symref-alias

[Jay Soffian]

- Incorporated Junio's feedback from v1 and v2.
- Rebuilt on-top of f78683f, which is where v1 was applied in pu.
- Added an additional patch to the series to reformat the tests in
  t5516-fetch-push.sh to use a consistent style, per Junio's tweaks to the
  test I wrote for v1.

Jay Soffian (3):
  receive-pack: switch global variable 'commands' to a parameter
  t5516-fetch-push.sh: style cleanup
  receive-pack: detect aliased updates which can occur with symrefs

 builtin-receive-pack.c |  128 ++++++++++++++++++++++++++++++++++--------------
 t/t5516-fetch-push.sh  |  118 +++++++++++++++++++++++++++++++++++---------
 2 files changed, 186 insertions(+), 60 deletions(-)

interdiff from what's in pu:
 builtin-receive-pack.c |   76 +++++++++++++++++++++++++------------
 t/t5516-fetch-push.sh  |   98 ++++++++++++++++++++++++++++++++++++------------
 2 files changed, 126 insertions(+), 48 deletions(-)

diff --git a/builtin-receive-pack.c b/builtin-receive-pack.c
index a2e3bc8..bb34757 100644
--- a/builtin-receive-pack.c
+++ b/builtin-receive-pack.c
@@ -130,6 +130,7 @@ static void write_head_info(void)
 struct command {
 	struct command *next;
 	const char *error_string;
+	unsigned int skip_update;
 	unsigned char old_sha1[20];
 	unsigned char new_sha1[20];
 	char ref_name[FLEX_ARRAY]; /* more */
@@ -487,30 +488,67 @@ static void run_update_post_hook(struct command *commands)
 	}
 }
 
-static int aliased_ref(struct command *cmd, struct string_list *list)
+static void check_aliased_update(struct command *cmd, struct string_list *list)
 {
 	struct string_list_item *item;
+	struct command *dst_cmd;
 	unsigned char sha1[20];
+	char cmd_oldh[41], cmd_newh[41], dst_oldh[41], dst_newh[41];
 	int flag;
 
 	const char *dst_name = resolve_ref(cmd->ref_name, sha1, 0, &flag);
 
 	if (!(flag & REF_ISSYMREF))
-		return 0;
+		return;
+
+	if ((item = string_list_lookup(dst_name, list)) == NULL)
+		return;
+
+	cmd->skip_update = 1;
+
+	dst_cmd = (struct command *) item->util;
+
+	if (!hashcmp(cmd->old_sha1, dst_cmd->old_sha1) &&
+	    !hashcmp(cmd->new_sha1, dst_cmd->new_sha1))
+		return;
+
+	dst_cmd->skip_update = 1;
+
+	strcpy(cmd_oldh, find_unique_abbrev(cmd->old_sha1, DEFAULT_ABBREV));
+	strcat(cmd_newh, find_unique_abbrev(cmd->new_sha1, DEFAULT_ABBREV));
+	strcpy(dst_oldh, find_unique_abbrev(dst_cmd->old_sha1, DEFAULT_ABBREV));
+	strcat(dst_newh, find_unique_abbrev(dst_cmd->new_sha1, DEFAULT_ABBREV));
+	rp_error("refusing inconsistent update between symref '%s' (%s..%s) and"
+		 " its target '%s' (%s..%s)",
+		 cmd->ref_name, cmd_oldh, cmd_newh,
+		 dst_cmd->ref_name, dst_oldh, dst_newh);
 
-	if ((item = string_list_lookup(dst_name, list)) != NULL) {
-		struct command *other_cmd = (struct command *) item->util;
-		return (!(hashcmp(cmd->old_sha1, other_cmd->old_sha1) &&
-			hashcmp(cmd->new_sha1, other_cmd->new_sha1)));
+	cmd->error_string = dst_cmd->error_string =
+		"inconsistent aliased update";
+}
+
+static void check_aliased_updates(struct command *commands)
+{
+	struct command *cmd;
+	struct string_list ref_list = { NULL, 0, 0, 0 };
+
+	for (cmd = commands; cmd; cmd = cmd->next) {
+		struct string_list_item *item =
+			string_list_append(cmd->ref_name, &ref_list);
+		item->util = (void *)cmd;
 	}
-	return 0;
+	sort_string_list(&ref_list);
+
+	for (cmd = commands; cmd; cmd = cmd->next)
+		check_aliased_update(cmd, &ref_list);
+
+	string_list_clear(&ref_list, 0);
 }
 
 static void execute_commands(struct command *commands, const char *unpacker_error)
 {
 	struct command *cmd;
 	unsigned char sha1[20];
-	struct string_list ref_list = { NULL, 0, 0, 0 };
 
 	if (unpacker_error) {
 		for (cmd = commands; cmd; cmd = cmd->next)
@@ -524,27 +562,19 @@ static void execute_commands(struct command *commands, const char *unpacker_erro
 		return;
 	}
 
-	head_name = resolve_ref("HEAD", sha1, 0, NULL);
+	check_aliased_updates(commands);
 
-	for (cmd = commands; cmd; cmd = cmd->next) {
-		struct string_list_item *item =
-			string_list_append(cmd->ref_name, &ref_list);
-		item->util = (void *)cmd;
-	}
-	sort_string_list(&ref_list);
+	head_name = resolve_ref("HEAD", sha1, 0, NULL);
 
-	for (cmd = commands; cmd; cmd = cmd->next) {
-		if (!aliased_ref(cmd, &ref_list))
+	for (cmd = commands; cmd; cmd = cmd->next)
+		if (!cmd->skip_update)
 			cmd->error_string = update(cmd);
-	}
-	string_list_clear(&ref_list, 0);
 }
 
 static struct command *read_head_info(void)
 {
 	struct command *commands = NULL;
 	struct command **p = &commands;
-
 	for (;;) {
 		static char line[1000];
 		unsigned char old_sha1[20], new_sha1[20];
@@ -573,12 +603,10 @@ static struct command *read_head_info(void)
 			if (strstr(refname + reflen + 1, "side-band-64k"))
 				use_sideband = LARGE_PACKET_MAX;
 		}
-		cmd = xmalloc(sizeof(struct command) + len - 80);
+		cmd = xcalloc(1, sizeof(struct command) + len - 80);
 		hashcpy(cmd->old_sha1, old_sha1);
 		hashcpy(cmd->new_sha1, new_sha1);
 		memcpy(cmd->ref_name, line + 82, len - 81);
-		cmd->error_string = NULL;
-		cmd->next = NULL;
 		*p = cmd;
 		p = &cmd->next;
 	}
@@ -671,8 +699,8 @@ static const char *unpack(void)
 
 static void report(struct command *commands, const char *unpack_status)
 {
-	struct strbuf buf = STRBUF_INIT;
 	struct command *cmd;
+	struct strbuf buf = STRBUF_INIT;
 
 	packet_buf_write(&buf, "unpack %s\n",
 			 unpack_status ? unpack_status : "ok");
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 350e734..f0813e0 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -64,13 +64,13 @@ check_push_result () {
 
 test_expect_success setup '
 
-	: >path1 &&
+	>path1 &&
 	git add path1 &&
 	test_tick &&
 	git commit -a -m repo &&
 	the_first_commit=$(git show-ref -s --verify refs/heads/master) &&
 
-	: >path2 &&
+	>path2 &&
 	git add path2 &&
 	test_tick &&
 	git commit -a -m second &&
@@ -483,8 +483,10 @@ git config --remove-section remote.there
 test_expect_success 'push with dry-run' '
 
 	mk_test heads/master &&
-	(cd testrepo &&
-	 old_commit=$(git show-ref -s --verify refs/heads/master)) &&
+	(
+		cd testrepo &&
+		old_commit=$(git show-ref -s --verify refs/heads/master)
+	) &&
 	git push --dry-run testrepo &&
 	check_push_result $old_commit heads/master
 '
@@ -493,10 +495,13 @@ test_expect_success 'push updates local refs' '
 
 	mk_test heads/master &&
 	mk_child child &&
-	(cd child &&
+	(
+		cd child &&
 		git pull .. master &&
 		git push &&
-	test $(git rev-parse master) = $(git rev-parse remotes/origin/master))
+		test $(git rev-parse master) = \
+			$(git rev-parse remotes/origin/master)
+	)
 
 '
 
@@ -506,10 +511,13 @@ test_expect_success 'push updates up-to-date local refs' '
 	mk_child child1 &&
 	mk_child child2 &&
 	(cd child1 && git pull .. master && git push) &&
-	(cd child2 &&
+	(
+		cd child2 &&
 		git pull ../child1 master &&
 		git push &&
-	test $(git rev-parse master) = $(git rev-parse remotes/origin/master))
+		test $(git rev-parse master) = \
+			$(git rev-parse remotes/origin/master)
+	)
 
 '
 
@@ -517,9 +525,11 @@ test_expect_success 'push preserves up-to-date packed refs' '
 
 	mk_test heads/master &&
 	mk_child child &&
-	(cd child &&
+	(
+		cd child &&
 		git push &&
-	! test -f .git/refs/remotes/origin/master)
+		! test -f .git/refs/remotes/origin/master
+	)
 
 '
 
@@ -530,11 +540,13 @@ test_expect_success 'push does not update local refs on failure' '
 	mkdir testrepo/.git/hooks &&
 	echo exit 1 >testrepo/.git/hooks/pre-receive &&
 	chmod +x testrepo/.git/hooks/pre-receive &&
-	(cd child &&
+	(
+		cd child &&
 		git pull .. master
 		test_must_fail git push &&
 		test $(git rev-parse master) != \
-			$(git rev-parse remotes/origin/master))
+			$(git rev-parse remotes/origin/master)
+	)
 
 '
 
@@ -575,34 +587,41 @@ test_expect_success 'push --delete refuses src:dest refspecs' '
 
 test_expect_success 'warn on push to HEAD of non-bare repository' '
 	mk_test heads/master
-	(cd testrepo &&
+	(
+		cd testrepo &&
 		git checkout master &&
-		git config receive.denyCurrentBranch warn) &&
+		git config receive.denyCurrentBranch warn
+	) &&
 	git push testrepo master 2>stderr &&
 	grep "warning: updating the current branch" stderr
 '
 
 test_expect_success 'deny push to HEAD of non-bare repository' '
 	mk_test heads/master
-	(cd testrepo &&
+	(
+		cd testrepo &&
 		git checkout master &&
-		git config receive.denyCurrentBranch true) &&
+		git config receive.denyCurrentBranch true
+	) &&
 	test_must_fail git push testrepo master
 '
 
 test_expect_success 'allow push to HEAD of bare repository (bare)' '
 	mk_test heads/master
-	(cd testrepo &&
+	(
+		cd testrepo &&
 		git checkout master &&
 		git config receive.denyCurrentBranch true &&
-		git config core.bare true) &&
+		git config core.bare true
+	) &&
 	git push testrepo master 2>stderr &&
 	! grep "warning: updating the current branch" stderr
 '
 
 test_expect_success 'allow push to HEAD of non-bare repository (config)' '
 	mk_test heads/master
-	(cd testrepo &&
+	(
+		cd testrepo &&
 		git checkout master &&
 		git config receive.denyCurrentBranch false
 	) &&
@@ -615,7 +634,8 @@ test_expect_success 'fetch with branches' '
 	git branch second $the_first_commit &&
 	git checkout second &&
 	echo ".." > testrepo/.git/branches/branch1 &&
-	(cd testrepo &&
+	(
+		cd testrepo &&
 		git fetch branch1 &&
 		r=$(git show-ref -s --verify refs/heads/branch1) &&
 		test "z$r" = "z$the_commit" &&
@@ -627,7 +647,8 @@ test_expect_success 'fetch with branches' '
 test_expect_success 'fetch with branches containing #' '
 	mk_empty &&
 	echo "..#second" > testrepo/.git/branches/branch2 &&
-	(cd testrepo &&
+	(
+		cd testrepo &&
 		git fetch branch2 &&
 		r=$(git show-ref -s --verify refs/heads/branch2) &&
 		test "z$r" = "z$the_first_commit" &&
@@ -641,7 +662,8 @@ test_expect_success 'push with branches' '
 	git checkout second &&
 	echo "testrepo" > .git/branches/branch1 &&
 	git push branch1 &&
-	(cd testrepo &&
+	(
+		cd testrepo &&
 		r=$(git show-ref -s --verify refs/heads/master) &&
 		test "z$r" = "z$the_first_commit" &&
 		test 1 = $(git for-each-ref refs/heads | wc -l)
@@ -652,7 +674,8 @@ test_expect_success 'push with branches containing #' '
 	mk_empty &&
 	echo "testrepo#branch3" > .git/branches/branch2 &&
 	git push branch2 &&
-	(cd testrepo &&
+	(
+		cd testrepo &&
 		r=$(git show-ref -s --verify refs/heads/branch3) &&
 		test "z$r" = "z$the_first_commit" &&
 		test 1 = $(git for-each-ref refs/heads | wc -l)
@@ -660,7 +683,7 @@ test_expect_success 'push with branches containing #' '
 	git checkout master
 '
 
-test_expect_success 'push into aliased refs' '
+test_expect_success 'push into aliased refs (consistent)' '
 	mk_test heads/master &&
 	mk_child child1 &&
 	mk_child child2 &&
@@ -682,4 +705,31 @@ test_expect_success 'push into aliased refs' '
 	)
 '
 
+test_expect_success 'push into aliased refs (inconsistent)' '
+	mk_test heads/master &&
+	mk_child child1 &&
+	mk_child child2 &&
+	(
+		cd child1 &&
+		git branch foo &&
+		git symbolic-ref refs/heads/bar refs/heads/foo
+		git config receive.denyCurrentBranch false
+	) &&
+	(
+		cd child2 &&
+		>path2 &&
+		git add path2 &&
+		test_tick &&
+		git commit -a -m child2 &&
+		git branch foo &&
+		>path3 &&
+		git add path3 &&
+		test_tick &&
+		git commit -a -m child2 &&
+		git branch bar &&
+		test_must_fail git push ../child1 foo bar 2>stderr &&
+		grep "refusing inconsistent update" stderr
+	)
+'
+
 test_done

[PATCH v3 1/3] receive-pack: switch global variable 'commands' to a parameter

[Jay Soffian]

Receive-pack is inconsistent in its usage of the 'commands'
variable; though it is setup as a global and accessed that way by
execute_commands(), report(), and run_receive_hook(), it is also
passed as a parameter to delete_only() and run_update_post_hook().

For consistency, make it local to cmd_receive_pack and pass it as a
parameter. As long as we're cleaning up, also make our use of the
names 'commands' and 'cmd' consistent.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
 builtin-receive-pack.c |   60 +++++++++++++++++++++--------------------------
 1 files changed, 27 insertions(+), 33 deletions(-)

diff --git a/builtin-receive-pack.c b/builtin-receive-pack.c
index 0559fcc..fffb6ea 100644
--- a/builtin-receive-pack.c
+++ b/builtin-receive-pack.c
@@ -134,8 +134,6 @@ struct command {
 	char ref_name[FLEX_ARRAY]; /* more */
 };
 
-static struct command *commands;
-
 static const char pre_receive_hook[] = "hooks/pre-receive";
 static const char post_receive_hook[] = "hooks/post-receive";
 
@@ -188,7 +186,7 @@ static int copy_to_sideband(int in, int out, void *arg)
 	return 0;
 }
 
-static int run_receive_hook(const char *hook_name)
+static int run_receive_hook(struct command *commands, const char *hook_name)
 {
 	static char buf[sizeof(commands->old_sha1) * 2 + PATH_MAX + 4];
 	struct command *cmd;
@@ -447,15 +445,15 @@ static const char *update(struct command *cmd)
 
 static char update_post_hook[] = "hooks/post-update";
 
-static void run_update_post_hook(struct command *cmd)
+static void run_update_post_hook(struct command *commands)
 {
-	struct command *cmd_p;
+	struct command *cmd;
 	int argc;
 	const char **argv;
 	struct child_process proc;
 
-	for (argc = 0, cmd_p = cmd; cmd_p; cmd_p = cmd_p->next) {
-		if (cmd_p->error_string)
+	for (argc = 0, cmd = commands; cmd; cmd = cmd->next) {
+		if (cmd->error_string)
 			continue;
 		argc++;
 	}
@@ -464,12 +462,12 @@ static void run_update_post_hook(struct command *cmd)
 	argv = xmalloc(sizeof(*argv) * (2 + argc));
 	argv[0] = update_post_hook;
 
-	for (argc = 1, cmd_p = cmd; cmd_p; cmd_p = cmd_p->next) {
+	for (argc = 1, cmd = commands; cmd; cmd = cmd->next) {
 		char *p;
-		if (cmd_p->error_string)
+		if (cmd->error_string)
 			continue;
-		p = xmalloc(strlen(cmd_p->ref_name) + 1);
-		strcpy(p, cmd_p->ref_name);
+		p = xmalloc(strlen(cmd->ref_name) + 1);
+		strcpy(p, cmd->ref_name);
 		argv[argc] = p;
 		argc++;
 	}
@@ -488,37 +486,32 @@ static void run_update_post_hook(struct command *cmd)
 	}
 }
 
-static void execute_commands(const char *unpacker_error)
+static void execute_commands(struct command *commands, const char *unpacker_error)
 {
-	struct command *cmd = commands;
+	struct command *cmd;
 	unsigned char sha1[20];
 
 	if (unpacker_error) {
-		while (cmd) {
+		for (cmd = commands; cmd; cmd = cmd->next)
 			cmd->error_string = "n/a (unpacker error)";
-			cmd = cmd->next;
-		}
 		return;
 	}
 
-	if (run_receive_hook(pre_receive_hook)) {
-		while (cmd) {
+	if (run_receive_hook(commands, pre_receive_hook)) {
+		for (cmd = commands; cmd; cmd = cmd->next)
 			cmd->error_string = "pre-receive hook declined";
-			cmd = cmd->next;
-		}
 		return;
 	}
 
 	head_name = resolve_ref("HEAD", sha1, 0, NULL);
 
-	while (cmd) {
+	for (cmd = commands; cmd; cmd = cmd->next)
 		cmd->error_string = update(cmd);
-		cmd = cmd->next;
-	}
 }
 
-static void read_head_info(void)
+static struct command *read_head_info(void)
 {
+	struct command *commands = NULL;
 	struct command **p = &commands;
 	for (;;) {
 		static char line[1000];
@@ -557,6 +550,7 @@ static void read_head_info(void)
 		*p = cmd;
 		p = &cmd->next;
 	}
+	return commands;
 }
 
 static const char *parse_pack_header(struct pack_header *hdr)
@@ -643,7 +637,7 @@ static const char *unpack(void)
 	}
 }
 
-static void report(const char *unpack_status)
+static void report(struct command *commands, const char *unpack_status)
 {
 	struct command *cmd;
 	struct strbuf buf = STRBUF_INIT;
@@ -667,12 +661,12 @@ static void report(const char *unpack_status)
 	strbuf_release(&buf);
 }
 
-static int delete_only(struct command *cmd)
+static int delete_only(struct command *commands)
 {
-	while (cmd) {
+	struct command *cmd;
+	for (cmd = commands; cmd; cmd = cmd->next) {
 		if (!is_null_sha1(cmd->new_sha1))
 			return 0;
-		cmd = cmd->next;
 	}
 	return 1;
 }
@@ -722,6 +716,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 	int stateless_rpc = 0;
 	int i;
 	char *dir = NULL;
+	struct command *commands;
 
 	argv++;
 	for (i = 1; i < argc; i++) {
@@ -772,18 +767,17 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 	if (advertise_refs)
 		return 0;
 
-	read_head_info();
-	if (commands) {
+	if ((commands = read_head_info()) != NULL) {
 		const char *unpack_status = NULL;
 
 		if (!delete_only(commands))
 			unpack_status = unpack();
-		execute_commands(unpack_status);
+		execute_commands(commands, unpack_status);
 		if (pack_lockfile)
 			unlink_or_warn(pack_lockfile);
 		if (report_status)
-			report(unpack_status);
-		run_receive_hook(post_receive_hook);
+			report(commands, unpack_status);
+		run_receive_hook(commands, post_receive_hook);
 		run_update_post_hook(commands);
 		if (auto_gc) {
 			const char *argv_gc_auto[] = {
-- 
1.7.0.3.436.g2b878

[PATCH v3 2/3] t5516-fetch-push.sh: style cleanup

[Jay Soffian]

Cleanup t5516-fetch-push.sh to use prevailing test script style

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
 t/t5516-fetch-push.sh |   69 ++++++++++++++++++++++++++++++++----------------
 1 files changed, 46 insertions(+), 23 deletions(-)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 0f04b2e..3148789 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -64,13 +64,13 @@ check_push_result () {
 
 test_expect_success setup '
 
-	: >path1 &&
+	>path1 &&
 	git add path1 &&
 	test_tick &&
 	git commit -a -m repo &&
 	the_first_commit=$(git show-ref -s --verify refs/heads/master) &&
 
-	: >path2 &&
+	>path2 &&
 	git add path2 &&
 	test_tick &&
 	git commit -a -m second &&
@@ -483,8 +483,10 @@ git config --remove-section remote.there
 test_expect_success 'push with dry-run' '
 
 	mk_test heads/master &&
-	(cd testrepo &&
-	 old_commit=$(git show-ref -s --verify refs/heads/master)) &&
+	(
+		cd testrepo &&
+		old_commit=$(git show-ref -s --verify refs/heads/master)
+	) &&
 	git push --dry-run testrepo &&
 	check_push_result $old_commit heads/master
 '
@@ -493,10 +495,13 @@ test_expect_success 'push updates local refs' '
 
 	mk_test heads/master &&
 	mk_child child &&
-	(cd child &&
+	(
+		cd child &&
 		git pull .. master &&
 		git push &&
-	test $(git rev-parse master) = $(git rev-parse remotes/origin/master))
+		test $(git rev-parse master) = \
+			$(git rev-parse remotes/origin/master)
+	)
 
 '
 
@@ -506,10 +511,13 @@ test_expect_success 'push updates up-to-date local refs' '
 	mk_child child1 &&
 	mk_child child2 &&
 	(cd child1 && git pull .. master && git push) &&
-	(cd child2 &&
+	(
+		cd child2 &&
 		git pull ../child1 master &&
 		git push &&
-	test $(git rev-parse master) = $(git rev-parse remotes/origin/master))
+		test $(git rev-parse master) = \
+			$(git rev-parse remotes/origin/master)
+	)
 
 '
 
@@ -517,9 +525,11 @@ test_expect_success 'push preserves up-to-date packed refs' '
 
 	mk_test heads/master &&
 	mk_child child &&
-	(cd child &&
+	(
+		cd child &&
 		git push &&
-	! test -f .git/refs/remotes/origin/master)
+		! test -f .git/refs/remotes/origin/master
+	)
 
 '
 
@@ -530,11 +540,13 @@ test_expect_success 'push does not update local refs on failure' '
 	mkdir testrepo/.git/hooks &&
 	echo exit 1 >testrepo/.git/hooks/pre-receive &&
 	chmod +x testrepo/.git/hooks/pre-receive &&
-	(cd child &&
+	(
+		cd child &&
 		git pull .. master
 		test_must_fail git push &&
 		test $(git rev-parse master) != \
-			$(git rev-parse remotes/origin/master))
+			$(git rev-parse remotes/origin/master)
+	)
 
 '
 
@@ -575,34 +587,41 @@ test_expect_success 'push --delete refuses src:dest refspecs' '
 
 test_expect_success 'warn on push to HEAD of non-bare repository' '
 	mk_test heads/master
-	(cd testrepo &&
+	(
+		cd testrepo &&
 		git checkout master &&
-		git config receive.denyCurrentBranch warn) &&
+		git config receive.denyCurrentBranch warn
+	) &&
 	git push testrepo master 2>stderr &&
 	grep "warning: updating the current branch" stderr
 '
 
 test_expect_success 'deny push to HEAD of non-bare repository' '
 	mk_test heads/master
-	(cd testrepo &&
+	(
+		cd testrepo &&
 		git checkout master &&
-		git config receive.denyCurrentBranch true) &&
+		git config receive.denyCurrentBranch true
+	) &&
 	test_must_fail git push testrepo master
 '
 
 test_expect_success 'allow push to HEAD of bare repository (bare)' '
 	mk_test heads/master
-	(cd testrepo &&
+	(
+		cd testrepo &&
 		git checkout master &&
 		git config receive.denyCurrentBranch true &&
-		git config core.bare true) &&
+		git config core.bare true
+	) &&
 	git push testrepo master 2>stderr &&
 	! grep "warning: updating the current branch" stderr
 '
 
 test_expect_success 'allow push to HEAD of non-bare repository (config)' '
 	mk_test heads/master
-	(cd testrepo &&
+	(
+		cd testrepo &&
 		git checkout master &&
 		git config receive.denyCurrentBranch false
 	) &&
@@ -615,7 +634,8 @@ test_expect_success 'fetch with branches' '
 	git branch second $the_first_commit &&
 	git checkout second &&
 	echo ".." > testrepo/.git/branches/branch1 &&
-	(cd testrepo &&
+	(
+		cd testrepo &&
 		git fetch branch1 &&
 		r=$(git show-ref -s --verify refs/heads/branch1) &&
 		test "z$r" = "z$the_commit" &&
@@ -627,7 +647,8 @@ test_expect_success 'fetch with branches' '
 test_expect_success 'fetch with branches containing #' '
 	mk_empty &&
 	echo "..#second" > testrepo/.git/branches/branch2 &&
-	(cd testrepo &&
+	(
+		cd testrepo &&
 		git fetch branch2 &&
 		r=$(git show-ref -s --verify refs/heads/branch2) &&
 		test "z$r" = "z$the_first_commit" &&
@@ -641,7 +662,8 @@ test_expect_success 'push with branches' '
 	git checkout second &&
 	echo "testrepo" > .git/branches/branch1 &&
 	git push branch1 &&
-	(cd testrepo &&
+	(
+		cd testrepo &&
 		r=$(git show-ref -s --verify refs/heads/master) &&
 		test "z$r" = "z$the_first_commit" &&
 		test 1 = $(git for-each-ref refs/heads | wc -l)
@@ -652,7 +674,8 @@ test_expect_success 'push with branches containing #' '
 	mk_empty &&
 	echo "testrepo#branch3" > .git/branches/branch2 &&
 	git push branch2 &&
-	(cd testrepo &&
+	(
+		cd testrepo &&
 		r=$(git show-ref -s --verify refs/heads/branch3) &&
 		test "z$r" = "z$the_first_commit" &&
 		test 1 = $(git for-each-ref refs/heads | wc -l)
-- 
1.7.0.3.436.g2b878

[PATCH v3 3/3] receive-pack: detect aliased updates which can occur with symrefs

[Jay Soffian]

When pushing to a remote repo the sending side filters out aliased
updates (e.g., foo:baz bar:baz). However, it is not possible for the
sender to know if two refs are aliased on the receiving side via
symrefs. Here is one such scenario:

  $ git init origin
  $ (cd origin && touch file && git add file && git commit -a -m intial)
  $ git clone --bare origin origin.git
  $ rm -rf origin

  $ git clone origin.git client

  $ git clone --mirror client backup.git &&
  $ (cd backup.git && git remote set-head origin --auto)

  $ (cd client &&
	git remote add --mirror backup ../backup.git &&
	echo change1 > file && git commit -a -m change1 &&
	git push origin &&
	git push backup
	)

The push to backup fails with:

  Counting objects: 5, done.
  Writing objects: 100% (3/3), 244 bytes, done.
  Total 3 (delta 0), reused 0 (delta 0)
  Unpacking objects: 100% (3/3), done.
  error: Ref refs/remotes/origin/master is at ef3... but expected 262...
  remote: error: failed to lock refs/remotes/origin/master
  To ../backup.git
     262cd57..ef307ff  master -> master
     262cd57..ef307ff  origin/HEAD -> origin/HEAD
   ! [remote rejected] origin/master -> origin/master (failed to lock)
  error: failed to push some refs to '../backup.git'

The reason is that refs/remotes/origin/HEAD is a symref to
refs/remotes/origin/master, but it is not possible for the sending side
to unambiguously know this.

This commit fixes the issue by having receive-pack ignore any update to
a symref whose target is being identically updated. If a symref and its
target are being updated inconsistently, then the update for both fails
with an error message ("refusing inconsistent update...") to help
diagnose the situation.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
 builtin-receive-pack.c |   68 +++++++++++++++++++++++++++++++++++++++++++++---
 t/t5516-fetch-push.sh  |   49 ++++++++++++++++++++++++++++++++++
 2 files changed, 113 insertions(+), 4 deletions(-)

diff --git a/builtin-receive-pack.c b/builtin-receive-pack.c
index fffb6ea..bb34757 100644
--- a/builtin-receive-pack.c
+++ b/builtin-receive-pack.c
@@ -9,6 +9,7 @@
 #include "object.h"
 #include "remote.h"
 #include "transport.h"
+#include "string-list.h"
 
 static const char receive_pack_usage[] = "git receive-pack <git-dir>";
 
@@ -129,6 +130,7 @@ static void write_head_info(void)
 struct command {
 	struct command *next;
 	const char *error_string;
+	unsigned int skip_update;
 	unsigned char old_sha1[20];
 	unsigned char new_sha1[20];
 	char ref_name[FLEX_ARRAY]; /* more */
@@ -486,6 +488,63 @@ static void run_update_post_hook(struct command *commands)
 	}
 }
 
+static void check_aliased_update(struct command *cmd, struct string_list *list)
+{
+	struct string_list_item *item;
+	struct command *dst_cmd;
+	unsigned char sha1[20];
+	char cmd_oldh[41], cmd_newh[41], dst_oldh[41], dst_newh[41];
+	int flag;
+
+	const char *dst_name = resolve_ref(cmd->ref_name, sha1, 0, &flag);
+
+	if (!(flag & REF_ISSYMREF))
+		return;
+
+	if ((item = string_list_lookup(dst_name, list)) == NULL)
+		return;
+
+	cmd->skip_update = 1;
+
+	dst_cmd = (struct command *) item->util;
+
+	if (!hashcmp(cmd->old_sha1, dst_cmd->old_sha1) &&
+	    !hashcmp(cmd->new_sha1, dst_cmd->new_sha1))
+		return;
+
+	dst_cmd->skip_update = 1;
+
+	strcpy(cmd_oldh, find_unique_abbrev(cmd->old_sha1, DEFAULT_ABBREV));
+	strcat(cmd_newh, find_unique_abbrev(cmd->new_sha1, DEFAULT_ABBREV));
+	strcpy(dst_oldh, find_unique_abbrev(dst_cmd->old_sha1, DEFAULT_ABBREV));
+	strcat(dst_newh, find_unique_abbrev(dst_cmd->new_sha1, DEFAULT_ABBREV));
+	rp_error("refusing inconsistent update between symref '%s' (%s..%s) and"
+		 " its target '%s' (%s..%s)",
+		 cmd->ref_name, cmd_oldh, cmd_newh,
+		 dst_cmd->ref_name, dst_oldh, dst_newh);
+
+	cmd->error_string = dst_cmd->error_string =
+		"inconsistent aliased update";
+}
+
+static void check_aliased_updates(struct command *commands)
+{
+	struct command *cmd;
+	struct string_list ref_list = { NULL, 0, 0, 0 };
+
+	for (cmd = commands; cmd; cmd = cmd->next) {
+		struct string_list_item *item =
+			string_list_append(cmd->ref_name, &ref_list);
+		item->util = (void *)cmd;
+	}
+	sort_string_list(&ref_list);
+
+	for (cmd = commands; cmd; cmd = cmd->next)
+		check_aliased_update(cmd, &ref_list);
+
+	string_list_clear(&ref_list, 0);
+}
+
 static void execute_commands(struct command *commands, const char *unpacker_error)
 {
 	struct command *cmd;
@@ -503,10 +562,13 @@ static void execute_commands(struct command *commands, const char *unpacker_erro
 		return;
 	}
 
+	check_aliased_updates(commands);
+
 	head_name = resolve_ref("HEAD", sha1, 0, NULL);
 
 	for (cmd = commands; cmd; cmd = cmd->next)
-		cmd->error_string = update(cmd);
+		if (!cmd->skip_update)
+			cmd->error_string = update(cmd);
 }
 
 static struct command *read_head_info(void)
@@ -541,12 +603,10 @@ static struct command *read_head_info(void)
 			if (strstr(refname + reflen + 1, "side-band-64k"))
 				use_sideband = LARGE_PACKET_MAX;
 		}
-		cmd = xmalloc(sizeof(struct command) + len - 80);
+		cmd = xcalloc(1, sizeof(struct command) + len - 80);
 		hashcpy(cmd->old_sha1, old_sha1);
 		hashcpy(cmd->new_sha1, new_sha1);
 		memcpy(cmd->ref_name, line + 82, len - 81);
-		cmd->error_string = NULL;
-		cmd->next = NULL;
 		*p = cmd;
 		p = &cmd->next;
 	}
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 3148789..f0813e0 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -683,4 +683,53 @@ test_expect_success 'push with branches containing #' '
 	git checkout master
 '
 
+test_expect_success 'push into aliased refs (consistent)' '
+	mk_test heads/master &&
+	mk_child child1 &&
+	mk_child child2 &&
+	(
+		cd child1 &&
+		git branch foo &&
+		git symbolic-ref refs/heads/bar refs/heads/foo
+		git config receive.denyCurrentBranch false
+	) &&
+	(
+		cd child2 &&
+		>path2 &&
+		git add path2 &&
+		test_tick &&
+		git commit -a -m child2 &&
+		git branch foo &&
+		git branch bar &&
+		git push ../child1 foo bar
+	)
+'
+
+test_expect_success 'push into aliased refs (inconsistent)' '
+	mk_test heads/master &&
+	mk_child child1 &&
+	mk_child child2 &&
+	(
+		cd child1 &&
+		git branch foo &&
+		git symbolic-ref refs/heads/bar refs/heads/foo
+		git config receive.denyCurrentBranch false
+	) &&
+	(
+		cd child2 &&
+		>path2 &&
+		git add path2 &&
+		test_tick &&
+		git commit -a -m child2 &&
+		git branch foo &&
+		>path3 &&
+		git add path3 &&
+		test_tick &&
+		git commit -a -m child2 &&
+		git branch bar &&
+		test_must_fail git push ../child1 foo bar 2>stderr &&
+		grep "refusing inconsistent update" stderr
+	)
+'
+
 test_done
-- 
1.7.0.3.436.g2b878

[PATCH v3 3/3] receive-pack: detect aliased updates which can occur with symrefs

[Jay Soffian]

When pushing to a remote repo the sending side filters out aliased
updates (e.g., foo:baz bar:baz). However, it is not possible for the
sender to know if two refs are aliased on the receiving side via
symrefs. Here is one such scenario:

  $ git init origin
  $ (cd origin && touch file && git add file && git commit -a -m intial)
  $ git clone --bare origin origin.git
  $ rm -rf origin

  $ git clone origin.git client

  $ git clone --mirror client backup.git &&
  $ (cd backup.git && git remote set-head origin --auto)

  $ (cd client &&
	git remote add --mirror backup ../backup.git &&
	echo change1 > file && git commit -a -m change1 &&
	git push origin &&
	git push backup
	)

The push to backup fails with:

  Counting objects: 5, done.
  Writing objects: 100% (3/3), 244 bytes, done.
  Total 3 (delta 0), reused 0 (delta 0)
  Unpacking objects: 100% (3/3), done.
  error: Ref refs/remotes/origin/master is at ef3... but expected 262...
  remote: error: failed to lock refs/remotes/origin/master
  To ../backup.git
     262cd57..ef307ff  master -> master
     262cd57..ef307ff  origin/HEAD -> origin/HEAD
   ! [remote rejected] origin/master -> origin/master (failed to lock)
  error: failed to push some refs to '../backup.git'

The reason is that refs/remotes/origin/HEAD is a symref to
refs/remotes/origin/master, but it is not possible for the sending side
to unambiguously know this.

This commit fixes the issue by having receive-pack ignore any update to
a symref whose target is being identically updated. If a symref and its
target are being updated inconsistently, then the update for both fails
with an error message ("refusing inconsistent update...") to help
diagnose the situation.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
 builtin-receive-pack.c |   68 +++++++++++++++++++++++++++++++++++++++++++++---
 t/t5516-fetch-push.sh  |   49 ++++++++++++++++++++++++++++++++++
 2 files changed, 113 insertions(+), 4 deletions(-)

diff --git a/builtin-receive-pack.c b/builtin-receive-pack.c
index fffb6ea..bb34757 100644
--- a/builtin-receive-pack.c
+++ b/builtin-receive-pack.c
@@ -9,6 +9,7 @@
 #include "object.h"
 #include "remote.h"
 #include "transport.h"
+#include "string-list.h"
 
 static const char receive_pack_usage[] = "git receive-pack <git-dir>";
 
@@ -129,6 +130,7 @@ static void write_head_info(void)
 struct command {
 	struct command *next;
 	const char *error_string;
+	unsigned int skip_update;
 	unsigned char old_sha1[20];
 	unsigned char new_sha1[20];
 	char ref_name[FLEX_ARRAY]; /* more */
@@ -486,6 +488,63 @@ static void run_update_post_hook(struct command *commands)
 	}
 }
 
+static void check_aliased_update(struct command *cmd, struct string_list *list)
+{
+	struct string_list_item *item;
+	struct command *dst_cmd;
+	unsigned char sha1[20];
+	char cmd_oldh[41], cmd_newh[41], dst_oldh[41], dst_newh[41];
+	int flag;
+
+	const char *dst_name = resolve_ref(cmd->ref_name, sha1, 0, &flag);
+
+	if (!(flag & REF_ISSYMREF))
+		return;
+
+	if ((item = string_list_lookup(dst_name, list)) == NULL)
+		return;
+
+	cmd->skip_update = 1;
+
+	dst_cmd = (struct command *) item->util;
+
+	if (!hashcmp(cmd->old_sha1, dst_cmd->old_sha1) &&
+	    !hashcmp(cmd->new_sha1, dst_cmd->new_sha1))
+		return;
+
+	dst_cmd->skip_update = 1;
+
+	strcpy(cmd_oldh, find_unique_abbrev(cmd->old_sha1, DEFAULT_ABBREV));
+	strcat(cmd_newh, find_unique_abbrev(cmd->new_sha1, DEFAULT_ABBREV));
+	strcpy(dst_oldh, find_unique_abbrev(dst_cmd->old_sha1, DEFAULT_ABBREV));
+	strcat(dst_newh, find_unique_abbrev(dst_cmd->new_sha1, DEFAULT_ABBREV));
+	rp_error("refusing inconsistent update between symref '%s' (%s..%s) and"
+		 " its target '%s' (%s..%s)",
+		 cmd->ref_name, cmd_oldh, cmd_newh,
+		 dst_cmd->ref_name, dst_oldh, dst_newh);
+
+	cmd->error_string = dst_cmd->error_string =
+		"inconsistent aliased update";
+}
+
+static void check_aliased_updates(struct command *commands)
+{
+	struct command *cmd;
+	struct string_list ref_list = { NULL, 0, 0, 0 };
+
+	for (cmd = commands; cmd; cmd = cmd->next) {
+		struct string_list_item *item =
+			string_list_append(cmd->ref_name, &ref_list);
+		item->util = (void *)cmd;
+	}
+	sort_string_list(&ref_list);
+
+	for (cmd = commands; cmd; cmd = cmd->next)
+		check_aliased_update(cmd, &ref_list);
+
+	string_list_clear(&ref_list, 0);
+}
+
 static void execute_commands(struct command *commands, const char *unpacker_error)
 {
 	struct command *cmd;
@@ -503,10 +562,13 @@ static void execute_commands(struct command *commands, const char *unpacker_erro
 		return;
 	}
 
+	check_aliased_updates(commands);
+
 	head_name = resolve_ref("HEAD", sha1, 0, NULL);
 
 	for (cmd = commands; cmd; cmd = cmd->next)
-		cmd->error_string = update(cmd);
+		if (!cmd->skip_update)
+			cmd->error_string = update(cmd);
 }
 
 static struct command *read_head_info(void)
@@ -541,12 +603,10 @@ static struct command *read_head_info(void)
 			if (strstr(refname + reflen + 1, "side-band-64k"))
 				use_sideband = LARGE_PACKET_MAX;
 		}
-		cmd = xmalloc(sizeof(struct command) + len - 80);
+		cmd = xcalloc(1, sizeof(struct command) + len - 80);
 		hashcpy(cmd->old_sha1, old_sha1);
 		hashcpy(cmd->new_sha1, new_sha1);
 		memcpy(cmd->ref_name, line + 82, len - 81);
-		cmd->error_string = NULL;
-		cmd->next = NULL;
 		*p = cmd;
 		p = &cmd->next;
 	}
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 3148789..f0813e0 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -683,4 +683,53 @@ test_expect_success 'push with branches containing #' '
 	git checkout master
 '
 
+test_expect_success 'push into aliased refs (consistent)' '
+	mk_test heads/master &&
+	mk_child child1 &&
+	mk_child child2 &&
+	(
+		cd child1 &&
+		git branch foo &&
+		git symbolic-ref refs/heads/bar refs/heads/foo
+		git config receive.denyCurrentBranch false
+	) &&
+	(
+		cd child2 &&
+		>path2 &&
+		git add path2 &&
+		test_tick &&
+		git commit -a -m child2 &&
+		git branch foo &&
+		git branch bar &&
+		git push ../child1 foo bar
+	)
+'
+
+test_expect_success 'push into aliased refs (inconsistent)' '
+	mk_test heads/master &&
+	mk_child child1 &&
+	mk_child child2 &&
+	(
+		cd child1 &&
+		git branch foo &&
+		git symbolic-ref refs/heads/bar refs/heads/foo
+		git config receive.denyCurrentBranch false
+	) &&
+	(
+		cd child2 &&
+		>path2 &&
+		git add path2 &&
+		test_tick &&
+		git commit -a -m child2 &&
+		git branch foo &&
+		>path3 &&
+		git add path3 &&
+		test_tick &&
+		git commit -a -m child2 &&
+		git branch bar &&
+		test_must_fail git push ../child1 foo bar 2>stderr &&
+		grep "refusing inconsistent update" stderr
+	)
+'
+
 test_done
-- 
1.7.0.3.436.g2b878

Re: [PATCH v3 3/3] receive-pack: detect aliased updates which can occur with symrefs

[Ævar Arnfjörð Bjarmason]

On Mon, Apr 19, 2010 at 22:19, Jay Soffian <jaysoffian@gmail.com> wrote:
> When pushing to a remote repo the sending side filters out aliased
> updates (e.g., foo:baz bar:baz). However, it is not possible for the
> sender to know if two refs are aliased on the receiving side via
> symrefs. Here is one such scenario:
>
>  $ git init origin
>  $ (cd origin && touch file && git add file && git commit -a -m intial)
>  $ git clone --bare origin origin.git
>  $ rm -rf origin
>
>  $ git clone origin.git client
>
>  $ git clone --mirror client backup.git &&
>  $ (cd backup.git && git remote set-head origin --auto)
>
>  $ (cd client &&
>        git remote add --mirror backup ../backup.git &&
>        echo change1 > file && git commit -a -m change1 &&
>        git push origin &&
>        git push backup
>        )
>
> The push to backup fails with:
>
>  Counting objects: 5, done.
>  Writing objects: 100% (3/3), 244 bytes, done.
>  Total 3 (delta 0), reused 0 (delta 0)
>  Unpacking objects: 100% (3/3), done.
>  error: Ref refs/remotes/origin/master is at ef3... but expected 262...
>  remote: error: failed to lock refs/remotes/origin/master
>  To ../backup.git
>     262cd57..ef307ff  master -> master
>     262cd57..ef307ff  origin/HEAD -> origin/HEAD
>   ! [remote rejected] origin/master -> origin/master (failed to lock)
>  error: failed to push some refs to '../backup.git'
>
> The reason is that refs/remotes/origin/HEAD is a symref to
> refs/remotes/origin/master, but it is not possible for the sending side
> to unambiguously know this.
>
> This commit fixes the issue by having receive-pack ignore any update to
> a symref whose target is being identically updated. If a symref and its
> target are being updated inconsistently, then the update for both fails
> with an error message ("refusing inconsistent update...") to help
> diagnose the situation.
>
> Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
> ---
>  builtin-receive-pack.c |   68 +++++++++++++++++++++++++++++++++++++++++++++---
>  t/t5516-fetch-push.sh  |   49 ++++++++++++++++++++++++++++++++++
>  2 files changed, 113 insertions(+), 4 deletions(-)
>
> diff --git a/builtin-receive-pack.c b/builtin-receive-pack.c
> index fffb6ea..bb34757 100644
> --- a/builtin-receive-pack.c
> +++ b/builtin-receive-pack.c
> @@ -9,6 +9,7 @@
>  #include "object.h"
>  #include "remote.h"
>  #include "transport.h"
> +#include "string-list.h"
>
>  static const char receive_pack_usage[] = "git receive-pack <git-dir>";
>
> @@ -129,6 +130,7 @@ static void write_head_info(void)
>  struct command {
>        struct command *next;
>        const char *error_string;
> +       unsigned int skip_update;
>        unsigned char old_sha1[20];
>        unsigned char new_sha1[20];
>        char ref_name[FLEX_ARRAY]; /* more */
> @@ -486,6 +488,63 @@ static void run_update_post_hook(struct command *commands)
>        }
>  }
>
> +static void check_aliased_update(struct command *cmd, struct string_list *list)
> +{
> +       struct string_list_item *item;
> +       struct command *dst_cmd;
> +       unsigned char sha1[20];
> +       char cmd_oldh[41], cmd_newh[41], dst_oldh[41], dst_newh[41];
> +       int flag;
> +
> +       const char *dst_name = resolve_ref(cmd->ref_name, sha1, 0, &flag);
> +
> +       if (!(flag & REF_ISSYMREF))
> +               return;
> +
> +       if ((item = string_list_lookup(dst_name, list)) == NULL)
> +               return;
> +
> +       cmd->skip_update = 1;
> +
> +       dst_cmd = (struct command *) item->util;
> +
> +       if (!hashcmp(cmd->old_sha1, dst_cmd->old_sha1) &&
> +           !hashcmp(cmd->new_sha1, dst_cmd->new_sha1))
> +               return;
> +
> +       dst_cmd->skip_update = 1;
> +
> +       strcpy(cmd_oldh, find_unique_abbrev(cmd->old_sha1, DEFAULT_ABBREV));
> +       strcat(cmd_newh, find_unique_abbrev(cmd->new_sha1, DEFAULT_ABBREV));
> +       strcpy(dst_oldh, find_unique_abbrev(dst_cmd->old_sha1, DEFAULT_ABBREV));
> +       strcat(dst_newh, find_unique_abbrev(dst_cmd->new_sha1, DEFAULT_ABBREV));
> +       rp_error("refusing inconsistent update between symref '%s' (%s..%s) and"
> +                " its target '%s' (%s..%s)",
> +                cmd->ref_name, cmd_oldh, cmd_newh,
> +                dst_cmd->ref_name, dst_oldh, dst_newh);
> +
> +       cmd->error_string = dst_cmd->error_string =
> +               "inconsistent aliased update";
> +}
> +
> +static void check_aliased_updates(struct command *commands)
> +{
> +       struct command *cmd;
> +       struct string_list ref_list = { NULL, 0, 0, 0 };
> +
> +       for (cmd = commands; cmd; cmd = cmd->next) {
> +               struct string_list_item *item =
> +                       string_list_append(cmd->ref_name, &ref_list);
> +               item->util = (void *)cmd;
> +       }
> +       sort_string_list(&ref_list);
> +
> +       for (cmd = commands; cmd; cmd = cmd->next)
> +               check_aliased_update(cmd, &ref_list);
> +
> +       string_list_clear(&ref_list, 0);
> +}
> +
>  static void execute_commands(struct command *commands, const char *unpacker_error)
>  {
>        struct command *cmd;
> @@ -503,10 +562,13 @@ static void execute_commands(struct command *commands, const char *unpacker_erro
>                return;
>        }
>
> +       check_aliased_updates(commands);
> +
>        head_name = resolve_ref("HEAD", sha1, 0, NULL);
>
>        for (cmd = commands; cmd; cmd = cmd->next)
> -               cmd->error_string = update(cmd);
> +               if (!cmd->skip_update)
> +                       cmd->error_string = update(cmd);
>  }
>
>  static struct command *read_head_info(void)
> @@ -541,12 +603,10 @@ static struct command *read_head_info(void)
>                        if (strstr(refname + reflen + 1, "side-band-64k"))
>                                use_sideband = LARGE_PACKET_MAX;
>                }
> -               cmd = xmalloc(sizeof(struct command) + len - 80);
> +               cmd = xcalloc(1, sizeof(struct command) + len - 80);
>                hashcpy(cmd->old_sha1, old_sha1);
>                hashcpy(cmd->new_sha1, new_sha1);
>                memcpy(cmd->ref_name, line + 82, len - 81);
> -               cmd->error_string = NULL;
> -               cmd->next = NULL;
>                *p = cmd;
>                p = &cmd->next;
>        }
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index 3148789..f0813e0 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -683,4 +683,53 @@ test_expect_success 'push with branches containing #' '
>        git checkout master
>  '
>
> +test_expect_success 'push into aliased refs (consistent)' '
> +       mk_test heads/master &&
> +       mk_child child1 &&
> +       mk_child child2 &&
> +       (
> +               cd child1 &&
> +               git branch foo &&
> +               git symbolic-ref refs/heads/bar refs/heads/foo
> +               git config receive.denyCurrentBranch false
> +       ) &&
> +       (
> +               cd child2 &&
> +               >path2 &&
> +               git add path2 &&
> +               test_tick &&
> +               git commit -a -m child2 &&
> +               git branch foo &&
> +               git branch bar &&
> +               git push ../child1 foo bar
> +       )
> +'
> +
> +test_expect_success 'push into aliased refs (inconsistent)' '
> +       mk_test heads/master &&
> +       mk_child child1 &&
> +       mk_child child2 &&
> +       (
> +               cd child1 &&
> +               git branch foo &&
> +               git symbolic-ref refs/heads/bar refs/heads/foo
> +               git config receive.denyCurrentBranch false
> +       ) &&
> +       (
> +               cd child2 &&
> +               >path2 &&
> +               git add path2 &&
> +               test_tick &&
> +               git commit -a -m child2 &&
> +               git branch foo &&
> +               >path3 &&
> +               git add path3 &&
> +               test_tick &&
> +               git commit -a -m child2 &&
> +               git branch bar &&
> +               test_must_fail git push ../child1 foo bar 2>stderr &&
> +               grep "refusing inconsistent update" stderr
> +       )
> +'
> +
>  test_done
> --
> 1.7.0.3.436.g2b878

I've been having troubles running t5516-fetch-push.sh which I traced
down to this patch, here's the gist of it:

    $ make
    rm -f -r test-results
    make aggregate-results-and-cleanup
    make[1]: Entering directory `/home/avar/g/git/t'
    echo "*** t5516-fetch-push.sh ***"; GIT_CONFIG=.git/config
'/bin/sh' t5516-fetch-push.sh
    *** t5516-fetch-push.sh ***
    *   ok 1: setup
    *   ok 2: fetch without wildcard
    *   ok 3: fetch with wildcard
    *   ok 4: fetch with insteadOf
    *   ok 5: fetch with pushInsteadOf (should not rewrite)
    *   ok 6: push without wildcard
    *   ok 7: push with wildcard
    *   ok 8: push with insteadOf
    *   ok 9: push with pushInsteadOf
    *   ok 10: push with pushInsteadOf and explicit pushurl
(pushInsteadOf should not rewrite)
    *   ok 11: push with matching heads
    *   ok 12: push with matching heads on the command line
    *   ok 13: failed (non-fast-forward) push with matching heads
    *   ok 14: push --force with matching heads
    *   ok 15: push with matching heads and forced update
    *   ok 16: push with no ambiguity (1)
    *   ok 17: push with no ambiguity (2)
    *   ok 18: push with colon-less refspec, no ambiguity
    *   ok 19: push with weak ambiguity (1)
    *   ok 20: push with weak ambiguity (2)
    *   ok 21: push with ambiguity
    *   ok 22: push with colon-less refspec (1)
    *   ok 23: push with colon-less refspec (2)
    *   ok 24: push with colon-less refspec (3)
    *   ok 25: push with colon-less refspec (4)
    *   ok 26: push head with non-existant, incomplete dest
    *   ok 27: push tag with non-existant, incomplete dest
    *   ok 28: push sha1 with non-existant, incomplete dest
    *   ok 29: push ref expression with non-existant, incomplete dest
    *   ok 30: push with HEAD
    *   ok 31: push with HEAD nonexisting at remote
    *   ok 32: push with +HEAD
    *   ok 33: push HEAD with non-existant, incomplete dest
    *   ok 34: push with config remote.*.push = HEAD
    *   ok 35: push with config remote.*.pushurl
    *   ok 36: push with dry-run
    *   ok 37: push updates local refs
    *   ok 38: push updates up-to-date local refs
    *   ok 39: push preserves up-to-date packed refs
    *   ok 40: push does not update local refs on failure
    *   ok 41: allow deleting an invalid remote ref
    *   ok 42: allow deleting a ref using --delete
    *   ok 43: allow deleting a tag using --delete
    *   ok 44: push --delete without args aborts
    *   ok 45: push --delete refuses src:dest refspecs
    *   ok 46: warn on push to HEAD of non-bare repository
    *   ok 47: deny push to HEAD of non-bare repository
    *   ok 48: allow push to HEAD of bare repository (bare)
    *   ok 49: allow push to HEAD of non-bare repository (config)
    *   ok 50: fetch with branches
    *   ok 51: fetch with branches containing #
    *   ok 52: push with branches
    *   ok 53: push with branches containing #
    *   ok 54: push into aliased refs (consistent)
    *** buffer overflow detected ***: receive-pack terminated
    ======= Backtrace: =========
    /lib/tls/i686/cmov/libc.so.6(__fortify_fail+0x50)[0x402a0350]
    /lib/tls/i686/cmov/libc.so.6(+0xe128a)[0x4029f28a]
    /lib/tls/i686/cmov/libc.so.6(+0xe05ba)[0x4029e5ba]
    receive-pack[0x8094de8]
    receive-pack[0x804b8ab]
    receive-pack[0x804be73]
    /lib/tls/i686/cmov/libc.so.6(__libc_start_main+0xe6)[0x401d4bd6]
    receive-pack[0x804b211]
    ======= Memory map: ========
    08048000-0813e000 r-xp 00000000 08:02 6792494
/home/avar/g/git/git-receive-pack
    0813e000-0813f000 r--p 000f6000 08:02 6792494
/home/avar/g/git/git-receive-pack
    0813f000-08143000 rw-p 000f7000 08:02 6792494
/home/avar/g/git/git-receive-pack
    08143000-08189000 rw-p 00000000 00:00 0
    0996b000-0998c000 rw-p 00000000 00:00 0          [heap]
    40000000-4001b000 r-xp 00000000 08:02 5496926    /lib/ld-2.11.1.so
    4001b000-4001c000 r--p 0001a000 08:02 5496926    /lib/ld-2.11.1.so
    4001c000-4001d000 rw-p 0001b000 08:02 5496926    /lib/ld-2.11.1.so
    4001d000-4001e000 r-xp 00000000 00:00 0          [vdso]
    4001e000-40020000 rw-p 00000000 00:00 0
    4003d000-40050000 r-xp 00000000 08:02 5497495    /lib/libz.so.1.2.3.3
    40050000-40051000 r--p 00012000 08:02 5497495    /lib/libz.so.1.2.3.3
    40051000-40052000 rw-p 00013000 08:02 5497495    /lib/libz.so.1.2.3.3
    40052000-4018a000 r-xp 00000000 08:02 4431990
/lib/i686/cmov/libcrypto.so.0.9.8
    4018a000-40192000 r--p 00137000 08:02 4431990
/lib/i686/cmov/libcrypto.so.0.9.8
    40192000-401a0000 rw-p 0013f000 08:02 4431990
/lib/i686/cmov/libcrypto.so.0.9.8
    401a0000-401a5000 rw-p 00000000 00:00 0
    401a5000-401ba000 r-xp 00000000 08:02 7234200
/lib/tls/i686/cmov/libpthread-2.11.1.so
    401ba000-401bb000 r--p 00014000 08:02 7234200
/lib/tls/i686/cmov/libpthread-2.11.1.so
    401bb000-401bc000 rw-p 00015000 08:02 7234200
/lib/tls/i686/cmov/libpthread-2.11.1.so
    401bc000-401be000 rw-p 00000000 00:00 0
    401be000-40311000 r-xp 00000000 08:02 7233919
/lib/tls/i686/cmov/libc-2.11.1.so
    40311000-40312000 ---p 00153000 08:02 7233919
/lib/tls/i686/cmov/libc-2.11.1.so
    40312000-40314000 r--p 00153000 08:02 7233919
/lib/tls/i686/cmov/libc-2.11.1.so
    40314000-40315000 rw-p 00155000 08:02 7233919
/lib/tls/i686/cmov/libc-2.11.1.so
    40315000-40318000 rw-p 00000000 00:00 0
    40318000-4031a000 r-xp 00000000 08:02 7233925
/lib/tls/i686/cmov/libdl-2.11.1.so
    4031a000-4031b000 r--p 00001000 08:02 7233925
/lib/tls/i686/cmov/libdl-2.11.1.so
    4031b000-4031c000 rw-p 00002000 08:02 7233925
/lib/tls/i686/cmov/libdl-2.11.1.so
    4031c000-4031d000 rw-p 00000000 00:00 0
    4031d000-4033a000 r-xp 00000000 08:02 7217157    /lib/libgcc_s.so.1
    4033a000-4033b000 r--p 0001c000 08:02 7217157    /lib/libgcc_s.so.1
    4033b000-4033c000 rw-p 0001d000 08:02 7217157    /lib/libgcc_s.so.1
    bf9b1000-bf9c6000 rw-p 00000000 00:00 0          [stack]
    * FAIL 55: push into aliased refs (inconsistent)
    	
    		mk_test heads/master &&
    		mk_child child1 &&
    		mk_child child2 &&
    		(
    			cd child1 &&
    			git branch foo &&
    			git symbolic-ref refs/heads/bar refs/heads/foo
    			git config receive.denyCurrentBranch false
    		) &&
    		(
    			cd child2 &&
    			>path2 &&
    			git add path2 &&
    			test_tick &&
    			git commit -a -m child2 &&
    			git branch foo &&
    			>path3 &&
    			git add path3 &&
    			test_tick &&
    			git commit -a -m child2 &&
    			git branch bar &&
    			test_must_fail git push ../child1 foo bar 2>stderr &&
    			grep "refusing inconsistent update" stderr
    		)
    	
    *   ok 56: push --porcelain
    *   ok 57: push --porcelain bad url
    *   ok 58: push --porcelain rejected
    *   ok 59: push --porcelain --dry-run rejected
    * failed 1 among 59 test(s)
    make[1]: *** [t5516-fetch-push.sh] Error 1
    make[1]: Leaving directory `/home/avar/g/git/t'
    make: *** [all] Error 2


And the bisect script/log from 1.7.0 to master:

    #!/bin/sh
    cd ~/g/git
    git reset --hard >/dev/null
    git clean -dxf >/dev/null

    make >/dev/null 2>&1
    cp -v /tmp/Makefile t/Makefile
    make test
    ret=$?

    git reset --hard >/dev/null
    git clean -dxf  >/dev/null

    exit $ret

log:

    $ git bisect log
    git bisect start
    # bad: [92a75a391e66cfc278cf59741e484efd80c02176] Merge branch 'maint'
    git bisect bad 92a75a391e66cfc278cf59741e484efd80c02176
    # good: [e923eaeb901ff056421b9007adcbbce271caa7b6] Git 1.7.0
    git bisect good e923eaeb901ff056421b9007adcbbce271caa7b6
    # good: [419fe5bc861517c789c8f028519e085fd8d1992f] fmt-merge-msg:
be quiet if nothing to merge
    git bisect good 419fe5bc861517c789c8f028519e085fd8d1992f
    # good: [b6b0afdc30e066788592ca07c9a6c6936c68cc11] test-lib: some
shells do not let $? propagate into an eval
    git bisect good b6b0afdc30e066788592ca07c9a6c6936c68cc11
    # good: [cd4ce1e8a81ef5c24af7b914fb72212273e7d489] Merge branch
'jc/status-show-ignored'
    git bisect good cd4ce1e8a81ef5c24af7b914fb72212273e7d489
    # bad: [71f1d729b39ce5c92df6d623151f88bbb5d4c774] Merge branch
'jn/gitweb-our-squelch'
    git bisect bad 71f1d729b39ce5c92df6d623151f88bbb5d4c774
    # good: [465ef577b59986d70c51ea72dd3f87759b2bcd4f] Merge branch
'jn/submodule-basic-test'
    git bisect good 465ef577b59986d70c51ea72dd3f87759b2bcd4f
    # bad: [9215f76fb6d938ae93889f46f27cff22723fe0e4] Merge branch
'js/maint-receive-pack-symref-alias'
    git bisect bad 9215f76fb6d938ae93889f46f27cff22723fe0e4
    # good: [9b0aa728705439ca4b4e7ec845f79f8487059320] Extract
verify_pack_index for reuse from verify_pack
    git bisect good 9b0aa728705439ca4b4e7ec845f79f8487059320
    # good: [90d05713575ea6ed21d05228bcda8461f7b28ccf]
http.c::new_http_pack_request: do away with the temp variable filename
    git bisect good 90d05713575ea6ed21d05228bcda8461f7b28ccf
    # bad: [da3efdb17bef25dedc753131462ee784d822132e] receive-pack:
detect aliased updates which can occur with symrefs
    git bisect bad da3efdb17bef25dedc753131462ee784d822132e
    # good: [5e1c71fd1488a33680c313f287b88d9f7a7d3e45] receive-pack:
switch global variable 'commands' to a parameter
    git bisect good 5e1c71fd1488a33680c313f287b88d9f7a7d3e45

What the t/Makefile patch does is this:

    diff --git a/t/Makefile b/t/Makefile
    index 25c559b..da0d5bc 100644
    --- a/t/Makefile
    +++ b/t/Makefile
    @@ -13,7 +13,7 @@ RM ?= rm -f
     # Shell quote;
     SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))

    -T = $(wildcard t[0-9][0-9][0-9][0-9]-*.sh)
    +T = $(wildcard t[5][5][1][6]-*.sh)
     TSVN = $(wildcard t91[0-9][0-9]-*.sh)

The reason is that I can't reproduce this when running just the
script, it has to be through make test, which suggests some sort of
evil heisenbug. Even running the exact same commands the Makefile
would run to execute the script doesn't work.

I also ran the tests under valgrind, here's the full output:
http://gist.github.com/433379

[RFC/PATCH] ignoring a fetch that overwrites local symref

[Junio C Hamano]

This is a companion to an ancient thread

    http://thread.gmane.org/gmane.comp.version-control.git/145311/focus=145337

in which an error was dealt with while pushing into a "mirror"
repository that has a symbolic reference refs/remotes/origin/HEAD
pointing at refs/remotes/origin/master with "git push --mirror".
The issue was that the receiving end was told to update origin/HEAD
and origin/master separately; if origin/HEAD is updated, that would
update origin/master at the same time, and then when attempting to
update origin/master, it would notice that it no longer has the
expected old value and barf.  After the series, we started ignoring
such pushes to HEAD on the receiving end.

But you can suffer from a similar issue transferring objects in the
opposite direction.  If you run "fetch --mirror" in to such a
"mirror" repository, the other side would advertise both 'master'
and 'HEAD' under refs/remotes/origin/ hierarchy, and refs/*:refs/*
wildcard would try to grab both of them.

Work it around by noticing a wildcard match that attempts to update
a local symbolic ref and ignoring it.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * While I do not think it is sane to have symbolic refs in the
   receiving ref hierarchy (e.g. refs/remotes/origin/) that has a
   matching ref in the corresponding ref hierarchy in the sending
   side (e.g. the same, or refs/heads/ if you are doing a mirror) in
   the first place, we addressed the issue brought by such a setting
   on the push side, so it is probably a good idea to do it on the
   fetch side.

   This is marked RFC as it cheats by ignoring symrefs that were not
   explicitly asked, instead of doing the "is the underlying thing
   going to be updated with the same operation?" logic the old patch
   did in da3efdb (receive-pack: detect aliased updates which can
   occur with symrefs, 2010-04-19). I think this simpler logic is
   sufficient but there may be corner cases that merit the more
   elaborate one, hence RFC.

 remote.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git i/remote.c w/remote.c
index 6aa49c0..ca1f8f2 100644
--- i/remote.c
+++ w/remote.c
@@ -1370,6 +1370,16 @@ int branch_merge_matches(struct branch *branch,
 	return refname_match(branch->merge[i]->src, refname, ref_fetch_rules);
 }
 
+static int ignore_symref_update(const char *refname)
+{
+	unsigned char sha1[20];
+	int flag;
+
+	if (!resolve_ref_unsafe(refname, sha1, 0, &flag))
+		return 0; /* non-existing refs are OK */
+	return (flag & REF_ISSYMREF);
+}
+
 static struct ref *get_expanded_map(const struct ref *remote_refs,
 				    const struct refspec *refspec)
 {
@@ -1383,7 +1393,8 @@ static struct ref *get_expanded_map(const struct ref *remote_refs,
 		if (strchr(ref->name, '^'))
 			continue; /* a dereference item */
 		if (match_name_with_pattern(refspec->src, ref->name,
-					    refspec->dst, &expn_name)) {
+					    refspec->dst, &expn_name) &&
+		    !ignore_symref_update(expn_name)) {
 			struct ref *cpy = copy_ref(ref);
 
 			cpy->peer_ref = alloc_ref(expn_name);

[PATCH] fetch: ignore wildcarded refspecs that update local symbolic refs

[Junio C Hamano]

In a repository cloned from somewhere else, you typically have a
symbolic ref refs/remotes/origin/HEAD pointing at the 'master'
remote-tracking ref that is next to it.  When fetching into such a
repository with "git fetch --mirror" from another repository that
was similarly cloned, the implied wildcard refspec refs/*:refs/*
will end up asking to update refs/remotes/origin/HEAD with the
object at refs/remotes/origin/HEAD at the remote side, while asking
to update refs/remotes/origin/master the same way.  Depending on the
order the two updates happen, the latter one would find that the
value of the ref before it is updated has changed from what the code
expects.

When the user asks to update the underlying ref via the symbolic ref
explicitly without using a wildcard refspec, e.g. "git fetch $there
refs/heads/master:refs/remotes/origin/HEAD", we should still let him
do so, but when expanding wildcard refs, it will result in a more
intuitive outcome if we simply ignore local symbolic refs.

As the purpose of the symbolic ref refs/remotes/origin/HEAD is to
follow the ref it points at (e.g. refs/remotes/origin/master), its
value would change when the underlying ref is updated.

Earlier commit da3efdb (receive-pack: detect aliased updates which
can occur with symrefs, 2010-04-19) fixed a similar issue for "git
push".

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * This time with minimal tests and an updated log message.

 remote.c                     | 13 ++++++++++++-
 t/t5535-fetch-push-symref.sh | 42 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+), 1 deletion(-)
 create mode 100755 t/t5535-fetch-push-symref.sh

diff --git a/remote.c b/remote.c
index 04fd9ea..a72748c 100644
--- a/remote.c
+++ b/remote.c
@@ -1370,6 +1370,16 @@ int branch_merge_matches(struct branch *branch,
 	return refname_match(branch->merge[i]->src, refname, ref_fetch_rules);
 }
 
+static int ignore_symref_update(const char *refname)
+{
+	unsigned char sha1[20];
+	int flag;
+
+	if (!resolve_ref_unsafe(refname, sha1, 0, &flag))
+		return 0; /* non-existing refs are OK */
+	return (flag & REF_ISSYMREF);
+}
+
 static struct ref *get_expanded_map(const struct ref *remote_refs,
 				    const struct refspec *refspec)
 {
@@ -1383,7 +1393,8 @@ static struct ref *get_expanded_map(const struct ref *remote_refs,
 		if (strchr(ref->name, '^'))
 			continue; /* a dereference item */
 		if (match_name_with_pattern(refspec->src, ref->name,
-					    refspec->dst, &expn_name)) {
+					    refspec->dst, &expn_name) &&
+		    !ignore_symref_update(expn_name)) {
 			struct ref *cpy = copy_ref(ref);
 
 			cpy->peer_ref = alloc_ref(expn_name);
diff --git a/t/t5535-fetch-push-symref.sh b/t/t5535-fetch-push-symref.sh
new file mode 100755
index 0000000..8ed58d2
--- /dev/null
+++ b/t/t5535-fetch-push-symref.sh
@@ -0,0 +1,42 @@
+#!/bin/sh
+
+test_description='avoiding conflicting update thru symref aliasing'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	test_commit one &&
+	git clone . src &&
+	git clone src dst1 &&
+	git clone src dst2 &&
+	test_commit two &&
+	( cd src && git pull )
+'
+
+test_expect_success 'push' '
+	(
+		cd src &&
+		git push ../dst1 "refs/remotes/*:refs/remotes/*"
+	) &&
+	git ls-remote src "refs/remotes/*" >expect &&
+	git ls-remote dst1 "refs/remotes/*" >actual &&
+	test_cmp expect actual &&
+	( cd src && git symbolic-ref refs/remotes/origin/HEAD ) >expect &&
+	( cd dst1 && git symbolic-ref refs/remotes/origin/HEAD ) >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'fetch' '
+	(
+		cd dst2 &&
+		git fetch ../src "refs/remotes/*:refs/remotes/*"
+	) &&
+	git ls-remote src "refs/remotes/*" >expect &&
+	git ls-remote dst2 "refs/remotes/*" >actual &&
+	test_cmp expect actual &&
+	( cd src && git symbolic-ref refs/remotes/origin/HEAD ) >expect &&
+	( cd dst2 && git symbolic-ref refs/remotes/origin/HEAD ) >actual &&
+	test_cmp expect actual
+'
+
+test_done
-- 
1.8.1.rc1.128.gd8d1528

Re: [PATCH] fetch: ignore wildcarded refspecs that update local symbolic refs

[Jay Soffian]

On Tue, Dec 11, 2012 at 5:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
> In a repository cloned from somewhere else, you typically have a
> [...]
>  * This time with minimal tests and an updated log message.

Sorry I haven't been reading the list much lately, so I don't know the
context which motivated this patch beyond the commit message. But,
this message is quite clear. So:

Acked-by: Jay Soffian

> diff --git a/remote.c b/remote.c
> index 04fd9ea..a72748c 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -1370,6 +1370,16 @@ int branch_merge_matches(struct branch *branch,
>         return refname_match(branch->merge[i]->src, refname, ref_fetch_rules);
>  }
>
> +static int ignore_symref_update(const char *refname)

s/ignore_symref_update/is_existing_symref/ ?

j.

Re: [RFC/PATCH] ignoring a fetch that overwrites local symref

[Shawn Pearce]

On Tue, Dec 11, 2012 at 11:46 AM, Junio C Hamano <gitster@pobox.com> wrote:
> This is a companion to an ancient thread
>
>     http://thread.gmane.org/gmane.comp.version-control.git/145311/focus=145337
>
> in which an error was dealt with while pushing into a "mirror"
> repository that has a symbolic reference refs/remotes/origin/HEAD
> pointing at refs/remotes/origin/master with "git push --mirror".
> The issue was that the receiving end was told to update origin/HEAD
> and origin/master separately; if origin/HEAD is updated, that would
> update origin/master at the same time, and then when attempting to
> update origin/master, it would notice that it no longer has the
> expected old value and barf.  After the series, we started ignoring
> such pushes to HEAD on the receiving end.
>
> But you can suffer from a similar issue transferring objects in the
> opposite direction.  If you run "fetch --mirror" in to such a
> "mirror" repository, the other side would advertise both 'master'
> and 'HEAD' under refs/remotes/origin/ hierarchy, and refs/*:refs/*
> wildcard would try to grab both of them.
>
> Work it around by noticing a wildcard match that attempts to update
> a local symbolic ref and ignoring it.

At what point should we just support symrefs on the protocol? :-(

Re: [RFC/PATCH] ignoring a fetch that overwrites local symref

[Junio C Hamano]

Shawn Pearce <spearce@spearce.org> writes:

>> Work it around by noticing a wildcard match that attempts to update
>> a local symbolic ref and ignoring it.
>
> At what point should we just support symrefs on the protocol? :-(

I think it is entirely an orthogonal matter.  When we learn that the
other side now has this ref as a symref pointing to this other ref,
an update of the local ref on the RHS of the refspec that has such a
symref on its LHS will not be using the current codepath to call
update_ref() to write the object name thru an existing symref.