Re: [PATCH v6] tracking branches: add advice to ambiguous refspec error

From: Glen Choo <chooglen@google.com>
Date: Apr 1, 2022, 16:41 UTC
Message-ID: <kl6lpmm0sqlw.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <CAPMMpojb7-tD2kFNZPiaC0MpB8GNXtYqpJs796pRcM7pE=ksmg@mail.gmail.com>

Tao Klerks <tao@klerks.biz> writes:

Show 56 quoted lines

> On Fri, Apr 1, 2022 at 1:57 AM Glen Choo <chooglen@google.com> wrote:
>>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>> > "Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes:
>> >
>> >>      if (!remote_find_tracking(remote, &tracking->spec)) {
>> >> -            if (++tracking->matches == 1) {
>> >> +            switch (++tracking->matches) {
>> >> +            case 1:
>> >>                      string_list_append(tracking->srcs, tracking->spec.src);
>> >>                      tracking->remote = remote->name;
>> >> -            } else {
>> >> +                    break;
>> >> +            case 2:
>> >> +                    /* there are at least two remotes; backfill the first one */
>> >> +                    string_list_append(&ftb->ambiguous_remotes, tracking->spec.src);
>> >> +                    /* fall through */
>> >> +            default:
>> >> +                    string_list_append(&ftb->ambiguous_remotes, remote->name);
>> >>                      free(tracking->spec.src);
>> >>                      string_list_clear(tracking->srcs, 0);
>> >> +            break;
>> >
>> > Just to sanity check this part,
>> >
>> >  - During the first iteration, we append tracking->spec.src to
>> >    tracking->srcs, and set tracking->remote to remote->name;
>> >
>> >  - In later iterations, we do not want to touch tracking->srcs, and
>> >    want to collect remote->name.
>> >
>> > And "backfill" assumes that tracking->spec.src at that point in the
>> > second iteration is the same as what we got in remote->name in the
>> > first round.  If that were a correct assumption, then it is curious
>> > that the first iteration uses tracking->spec.src and remote->name
>> > separately for different purposes, which makes me want to double
>> > check if the assumption is indeed correct.
>> >
>> > If it were tracking->remote (which was assigned the value of
>> > remote->name during the first iteration) that is used to backfill
>> > before we append remote->name in the second iteration, I wouldn't
>> > find it "curious", but the use of tracking->spec.src there makes me
>> > feel confused.
>> >
>> > Thanks.
>>
>> Thanks for bringing this up, I also found this unusual when I was
>> reading v5.
>
> If you never hear from me again, please know it's because I crawled
> back under the rock I had crawled out from. This is clearly a bug from
> a silly typo, and I've managed to look at the resulting output twice
> without noticing the wrong thing was printed. I'm guessing the use of
> the word "unusual" here is a polite euphemism for "you numskull, what
> you wrote makes no sense!" :)

Please don't take it that way! I use it the way I think most others use it, which is a more charitable "Hm, I trust the author, but this looks confusing to me and let me ask just to be sure that all our bases are covered."

After all, even the best of us make mistakes, so I don't think it's a big deal. Plus, if the mistake managed to sneak past review, the fault is on all of us :)

> I did not think adding an automated test for advise() output made
> sense, but I guess I have proved myself wrong.

Heh, nearly every time I think that a test isn't necessary, I find a way to prove myself wrong too.