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

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

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

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

[Jeff King]

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

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

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

[Martin von Zweigbergk]

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

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

[Junio C Hamano]

Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:

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

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

[Martin von Zweigbergk]

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

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

[Junio C Hamano]

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.

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

[Martin von Zweigbergk]

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

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

[Junio C Hamano]

Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:

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

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

[Martin von Zweigbergk]

'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