threads / discuss / 28283

[PATCHv2 1/2] remote: write correct fetch spec when renaming remote 'remote'

Subject: [PATCHv2 1/2] remote: write correct fetch spec when renaming remote 'remote'

## tl;dr

10 messages between Sep 2, 2011 and Sep 10, 2011.

replies: 9people: 3as markdown or json

Martin von Zweigbergk· Sep 2, 2011, 00:50 UTC · lore

When renaming a remote whose name is contained in a configured fetch refspec for that remote, we currently replace the first occurrence of the remote name in the refspec. This is correct in most cases, but breaks if the remote name occurs in the fetch refspec before the expected place. For example, we currently change

[remote "remote"]
	url = git://git.kernel.org/pub/scm/git/git.git
	fetch = +refs/heads/*:refs/remotes/remote/*
into
[remote "origin"]
	url = git://git.kernel.org/pub/scm/git/git.git
	fetch = +refs/heads/*:refs/origins/remote/*

Reduce the risk of changing incorrect sections of the refspec by matching the entire ":refs/remotes/<name>/" instead of just "<name>".

Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---
Now matches more strictly, namely on ":/refs/remotes/$OLD/".
 builtin/remote.c  |   12 ++++++++----
 t/t5505-remote.sh |   20 ++++++++++++++++++++
 2 files changed, 28 insertions(+), 4 deletions(-)
diff --git a/builtin/remote.c b/builtin/remote.c
index f2a9c26..6d08738 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -621,7 +621,8 @@ static int mv(int argc, const char **argv)
 		OPT_END()
 	};
 	struct remote *oldremote, *newremote;
-	struct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT, buf3 = STRBUF_INIT;
+	struct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT, buf3 = STRBUF_INIT,
+		old_remote_context = STRBUF_INIT;
 	struct string_list remote_branches = STRING_LIST_INIT_NODUP;
 	struct rename_info rename;
 	int i;
@@ -659,15 +660,18 @@ static int mv(int argc, const char **argv)
 	strbuf_addf(&buf, "remote.%s.fetch", rename.new);
 	if (git_config_set_multivar(buf.buf, NULL, NULL, 1))
 		return error("Could not remove config section '%s'", buf.buf);
+	strbuf_addf(&old_remote_context, ":refs/remotes/%s/", rename.old);
 	for (i = 0; i < oldremote->fetch_refspec_nr; i++) {
 		char *ptr;
 
 		strbuf_reset(&buf2);
 		strbuf_addstr(&buf2, oldremote->fetch_refspec[i]);
-		ptr = strstr(buf2.buf, rename.old);
+		ptr = strstr(buf2.buf, old_remote_context.buf);
 		if (ptr)
-			strbuf_splice(&buf2, ptr-buf2.buf, strlen(rename.old),
-					rename.new, strlen(rename.new));
+			strbuf_splice(&buf2,
+				      ptr-buf2.buf + strlen(":refs/remotes/"),
+				      strlen(rename.old), rename.new,
+				      strlen(rename.new));
 		if (git_config_set_multivar(buf.buf, buf2.buf, "^$", 0))
 			return error("Could not append '%s'", buf.buf);
 	}
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 0d0222e..36c807c 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -631,6 +631,26 @@ test_expect_success 'rename a remote' '
 
 '
 
+test_expect_success 'rename does not update a non-default fetch refspec' '
+
+	git clone one four.one &&
+	(cd four.one &&
+	 git config remote.origin.fetch +refs/heads/*:refs/heads/origin/* &&
+	 git remote rename origin upstream &&
+	 test "$(git config remote.upstream.fetch)" = "+refs/heads/*:refs/heads/origin/*")
+
+'
+
+test_expect_success 'rename a remote with name part of fetch spec' '
+
+	git clone one four.two &&
+	(cd four.two &&
+	 git remote rename origin remote &&
+	 git remote rename remote upstream &&
+	 test "$(git config remote.upstream.fetch)" = "+refs/heads/*:refs/remotes/upstream/*")
+
+'
+
 cat > remotes_origin << EOF
 URL: $(pwd)/one
 Push: refs/heads/master:refs/heads/upstream

-- 
1.7.6.51.g07e0e
Martin von Zweigbergk· Sep 2, 2011, 00:50 UTC · re: Martin von Zweigbergk · lore

[PATCHv2 2/2] remote: "rename o foo" should not rename ref "origin/bar"

When renaming a remote called 'o' using 'git remote rename o foo', git should also rename any remote-tracking branches for the remote. This does happen, but any remote-tracking branches starting with 'refs/remotes/o', such as 'refs/remotes/origin/bar', will also be renamed (to 'refs/remotes/foorigin/bar' in this case).

Fix it by simply matching one more character, up to the slash following the remote name.

Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---
No changes since v1.
 builtin/remote.c  |    2 +-
 t/t5505-remote.sh |   10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletions(-)
diff --git a/builtin/remote.c b/builtin/remote.c
index 6d08738..0df6ab0 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -570,7 +570,7 @@ static int read_remote_branches(const char *refname,
 	unsigned char orig_sha1[20];
 	const char *symref;
 
-	strbuf_addf(&buf, "refs/remotes/%s", rename->old);
+	strbuf_addf(&buf, "refs/remotes/%s/", rename->old);
 	if (!prefixcmp(refname, buf.buf)) {
 		item = string_list_append(rename->remote_branches, xstrdup(refname));
 		symref = resolve_ref(refname, orig_sha1, 1, &flag);
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 36c807c..15186c8 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -651,6 +651,16 @@ test_expect_success 'rename a remote with name part of fetch spec' '
 
 '
 
+test_expect_success 'rename a remote with name prefix of other remote' '
+
+	git clone one four.three &&
+	(cd four.three &&
+	 git remote add o git://example.com/repo.git &&
+	 git remote rename o upstream &&
+	 test "$(git rev-parse origin/master)" = "$(git rev-parse master)")
+
+'
+
 cat > remotes_origin << EOF
 URL: $(pwd)/one
 Push: refs/heads/master:refs/heads/upstream

-- 
1.7.6.51.g07e0e
Jeff King· Sep 2, 2011, 16:03 UTC · re: Martin von Zweigbergk · lore

Re: [PATCHv2 1/2] remote: write correct fetch spec when renaming remote 'remote'

On Thu, Sep 01, 2011 at 08:50:33PM -0400, Martin von Zweigbergk wrote:
Show 17 quoted lines
> +	strbuf_addf(&old_remote_context, ":refs/remotes/%s/", rename.old);
>  	for (i = 0; i < oldremote->fetch_refspec_nr; i++) {
>  		char *ptr;
>  
>  		strbuf_reset(&buf2);
>  		strbuf_addstr(&buf2, oldremote->fetch_refspec[i]);
> -		ptr = strstr(buf2.buf, rename.old);
> +		ptr = strstr(buf2.buf, old_remote_context.buf);
>  		if (ptr)
> -			strbuf_splice(&buf2, ptr-buf2.buf, strlen(rename.old),
> -					rename.new, strlen(rename.new));
> +			strbuf_splice(&buf2,
> +				      ptr-buf2.buf + strlen(":refs/remotes/"),
> +				      strlen(rename.old), rename.new,
> +				      strlen(rename.new));
>  		if (git_config_set_multivar(buf.buf, buf2.buf, "^$", 0))
>  			return error("Could not append '%s'", buf.buf);

If we find a refspec that does not match this pattern, what should we do? This code silently ignores it.

At the very least, I'd like to see a warning indicating that one or more fetch refspecs were not rewritten, and that the user should update the config manually.

What should happen if there are multiple refspecs, and one doesn't match? Should we rewrite the ones we can, or should we abort and not rewrite anything at all? I think the latter is probably less confusing.

These are corner cases, obviously, and most people will just have the stock refspecs or something that follows their pattern.

-Peff
Martin von Zweigbergk· Sep 3, 2011, 15:26 UTC · re: Jeff King · lore

[PATCHv2 3/2] remote rename: warn when refspec was not updated

When renaming a remote, we also try to update the fetch refspec accordingly, but only if it has the default format. For others, such as refs/heads/master:refs/heads/origin, we are conservative and leave it untouched. Let's give the user a warning about refspecs that are not updated, so he can manually update the config if necessary.

Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---

This addresses the first part of your email. I'm not sure what should happen when only some refspecs can be updated. I can't think of any sensible examples with mixed default and non-default refspecs, so for me it's hard to see whether we would make things better or worse by making it all-or-nothing. I have more or less always used the default ones myself. Do you have any good examples?

 builtin/remote.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/builtin/remote.c b/builtin/remote.c
index 0df6ab0..61326cb 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -672,6 +672,12 @@ static int mv(int argc, const char **argv)
 				      ptr-buf2.buf + strlen(":refs/remotes/"),
 				      strlen(rename.old), rename.new,
 				      strlen(rename.new));
+		else
+			warning("Not updating non-default fetch respec\n"
+				"\t%s\n"
+				"\tPlease update the configuration manually if necessary.",
+				buf2.buf);
+
 		if (git_config_set_multivar(buf.buf, buf2.buf, "^$", 0))
 			return error("Could not append '%s'", buf.buf);
 	}

-- 
1.7.6.51.g07e0e
Junio C Hamano· Sep 6, 2011, 22:58 UTC · re: Martin von Zweigbergk · lore

Re: [PATCHv2 1/2] remote: write correct fetch spec when renaming remote 'remote'

Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:
Show 22 quoted lines
> @@ -659,15 +660,18 @@ static int mv(int argc, const char **argv)
>  	strbuf_addf(&buf, "remote.%s.fetch", rename.new);
>  	if (git_config_set_multivar(buf.buf, NULL, NULL, 1))
>  		return error("Could not remove config section '%s'", buf.buf);
> +	strbuf_addf(&old_remote_context, ":refs/remotes/%s/", rename.old);
>  	for (i = 0; i < oldremote->fetch_refspec_nr; i++) {
>  		char *ptr;
>  
>  		strbuf_reset(&buf2);
>  		strbuf_addstr(&buf2, oldremote->fetch_refspec[i]);
> -		ptr = strstr(buf2.buf, rename.old);
> +		ptr = strstr(buf2.buf, old_remote_context.buf);
>  		if (ptr)
> -			strbuf_splice(&buf2, ptr-buf2.buf, strlen(rename.old),
> -					rename.new, strlen(rename.new));
> +			strbuf_splice(&buf2,
> +				      ptr-buf2.buf + strlen(":refs/remotes/"),
> +				      strlen(rename.old), rename.new,
> +				      strlen(rename.new));
>  		if (git_config_set_multivar(buf.buf, buf2.buf, "^$", 0))
>  			return error("Could not append '%s'", buf.buf);
>  	}

It is somewhat bothering that we do not say "we didn't do any magic" here when we did not move the tracking branch specifications, but that is not a new problem, so I am OK with this change.

I however suspect that you would want to keep the record of what you changed here, so that the renaming of actual refs done in [PATCH 2/2] is limited to those that you updated the specifications for, no?

Martin von Zweigbergk· Sep 8, 2011, 01:40 UTC · re: Junio C Hamano · lore

Re: [PATCHv2 1/2] remote: write correct fetch spec when renaming remote 'remote'

On Tue, 6 Sep 2011, Junio C Hamano wrote:
> It is somewhat bothering that we do not say "we didn't do any magic" here
> when we did not move the tracking branch specifications, but that is not a
> new problem, so I am OK with this change.

If I understand you correctly, this is the same concern that Jeff had and that I tried to address in patch 3/2.

> I however suspect that you would want to keep the record of what you
> changed here, so that the renaming of actual refs done in [PATCH 2/2] is
> limited to those that you updated the specifications for, no?

Sorry, I don't think I really understand. Are you worried that we might rename too many refs, i.e. unrelated ones? We match exactly the same pattern both when updating refspecs and when renaming refs. Of course, we can never be certain that a ref "refs/remotes/origin/foo" is really related to the remote called "origin". The user could have simply created the ref manually. Is that what you are getting at?

Martin
Junio C Hamano· Sep 8, 2011, 03:43 UTC · re: Martin von Zweigbergk · lore

Re: [PATCHv2 1/2] remote: write correct fetch spec when renaming remote 'remote'

Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:
> same pattern both when updating refspecs and when renaming refs. Of
> course, we can never be certain that a ref "refs/remotes/origin/foo"
> is really related to the remote called "origin". The user could have
> simply created the ref manually. Is that what you are getting at?

You have two separate and independent code that are not linked together but should logically be.

One updates fetch refspec whose RHS is "refs/remotes/$OLD/<anything>" to "refs/remotes/$NEW/<the same thing>". If you do not find any such fetch refspec, then you do not update these configuration variables, which is good.

Later in the same mv() function, the other one renames refs/remotes/$OLD/ to refs/remotes/$NEW/, even when you did not find any fetch refspec that stores under "refs/remotes/$OLD/<anything>" in the earlier logic.

Now, these actual refs may have been placed manually by the user. They may have been placed by an old config that the user may have edited. You simply do not know.

But you know one thing. You _do_ know is that these refs did _not_ come from any "[remote "$OLD"] fetch = ..." configuration, and by inference, it will not come from any "[remote "$NEW"] fetch = ...", in other words, they do not have any relation with the "$NEW" remote. So I do not see a good reason to move them from refs/remotes/$OLD/ to refs/remotes/$NEW/. That was what I was pointing out.

Martin von Zweigbergk· Sep 8, 2011, 09:08 UTC · re: Junio C Hamano · lore

Re: [PATCHv2 1/2] remote: write correct fetch spec when renaming remote 'remote'

On Wed, 7 Sep 2011, Junio C Hamano wrote:
> Later in the same mv() function, the other one renames refs/remotes/$OLD/
> to refs/remotes/$NEW/, even when you did not find any fetch refspec that
> stores under "refs/remotes/$OLD/<anything>" in the earlier logic.

Ah, of course. Sorry for being slow and thanks for the explanation. So if there are two configured refspecs with RHSs "refs/remotes/$OLD/foo" and "refs/remotes/$OLD/bar/*", we should remember that we updated those and only update refs that match the same patterns. I will see what I can do and will hopefully soon get back with a "patch 4/2".

Martin
Junio C Hamano· Sep 8, 2011, 16:46 UTC · re: Martin von Zweigbergk · lore

Re: [PATCHv2 1/2] remote: write correct fetch spec when renaming remote 'remote'

Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:
Show 11 quoted lines
> On Wed, 7 Sep 2011, Junio C Hamano wrote:
>
>> Later in the same mv() function, the other one renames refs/remotes/$OLD/
>> to refs/remotes/$NEW/, even when you did not find any fetch refspec that
>> stores under "refs/remotes/$OLD/<anything>" in the earlier logic.
>
> Ah, of course. Sorry for being slow and thanks for the explanation. So
> if there are two configured refspecs with RHSs "refs/remotes/$OLD/foo"
> and "refs/remotes/$OLD/bar/*", we should remember that we updated
> those and only update refs that match the same patterns. I will see
> what I can do and will hopefully soon get back with a "patch 4/2".

What I had in mind was nothing that elaborate--it would be sufficiently safe to just remember if you replaced any in the config, and if you didn't, not to move anything.

Martin von Zweigbergk· Sep 10, 2011, 19:39 UTC · re: Junio C Hamano · lore

[PATCH 4/2] remote: only update remote-tracking branch if updating refspec

'git remote rename' will only update the remote's fetch refspec if it looks like a default one. If the remote has no default fetch refspec, as in

[remote "origin"]
    url = git://git.kernel.org/pub/scm/git/git.git
    fetch = +refs/heads/*:refs/remotes/upstream/*

we would not update the fetch refspec and even if there is a ref called "refs/remotes/origin/master", we should not rename it, since it was not created by fetching from the remote.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---
Some questions on style:
1. Should I wrap the statement in "else" block in braces when "then"
   block has braces? I couldn't find anything conclusive by looking at
   existing code.
2. Is it ok (as I did) to return from the function prematurely even
   though it is not an error case? This will of course make the
   function more brittle when it comes to adding code at the end of
   it. Would you prefer to invert the condition and put the remainder
   of the function inside the "then" block? Or even to extract the
   code into a new function?
 builtin/remote.c  |   10 +++++++---
 t/t5505-remote.sh |    3 ++-
 2 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/builtin/remote.c b/builtin/remote.c
index 61326cb..b25dfb4 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -625,7 +625,7 @@ static int mv(int argc, const char **argv)
 		old_remote_context = STRBUF_INIT;
 	struct string_list remote_branches = STRING_LIST_INIT_NODUP;
 	struct rename_info rename;
-	int i;
+	int i, refspec_updated = 0;
 
 	if (argc != 3)
 		usage_with_options(builtin_remote_rename_usage, options);
@@ -667,12 +667,13 @@ static int mv(int argc, const char **argv)
 		strbuf_reset(&buf2);
 		strbuf_addstr(&buf2, oldremote->fetch_refspec[i]);
 		ptr = strstr(buf2.buf, old_remote_context.buf);
-		if (ptr)
+		if (ptr) {
+			refspec_updated = 1;
 			strbuf_splice(&buf2,
 				      ptr-buf2.buf + strlen(":refs/remotes/"),
 				      strlen(rename.old), rename.new,
 				      strlen(rename.new));
-		else
+		} else
 			warning("Not updating non-default fetch respec\n"
 				"\t%s\n"
 				"\tPlease update the configuration manually if necessary.",
@@ -695,6 +696,9 @@ static int mv(int argc, const char **argv)
 		}
 	}
 
+	if (!refspec_updated)
+		return 0;
+
 	/*
 	 * First remove symrefs, then rename the rest, finally create
 	 * the new symrefs.
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 15186c8..e8af615 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -637,7 +637,8 @@ test_expect_success 'rename does not update a non-default fetch refspec' '
 	(cd four.one &&
 	 git config remote.origin.fetch +refs/heads/*:refs/heads/origin/* &&
 	 git remote rename origin upstream &&
-	 test "$(git config remote.upstream.fetch)" = "+refs/heads/*:refs/heads/origin/*")
+	 test "$(git config remote.upstream.fetch)" = "+refs/heads/*:refs/heads/origin/*" &&
+	 git rev-parse -q origin/master)
 
 '
 

-- 
1.7.7.rc0.317.gd0afe

← back to recent threads