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:

+	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