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

> Hi Harald
>
> Sorry it has taken so long for we to look at this again - I'm not sure
> where last week went.

No worries!

> > A branch is not deleted when:
> >
> >    * it is checked out in any worktree
> >    * its configured upstream ref no longer exists, since a missing
> >      upstream is not by itself a sign of integration
> >    * pushing it by name to the remote configured by
> >      branch.<name>.remote would update its upstream, as determined by
> >      mapping the branch ref through that remote's fetch refspec. For
> >      example, a local "main" that tracks "origin/main" is kept even when
> >      remote.pushDefault names a fork. Right after a pull it merely looks
> >      fully merged.
> >
> > A branch whose work is not yet merged into its upstream is silently
> > skipped, so one unmerged topic does not abort the whole sweep.
> >
> > A branch that a surviving branch depends on through a chain of local
> > upstreams is also kept, so no branch is deleted out from under stacked
> > work.
>
> Shouldn't this be part of the list above. So we no-longer delete any
> branch in a chain of stacked branches when one of them is unmerged?
> Previously we only kept the upstream of the unmerged branch and deleted
> the rest.

Good point.

> > +* its configured upstream ref no longer exists,
> > +* it is checked out in any worktree, or
> > +* pushing it by name to the remote configured by
> > +  `branch.<name>.remote` would update its upstream, so it cannot be
> > +  distinguished from a branch that just looks "fully merged" right
> > +  after a pull.
>
>   * it is the upstream of an unmerged branch

Thanks!

> > +--
> > ++
> > +A branch whose work has not yet been merged into its upstream is
> > +silently skipped. Delete it with `git branch -D` if you want to
> > +remove it anyway.
> > ++
> > +A branch that a surviving branch depends on through a chain of local
> > +upstreams is kept, so a branch is never deleted out from under stacked
> > +work.
> > +
> >   `-v`::
> >   `-vv`::
> >   `--verbose`::
> > diff --git a/builtin/branch.c b/builtin/branch.c
> > index 1ef8362c12..78b694034f 100644
> > --- a/builtin/branch.c
> > +++ b/builtin/branch.c
> > @@ -21,6 +21,7 @@
> >   #include "branch.h"
> >   #include "path.h"
> >   #include "string-list.h"
> > +#include "strmap.h"
> >   #include "column.h"
> >   #include "utf8.h"
> >   #include "ref-filter.h"
> > @@ -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>...]
```

> >       NULL
> >   };
> >
> > @@ -699,6 +701,148 @@ static int parse_opt_forked(const struct option *opt, const char *arg, int unset
> >       return 0;
> >   }
> >
> > +struct stacked_branch_data {
> > +     struct strset *deletable_branch_names;
> > +     struct strset *protected_branch_names;
> > +     struct strset *visited_branch_names;
> > +};
> > +
> > +static int collect_stacked_branch_bases(const struct reference *ref,
> > +                                     void *cb_data)
> > +{
> > +     struct stacked_branch_data *data = cb_data;
> > +     const char *branch_name;
> > +
> > +     if (!skip_prefix(ref->name, "refs/heads/", &branch_name))
> > +             BUG("expected local branch ref, got '%s'", ref->name);
> > +     if (strset_contains(data->deletable_branch_names, branch_name))
> > +             return 0;
> > +
> > +     while (strset_add(data->visited_branch_names, branch_name)) {
> > +             struct branch *branch = branch_get(branch_name);
> > +             const char *upstream_refname = branch_get_upstream(branch, NULL);
> > +             const char *upstream_branch_name;
> > +
> > +             if (!upstream_refname ||
> > +                 !skip_prefix(upstream_refname, "refs/heads/",
> > +                              &upstream_branch_name) ||
> > +                 !strset_contains(data->deletable_branch_names,
> > +                                 upstream_branch_name))
> > +                     break;
> > +
> > +             strset_add(data->protected_branch_names, upstream_branch_name);
> > +             branch_name = upstream_branch_name;
> > +     }
>
> This looks correct, it is a shame we have to build
> "visited_branch_names" but the code is clear.
>
> > +     return 0;
> > +}
> > +
> > +static void protect_stacked_branch_bases(struct ref_store *refs,
> > +                                      struct strset *deletable_branch_names)
> > +{
> > +     struct strset protected_branch_names = STRSET_INIT;
> > +     struct strset visited_branch_names = STRSET_INIT;
> > +     struct stacked_branch_data data = {
> > +             .deletable_branch_names = deletable_branch_names,
> > +             .protected_branch_names = &protected_branch_names,
> > +             .visited_branch_names = &visited_branch_names,
> > +     };
> > +     struct refs_for_each_ref_options opts = {
> > +             .prefix = "refs/heads/",
> > +     };
> > +     struct hashmap_iter iter;
> > +     struct strmap_entry *entry;
> > +
> > +     refs_for_each_ref_ext(refs, collect_stacked_branch_bases, &data, &opts);
> > +
> > +     strset_for_each_entry(&protected_branch_names, &iter, entry)
> > +             strset_remove(deletable_branch_names, entry->key);
>
> We remove the protected branches from deleteable - good
>
> > +
> > +     strset_clear(&visited_branch_names);
> > +     strset_clear(&protected_branch_names);
> > +}
> > +
> > +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.

I'll try to do something about this, but I removed a big part of the
push logic when introducing the stacked branches, it simplified things
a lot without it. But maybe too simplistic.

> > +test_expect_success '--delete-merged keeps cloned main without a default push remote' '
> > +     setup_repo_for_delete_merged &&
> > +     (
> > +             cd repo &&
> > +             git checkout --detach &&
> > +
> > +             git branch --delete-merged */* &&
>
> Unless I've missed something main does not have an upstream branch set,
> so we'd never expect it to be deleted, even if a push remote was set.

I'll take a look.

> > +test_expect_success '--delete-merged deletes only selected merged branches' '
> > +     setup_repo_for_delete_merged &&
> > +     create_merged_branch also-merged &&
> > +     create_merged_branch merged &&
> > +     (
> > +             cd repo &&
> > +             git checkout -b unmerged origin/next --track &&
>
> In the tests we try to avoid mixing options and positional arguments so
> we use either
>         git checkout --track -b unmerged origin/next
> or
>         git checkout -b unmerged --track origin/next

Good point.

> > +test_expect_success '--delete-merged keeps main despite a different default push remote' '
> > +     setup_repo_for_delete_merged &&
> > +     create_merged_branch on-next &&
> > +     create_merged_branch checked-out &&
> > +     create_merged_branch upstream-gone &&
> > +     (
> > +             cd repo &&
> > +             git config remote.pushDefault fork &&
> > +             git checkout -b local-to-delete main --track &&
> > +             git update-ref refs/remotes/origin/topic refs/remotes/origin/next &&
> > +             git branch --set-upstream-to=origin/topic upstream-gone &&
> > +             git update-ref -d refs/remotes/origin/topic &&
>
> As I think I said last time, this is a very round-about way to have the
> upstream gone. It would be much simpler just to set the config directly.

Good point.

> > +             git checkout -b tracks-other other/main --track &&
> > +             git checkout checked-out &&
> > +
> > +             git branch --delete-merged origin/* \
> > +                     --delete-merged main &&
>
> These lines look very short why the wrapping?

Will unwrap some of these.

> Why do we keep "lower", rather than clear the upstream config of "mid"?

We keep lower to preserve the upstream chain of surviving tip.



Harald