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

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

[Martin von Zweigbergk]

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

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

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

[Martin von Zweigbergk]

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

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

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

[Jeff King]

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?

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.

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

[Martin von Zweigbergk]

On Wed, 31 Aug 2011, Jeff King wrote:

> 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

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

[Jeff King]

On Thu, Sep 01, 2011 at 08:35:01PM -0400, Martin von Zweigbergk wrote:

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

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

[Jeff King]

On Wed, Aug 31, 2011 at 09:50:42PM -0400, Martin von Zweigbergk wrote:

> 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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

[Martin von Zweigbergk]

On Wed, 31 Aug 2011, Jeff King wrote:

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

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