Re: [PATCH v23 5/7] branch: add --delete-merged <branch>

Thanks for all the help to drive this across the finish line!

> >>> @@ -38,6 +39,7 @@ static const char * const builtin_branch_usage[] = {
> >>>        N_("git branch [<options>] (-c | -C) [<old-branch>] <new-branch>"),
> >>>        N_("git branch [<options>] [-r | -a] [--points-at]"),
> >>>        N_("git branch [<options>] [-r | -a] [--format]"),
> >>> +     N_("git branch [<options>] (--delete-merged <branch>)... [<pattern>...]"),
> >>
> >> I don't quite follow this - why the "()" and doesn't --delete-merged
> >> take a pattern?
> >
> > I don't get this one, but would this be better?
> >
> > ```
> > git branch [--dry-run] --delete-merged <pattern>
> >     [--delete-merged <pattern>]... [<branch-pattern>...]
>
> I think I just didn't understand what "(--option)..." means - grepping
> around that does seem to be what we use for "give this option at least
> once" so I think the original is fine.

Ok, will use the ().

> >>> +static int branch_pushes_to_upstream(struct branch *branch,
> >>> +                                  const char *upstream)
> >>> +{
> >>> +     struct remote *remote = remote_get(remote_for_branch(branch, NULL));
> >>> +     char *tracking = NULL;
> >>> +     int ret = 0;
> >>> +
> >>> +     if (remote)
> >>> +             tracking = apply_refspecs(&remote->fetch, branch->refname);
> >>
> >> This tells us which remote tracking ref corresponds to the branch
> >>
> >>> +     if (tracking && !strcmp(tracking, upstream))
> >>> +             ret = 1;
> >>
> >> Here we check that it does not match the upstream branch. That ignores
> >> the push refspect though so does not tell us whether pushing the branch
> >> to the upstream remote would update the upstream branch on that remote.
> >
> >> We need to apply the push refspec to the local branch, apply the fetch
> >> refspec in reverse to the result and then compare that to the upstream
> >> branch.
>
> Oops I think that's wrong. We should apply the fetch refspec to the
> result of the push refspec, not apply it in reverse.
>
> If we want to pretend that origin uses "main" rather than "master" we
> could set
>
>         remote.origin.push refs/heads/main:refs/heads/master
>         remote.origin.fetch refs/heads/master:refs/remotes/origin/main
>
> so we map refs/heads/main to refs/heads/master and then map that to
> refs/remotes/origin/main. There's nothing (apart from maybe common
> sense) to say the refspecs have to be symmetric though.

I'm writing logic for this and an integration test. Thanks!

> >> Why do we keep "lower", rather than clear the upstream config of "mid"?
> >
> > We keep lower to preserve the upstream chain of surviving tip.
>
> Yes but why do we want to do that? It's a change in behavior from
> clearing the upstream of mid that wasn't really called out clearly as a
> change in behavior for xxx reason in the cover letter of the iteration
> that introduced it. Why is it desirable to keep the whole chain rather
> than just the upstream of the unmerged branch?

I can do it and get rid of the graph traversal (visited), but I think
we are once again going back to an earlier iteration. One of the other
reviewers wanted to protect transitive branches I think.

Either way is fine, I think it's a pretty small detail in the grand
scheme of things, I just don't want another round of ping-pongs on
this part.


Harald