Re: [PATCH 3/4] git-remote rename: support branches->config migration

[Junio C Hamano <gitster@pobox.com>]

Miklos Vajna <vmiklos@frugalware.org> writes:

> On Tue, Nov 11, 2008 at 04:49:14PM -0800, Junio C Hamano <gitster@pobox.com> wrote:
>> There is something fishy going on between 2/4 and 3/4.  2/4 was advertised
>> to migrate remotes to config and had a call to migrate_file() for that
>> purpose.  Here this one now allows to convert branches but there is no
>> change to the callsite of migrate_file().
>> 
>> Which would mean that 2/4 would convert branches/foo too.  And this one is
>> only to remove the leftover branches/foo file.
>> 
>> Or am I utterly confused?
>
> The trick is that 2/4 already added support for remotes/foo as it uses
> remote_get() and that detects remotes/foo as well, but that is
> completely unintentional.

That is not a trick; it merely is a broken code.

The function migrate_file() introduced by [2/4] is called for any remote
definition that did not come from config (by definition, it either came
from remotes/foo or branches/foo).  The function adds the entries for the
given remote definition to the config file, and then removes remotes/foo
file if the remote definition came from it.  So it is a logically
consistent change if you only called this function only for remote
definitions that came from remotes/foo.

But the function is called for a remote definition that originally came
from branches/foo as well.  It happily adds the definition to the config,
even though it *fails to remove* branches/foo file.

Do you still think 2/4 is a logically contained good change?

If you apply this to 5505 (taken from 3/4, but removing the check for
branches/origin file), and look at resulting t/trash*/six/.git/config
file, you will see you have already migrated the remote definition to the
config.

diff --git i/t/t5505-remote.sh w/t/t5505-remote.sh
index 1567631..60bb9e5 100755
--- i/t/t5505-remote.sh
+++ w/t/t5505-remote.sh
@@ -364,4 +364,15 @@ test_expect_success 'migrate a remote from named file in $GIT_DIR/remotes' '
 	 test "$(git config remote.origin.fetch)" = "refs/heads/master:refs/heads/origin")
 '
 
-test_done
+test_expect_success 'migrate a remote from named file in $GIT_DIR/branches' '
+	git clone one six &&
+	origin_url=$(pwd)/one &&
+	(cd six &&
+	 git remote rm origin &&
+	 echo "$origin_url" > .git/branches/origin &&
+	 git remote rename origin origin &&
+	 test "$(git config remote.origin.url)" = "$origin_url" &&
+	 test "$(git config remote.origin.fetch)" = "refs/heads/master:refs/heads/origin")
+'
+
+: test_done