threads / patch / 28274

patch, 2 partsremote: write correct fetch spec when renaming remote 'remote'

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

## tl;dr

8 messages between Sep 1, 2011 and Sep 2, 2011. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Martin von Zweigbergk· Sep 1, 2011, 01: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 requiring the string to be matched to have leading and trailing slashes, i.e. match "/<name>/" instead of just "<name>".

We could have required even a leading ":refs/remotes/", but that would mean that we would limit the types of refspecs we could help the user update.

Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---
 builtin/remote.c  |   12 ++++++++----
 t/t5505-remote.sh |   20 ++++++++++++++++++++
 2 files changed, 28 insertions(+), 4 deletions(-)
Show changes to 2 files +28 −4

builtin/remote.c, t/t5505-remote.sh

diff --git a/builtin/remote.c b/builtin/remote.c
index f2a9c26..c1763ed 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, "/%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 + 1,
+				      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..7b6f443 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 a remote with 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/upstream/*")
+
+'
+
+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 1, 2011, 01:50 UTC · re: Martin von Zweigbergk · lore

[PATCH 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>
---
 builtin/remote.c  |    2 +-
 t/t5505-remote.sh |   10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletions(-)
Show changes to 2 files +11 −1

builtin/remote.c, t/t5505-remote.sh

diff --git a/builtin/remote.c b/builtin/remote.c
index c1763ed..127cff4 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 7b6f443..57b584c 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 1, 2011, 02:46 UTC · re: Martin von Zweigbergk · lore

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

On Wed, Aug 31, 2011 at 09:50:43PM -0400, Martin von Zweigbergk wrote:
Show 5 quoted lines
> 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).

To be totally correct, shouldn't this check each ref against the RHS of the remote's old refspec, and rename it according to the remote's new refspec?

Maybe that is just being pedantic, though. This should work fine with the default config[1].

-Peff
[1] Since this part of the renaming process obviously depends heavily on
    refs/remotes/$OLD being the naming convention, shouldn't the
    renaming of the refspecs do the same thing? I.e., it's another
    reason that your patch 1/2 should only tweak refs/remotes/$OLD.
    Otherwise you will get renamed refspecs, but the actual refs won't
    be moved.
Martin von Zweigbergk· Sep 2, 2011, 00:35 UTC · re: Jeff King · lore

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

On Wed, 31 Aug 2011, Jeff King wrote:
Show 11 quoted lines
> On Wed, Aug 31, 2011 at 09:50:43PM -0400, Martin von Zweigbergk wrote:
> 
> > 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).
> 
> To be totally correct, shouldn't this check each ref against the RHS of
> the remote's old refspec, and rename it according to the remote's new
> refspec?

That's what I thought too, but was planning on leaving it for a separate patch. However, after changing patch 1 to only update the fetch refspecs from "refs/remotes/$OLD" to "refs/remotes/$NEW", there is no other place in the fetchspec where a remote name can occur and 'git remote rename' still understands it. So since we're now being more conservative about updating refspecs, I guess we need to be equally conservative about updating ref names.

Martin
Jeff King· Sep 2, 2011, 15:55 UTC · re: Martin von Zweigbergk · lore

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

On Thu, Sep 01, 2011 at 08:35:01PM -0400, Martin von Zweigbergk wrote:
Show 11 quoted lines
> > To be totally correct, shouldn't this check each ref against the RHS of
> > the remote's old refspec, and rename it according to the remote's new
> > refspec?
> 
> That's what I thought too, but was planning on leaving it for a
> separate patch. However, after changing patch 1 to only update the
> fetch refspecs from "refs/remotes/$OLD" to "refs/remotes/$NEW", there
> is no other place in the fetchspec where a remote name can occur and
> 'git remote rename' still understands it. So since we're now being
> more conservative about updating refspecs, I guess we need to be
> equally conservative about updating ref names.

Yeah, I think with the change in patch 1, your patch 2 is very reasonable. It is admitting that "git remote" is really about tweaking the default remote config, and punting on config that doesn't appear to follow it. And doing it consistently. I don't think there's nothing wrong with that.

-Peff
Jeff King· Sep 1, 2011, 02:42 UTC · re: Martin von Zweigbergk · lore

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

On Wed, Aug 31, 2011 at 09:50:42PM -0400, Martin von Zweigbergk wrote:
Show 15 quoted lines
> 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/*
Oops.
> Reduce the risk of changing incorrect sections of the refspec by
> requiring the string to be matched to have leading and trailing
> slashes, i.e. match "/<name>/" instead of just "<name>".
Doesn't this just mean that:
  git remote rename remotes foo
will break in the same way?
> We could have required even a leading ":refs/remotes/", but that would
> mean that we would limit the types of refspecs we could help the user
> update.
Actually, I think it's better to be more conservative, and rename only:
  refs/remotes/$OLD/
into
  refs/remotes/$NEW/

If we are tweaking the refspecs, it's because we assume that the refspec follows a certain naming convention (i.e., the one we set up with "git remote add"). If somebody has tweaked this to be:

  refs/heads/$OLD/*
or even:
  refs/heads/foo/$OLD/bar

then we are just guessing about what they want. And I suspect such a person would not use "git remote rename", anyway, but would instead edit the config themselves. I'd rather make it safe for people using the default config, and people who want to stray from that can deal with updating the config themselves (since they would have to have done so to get into that state in the first place).

Maybe we should even print a warning in that case.
-Peff
Junio C Hamano· Sep 1, 2011, 03:18 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 7 quoted lines
> Actually, I think it's better to be more conservative, and rename only:
>
>   refs/remotes/$OLD/
>
> into
>
>   refs/remotes/$NEW/
Thanks.

I wholeheartedly agree with this. It would let me sleep better if we did this rewriting only when remote.$name.fetch is set exactly to its default value, "+refs/heads/*:refs/origin/$name/*" (it is Ok if the leading '+' is missing, though), and we didn't touch either config nor the existing tracking refs at all otherwise, and gave warning saying we didn't do anything to them.

Martin von Zweigbergk· Sep 2, 2011, 00:02 UTC · re: Jeff King · lore

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

On Wed, 31 Aug 2011, Jeff King wrote:
Show 11 quoted lines
> On Wed, Aug 31, 2011 at 09:50:42PM -0400, Martin von Zweigbergk wrote:
> 
> > Reduce the risk of changing incorrect sections of the refspec by
> > requiring the string to be matched to have leading and trailing
> > slashes, i.e. match "/<name>/" instead of just "<name>".
> 
> Doesn't this just mean that:
> 
>   git remote rename remotes foo
> 
> will break in the same way?

Yes, "r", "ead" and such now work and only exactly "refs", "heads" and "remotes" don't work. Sorry, forgot to mention that in the commit message.

Show 5 quoted lines
> > We could have required even a leading ":refs/remotes/", but that would
> > mean that we would limit the types of refspecs we could help the user
> > update.
> 
> Actually, I think it's better to be more conservative [...]

I think you are right. That's what I did (match all the way from "refs/...") at first, but then I thought that maybe the reason the matching was relaxed was to allow non-default refspecs. However, as you point out in the footnote to your reply to PATCH 2/2, that case is not working consistently anyway, so it seems safe (w.r.t. backwards compatibility too) to ignore it. Will switch back to matching ":refs/remotes/$OLD".

Martin

← back to recent threads