public inbox for git@vger.kernel.org 
 help / color / mirror / Atom feed

From: Junio C Hamano <gitster@pobox•com>
To: Benny Halevy <bhalevy@tonian•com>
Cc: Martin von Zweigbergk <martin.von.zweigbergk@gmail•com>,
	git@vger•kernel.org
Subject: Re: [PATCH] git-remote rename should match whole string when renaming remote ref directory
Date: Mon, 26 Sep 2011 23:07:03 -0700	[thread overview]
Message-ID: <7voby6y0vs.fsf@alter.siamese.dyndns.org> (raw)
In-Reply-To: <4E8138B3.9090909@tonian.com> (Benny Halevy's message of "Tue, 27 Sep 2011 05:45:07 +0300")

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?

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

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 ;-)

next prev parent reply	other threads:[~2011-09-27  6:07 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-09-26 13:53 [PATCH] git-remote rename should match whole string when renaming remote ref directory Benny Halevy
2011-09-26 18:04 ` Junio C Hamano
2011-09-27  2:45   ` Benny Halevy
2011-09-27  6:07     ` Junio C Hamano [this message]
2011-09-27  9:38       ` Benny Halevy

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7voby6y0vs.fsf@alter.siamese.dyndns.org \
    --to=gitster@pobox$(echo .)com \
    --cc=bhalevy@tonian$(echo .)com \
    --cc=git@vger$(echo .)kernel.org \
    --cc=martin.von.zweigbergk@gmail$(echo .)com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox