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

[PATCH 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 |   63 ++++++++++++++++++++++--------------------------
 1 files changed, 29 insertions(+), 34 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 0559fcc..3fc73cf 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,38 +486,34 @@ 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];
 		unsigned char old_sha1[20], new_sha1[20];
@@ -557,6 +551,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,10 +638,10 @@ 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;
+	struct command *cmd;
 
 	packet_buf_write(&buf, "unpack %s\n",
 			 unpack_status ? unpack_status : "ok");
@@ -667,12 +662,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 +717,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 +768,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 2/2] receive-pack: ignore duplicated commands which can occur with symrefs

[Jay Soffian]

When pushing to a remote repo, normally the sending side tries to
filter out any aliased updates (e.g, foo:baz bar:baz). However, it
is impossible 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 ef307ff6d0026900f84bae7bfe2f5d695238ca66 but expected 262cd5762e76e0aca2c185a3995095318772e2f2
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 impossible for the sending
side to unambiguously know this in all cases.

This commit fixes the issue by having receive pack ignore updates to
any symrefs IFF the target of the symref is being identically
updated.

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

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 3fc73cf..a2e3bc8 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>";
 
@@ -486,10 +487,30 @@ static void run_update_post_hook(struct command *commands)
 	}
 }
 
+static int aliased_ref(struct command *cmd, struct string_list *list)
+{
+	struct string_list_item *item;
+	unsigned char sha1[20];
+	int flag;
+
+	const char *dst_name = resolve_ref(cmd->ref_name, sha1, 0, &flag);
+
+	if (!(flag & REF_ISSYMREF))
+		return 0;
+
+	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)));
+	}
+	return 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)
@@ -505,8 +526,18 @@ static void execute_commands(struct command *commands, const char *unpacker_erro
 
 	head_name = resolve_ref("HEAD", sha1, 0, NULL);
 
-	for (cmd = commands; cmd; cmd = cmd->next)
-		cmd->error_string = update(cmd);
+	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) {
+		if (!aliased_ref(cmd, &ref_list))
+			cmd->error_string = update(cmd);
+	}
+	string_list_clear(&ref_list, 0);
 }
 
 static struct command *read_head_info(void)
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 2de98e6..c384529 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -660,6 +660,26 @@ test_expect_success 'push with branches containing #' '
 	git checkout master
 '
 
+test_expect_success 'push into aliased refs' '
+	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 --porcelain' '
 	mk_empty &&
 	echo >.git/foo  "To testrepo" &&
-- 
1.7.0.3.436.g2b878

Re: [PATCH 2/2] receive-pack: ignore duplicated commands which can occur with symrefs

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

> When pushing to a remote repo, normally the sending side tries to
> filter out any aliased updates (e.g, foo:baz bar:baz). However, it
> is impossible for the sender to know if two refs are aliased on the
> receiving side via symrefs. Here is one such scenario:
>
> $ git init origin

Indent this line a bit.

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

Consistently use prompt and indent these to match the first "init".

>
> 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 ef307ff6d0026900f84bae7bfe2f5d695238ca66 but expected 262cd5762e76e0aca2c185a3995095318772e2f2

Indent this as well, and trim the object names as the exact values do not
matter.

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

> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 3fc73cf..a2e3bc8 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>";
>  
> @@ -486,10 +487,30 @@ static void run_update_post_hook(struct command *commands)
>  	}
>  }
>  
> +static int aliased_ref(struct command *cmd, struct string_list *list)
> +{

Nit; what this does sounds more like "aliased update" to me.

> +	struct string_list_item *item;
> +	unsigned char sha1[20];
> +	int flag;
> +
> +	const char *dst_name = resolve_ref(cmd->ref_name, sha1, 0, &flag);
> +
> +	if (!(flag & REF_ISSYMREF))
> +		return 0;
> +
> +	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)));

This will also catch two symrefs that point at the same underlying ref.
If all three are updated consistently then all will be fine.  If even one
of them is inconsistent, we will try the update() and give an error
message.

We _could_ give even stronger error message to help diagnosing the
situation if we wanted to.

Very nice.

Re: [PATCH 2/2] receive-pack: ignore duplicated commands which can occur with symrefs

[Jay Soffian]

On Thu, Apr 15, 2010 at 6:57 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jay Soffian <jaysoffian@gmail.com> writes:
>
>> When pushing to a remote repo, normally the sending side tries to
>> filter out any aliased updates (e.g, foo:baz bar:baz). However, it
>> is impossible for the sender to know if two refs are aliased on the
>> receiving side via symrefs. Here is one such scenario:
>>

> [... lots of good feedback ...]

Will incorporate and re-roll.

> This will also catch two symrefs that point at the same underlying ref.
> If all three are updated consistently then all will be fine.  If even one
> of them is inconsistent, we will try the update() and give an error
> message.
>
> We _could_ give even stronger error message to help diagnosing the
> situation if we wanted to.

Okay, I'll see what I can figure out.

> Very nice.

I appreciate the compliment, but it is unexpected after reading your
reply to "failed to lock". I think your suggestion there (fix
verify_lock()) probably addresses the real problem in which case this
is just a band-aid.

j.

Re: [PATCH 2/2] receive-pack: ignore duplicated commands which can occur with symrefs

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

> On Thu, Apr 15, 2010 at 6:57 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>> Very nice.
>
> I appreciate the compliment, but it is unexpected after reading your
> reply to "failed to lock". I think your suggestion there (fix
> verify_lock()) probably addresses the real problem in which case this
> is just a band-aid.

Not necessarily.  I thought I said we may need _both_, and this is a good
change regardless.

Thanks.