threads / patch / 23484

patch, 2 partsreceive-pack: ignore duplicated commands which can occur with symrefs

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

## tl;dr

5 messages between Apr 15, 2010 and Apr 18, 2010. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

Jay Soffian· Apr 15, 2010, 20:54 UTC · lore

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

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(-)
Show changes to builtin/receive-pack.c +29 −34
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
Jay Soffian· Apr 15, 2010, 20:54 UTC · re: Jay Soffian · lore

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(-)
Show changes to 2 files +53 −2

builtin/receive-pack.c, t/t5516-fetch-push.sh

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
Junio C Hamano· Apr 15, 2010, 22:57 UTC · re: Jay Soffian · lore

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

Jay Soffian <jaysoffian@gmail.com> writes:
Show 6 quoted lines
> 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.
Show 15 quoted lines
> (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".
Show 8 quoted lines
>
> 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.

Show 6 quoted lines
> 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'
Show 18 quoted lines
> 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.
Show 13 quoted lines
> +	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.
Jay Soffian· Apr 15, 2010, 23:07 UTC · re: Junio C Hamano · lore

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

On Thu, Apr 15, 2010 at 6:57 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 7 quoted lines
> 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.
Show 7 quoted lines
> 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.
Junio C Hamano· Apr 18, 2010, 23:11 UTC · re: Jay Soffian · lore

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

Jay Soffian <jaysoffian@gmail.com> writes:
Show 8 quoted lines
> 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.

← back to recent threads