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

[Tao Klerks <tao@klerks.biz>]

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

> > +     /*
> > +      * 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 :)

>
> > +     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?