Re: [PATCH v3 1/2] merge: new autosetupmerge option 'simple' for matching branches

From: Tao Klerks <tao@klerks.biz>
Date: Mar 2, 2022, 09:35 UTC
Message-ID: <CAPMMpoi9gQscSQ5Xn1xTb6WaCXu+qR67DJh9nCbqN0jp7-b_5A@mail.gmail.com>
In-Reply-To: <220228.86k0df5key.gmgdl@evledraar.gmail.com>

On Mon, Feb 28, 2022 at 11:56 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 6 quoted lines

>
> On Mon, Feb 28 2022, Tao Klerks via GitGitGadget wrote:
>
> I think squashing 2/2 inot this would make this much easier to follow,
> i.e. to have tests along with the new feature.
>

OK! Doing.

Show 15 quoted lines

> > +     /*
> > +      * This check does not apply to the BRANCH_TRACK_INHERIT
> > +      * option; you can inherit one or more tracking entries
> > +      * and the tracking.matches counter is not incremented.
> > +      */
> >       if (tracking.matches > 1)
> >               die(_("not tracking: ambiguous information for ref %s"),
> >                   orig_ref);
>
> This function is the only user of find_tracked_branch(). For e.g. "git
> checkout we emit";
>
>     fatal: builtin/checkout.c:1246: 'foo' matched multiple (4) remote tracking branches
>
> Perhaps we can do something similar here

I'm not sure what you're pointing to specifically - the fact that the checkout message provides a count? If so I guess I understand/agree, find_tracked_branch() could be enhanced to keep counting rather than exiting at the first sign of trouble, to support such a slightly-more-explicit message here.

I'm not convinced that this situation is common enough to warrant change: mapping multiple remotes to the same remote-tracking path seems like a strange setup - is this something we recommend or document anywhere? maybe to have 2 "remotes" that correspond to the same server over different protocols appear as one set of tracking branches?

On the other hand I am of course happy to make things better if we think this will do that!

> even with some advise()
> emit information about what other branches conflicted.

I believe the conflict is not about different "branches" exactly, but about *refspecs* that map to the tracking branch.

If I understand correctly this change would entail creating a new advice type (and documenting it), and figuring out what the advice should look like - something like "find and disambiguate your fetch refspecs to enable auto tracking setup! If you want to keep your ambiguous refspecs, set auto tracking setup to false!" - but nicer :)

Show 23 quoted lines

>
> > +     if (track == BRANCH_TRACK_SIMPLE) {
> > +             /*
> > +              * Only track if remote branch name matches.
> > +              * Reaching into items[0].string is safe because
> > +              * we know there is at least one and not more than
> > +              * one entry (because not BRANCH_TRACK_INHERIT).
> > +              */
> > +             const char *tracked_branch;
> > +             if (!skip_prefix(tracking.srcs->items[0].string,
> > +                              "refs/heads/", &tracked_branch) ||
> > +                 strcmp(tracked_branch, new_ref))
> > +                     return;
> > +     }
> > +
>
> I wondered when reading this if there isn't a way to merge this and the
> "branch_get" call made in "inherit_tracking" earlier in this function in
> the "track != BRANCH_TRACK_INHERIT" case.
>
> But maybe not, and that whole API entry point is a bit messy in needing
> to cover both the use-case of an existing branch & nonexisting
> (i.e. initial creation).

Hmm, I had a hard time understanding this comment. I *think* you were saying "why don't you use an existing API to get the full ref name of the new local branch, and compare that to the full name of the remote branch you already have, rather than messing with a "refs/heads/" prefix explicitly/redundantly"... Is that right?