Re: [PATCH] git-remote rename should match whole string when renaming remote ref directory
- From
- Benny Halevy <bhalevy@tonian.com>
- Date
- Sep 27, 2011, 09:38 UTC
- Message-ID
- <4E8199A6.5010804@tonian.com>
- In-Reply-To
- <7voby6y0vs.fsf@alter.siamese.dyndns.org>
On 2011-09-27 09:07, Junio C Hamano wrote:
Show 22 quoted lines
> 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.
Show 16 quoted lines
>> 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
Show 22 quoted lines
> > 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 ;-)