threads / patch / 59124

patchfetch: fix duplicate remote parallel fetch bug

Subject: [PATCH] fetch: fix duplicate remote parallel fetch bug

## tl;dr

3 messages between Jan 19, 2023 and Jan 19, 2023. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Calvin Wan· Jan 19, 2023, 22:05 UTC · lore

Fetching in parallel from a remote group with a duplicated remote results in the following:

error: cannot lock ref '<ref>': is at <oid> but expected <oid>

This doesn't happen in serial since fetching from the same remote that has already been fetched from is a noop. Therefore, remove any duplicated remotes after remote groups are parsed.

Signed-off-by: Calvin Wan <calvinwan@google.com>
---
 builtin/fetch.c          | 1 +
 t/t5506-remote-groups.sh | 9 +++++++++
 2 files changed, 10 insertions(+)

Show changes to 2 files +10 −0

builtin/fetch.c, t/t5506-remote-groups.sh

diff --git a/builtin/fetch.c b/builtin/fetch.c
index b06e454cbd..508ab2670c 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -2225,6 +2225,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 			argv++;
 		}
 	}
+	string_list_remove_duplicates(&list, 0);
 
 	if (negotiate_only) {
 		struct oidset acked_commits = OIDSET_INIT;
diff --git a/t/t5506-remote-groups.sh b/t/t5506-remote-groups.sh
index 5bac03ede8..0e176175a3 100755
--- a/t/t5506-remote-groups.sh
+++ b/t/t5506-remote-groups.sh
@@ -99,4 +99,13 @@ test_expect_success 'updating remote name updates that remote' '
 	! repo_fetched two
 '
 
+test_expect_success 'updating group in parallel with a duplicate remote does not fail (fetch)' '
+	mark fetch-group-duplicate &&
+	update_repo one &&
+	git config --add remotes.duplicate one &&
+	git config --add remotes.duplicate one &&
+	git -c fetch.parallel=2 remote update duplicate &&
+	repo_fetched one
+'
+
 test_done

-- 
2.39.0.246.g2a6d74b583-goog

Junio C Hamano· Jan 19, 2023, 22:40 UTC · re: Calvin Wan · lore

Re: [PATCH] fetch: fix duplicate remote parallel fetch bug

Calvin Wan <calvinwan@google.com> writes:

Show 24 quoted lines

> Fetching in parallel from a remote group with a duplicated remote results
> in the following:
>
> error: cannot lock ref '<ref>': is at <oid> but expected <oid>
>
> This doesn't happen in serial since fetching from the same remote that
> has already been fetched from is a noop. Therefore, remove any duplicated
> remotes after remote groups are parsed.
>
> Signed-off-by: Calvin Wan <calvinwan@google.com>
> ---
>  builtin/fetch.c          | 1 +
>  t/t5506-remote-groups.sh | 9 +++++++++
>  2 files changed, 10 insertions(+)
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index b06e454cbd..508ab2670c 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -2225,6 +2225,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
>  			argv++;
>  		}
>  	}
> +	string_list_remove_duplicates(&list, 0);

As it always is possible to edit .git/config manually, it is necessary to perform deduplication like this patch does on the consumer side of the list, but do you know if our tool create duplication, or is it entirely something the end-user does manually?

If it is the former, I am wondering if we should also fix such a code path that does so in the first place.

Will queue.  Thanks.

Calvin Wan· Jan 19, 2023, 23:44 UTC · re: Junio C Hamano · lore

Re: [PATCH] fetch: fix duplicate remote parallel fetch bug

> As it always is possible to edit .git/config manually, it is
> necessary to perform deduplication like this patch does on the
> consumer side of the list, but do you know if our tool create
> duplication, or is it entirely something the end-user does manually?

I checked git-remote and there is protection against duplication there, but I'm unsure if there are other places where remotes are being added/renamed. I discovered the bug initially by using git-config.