Re: [PATCH] git-remote rename should match whole string when renaming remote ref directory

[Benny Halevy <bhalevy@tonian.com>]

On 2011-09-27 09:07, Junio C Hamano wrote:
> Benny Halevy <bhalevy@tonian.com> writes:
> 
>> On 2011-09-26 21:04, Junio C Hamano wrote:
>>> Benny Halevy <benny@tonian.com> writes:
>>> ...
>>>> -	if (!prefixcmp(refname, buf.buf)) {
>>>> +	if (!strcmp(refname, buf.buf)) {
>>>
>>> At this point of the code, refname has "refs/remotes/test/foo" and it is
>>> queued to later rename it to "refs/remotes/test-/foo" (the next invocation
>>> of this function will see "refs/remotes/test/bar" in refname). And the
>>> strbuf buf.buf has "refs/remotes/test"; your !strcmp(refname, buf.buf)
>>> would never trigger, I suspect.
>>>
>>> Isn't 60e5eee (remote: "rename o foo" should not rename ref "origin/bar",
>>> 2011-09-01) the correct fix for this issue?...
>>
>> OK, 60e5eee solves the problem too.
> 
> Hmm, what do you mean by "too" here?  Martin's patch fixes the issue, but
> does yours, too?
> 

correct.

>> FWIW, here's the test I used:
>> ...
>> cd main || fail cd main failed
>> for i in test test-2; do
>> 	$git remote add $i file://$cwd/$i || fail git remote add $i failed
>> done
>> $git remote update || fail git remote update fail
>> $git remote rename test test-
>> $git show test-2/master || fail FAILED
>> echo PASSED
> 
> Before your last "echo PASSED", add this line:
> 
> 	$git show test-/master || fail FAILED
> 
> and see what happens with your patch.

Yup, with my patch this test indeed fails, and with Martin's
it passes.

Thanks!

Benny

> 
> It is unfortunately a rather common trap to fall into, so I wouldn't blame
> you too much. People tend to concentrate only on an aspect of the problem
> that originally motivated them, and forget about the other issues that are
> equally important, if not more. In this case, you were too thrilled to see
> that your updated code no longer renames "test-2" mistakenly to "test--2",
> and you forgot that the primary task of the resulting code was to rename
> "test" to "test-" correctly. The additional line I gave you above is to
> test that.
> 
> When testing your own code, make it a habit to _always_ test both sides of
> the coin. It is somewhat difficult until you get used to it [*1*], but it
> is a skill that is really worth acquiring.
> 
> Thanks.
> 
> 
> [Footnote]
> 
> *1* ...and I do not claim that I myself never forget to fully enumerate
> other sides; even experienced people still overlook and embarrass
> themselves in public ;-)