Re: [PATCH v2 8/8] reflog expire: cover reflog from all worktrees

From: Duy Nguyen <pclouds@gmail.com>
Date: Oct 2, 2018, 16:16 UTC
Message-ID: <CACsJy8C1niWzC8GPiKkXmyBwxbctb-vmM51CVts12TMzLFnJVA@mail.gmail.com>
In-Reply-To: <CAPig+cQBiXRvzf6skmmyVap-6fLEbmVRGNBQqD4H-HfAPampjA@mail.gmail.com>

On Sun, Sep 30, 2018 at 7:36 AM Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 15 quoted lines

>
> On Sat, Sep 29, 2018 at 3:11 PM Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> > ---
> > diff --git a/Documentation/git-reflog.txt b/Documentation/git-reflog.txt
> > @@ -72,6 +72,11 @@ Options for `expire`
> > +--single-worktree::
> > +       By default when `--all` is specified, reflogs from all working
> > +       trees are processed. This option limits the processing to reflogs
> > +       from the current working tree only.
>
> Bikeshedding: I wonder if this should be named "--this-worktree" or
> "--this-worktree-only" or if it should somehow be orthogonal to --all
> rather than modifying it. (Genuine questions. I don't have the
> answers.)

It follows a precedent (made by me :p) which is rev-list --single-worktree. I doubt that option is widely used though so we could still rename it if there's a better name. I made --single-worktree to contrast "all worktrees" by default. Even if it's "this/current worktree" it still has to somehow say "everything in this worktree" so I felt modifying --all was a good idea.

Show 19 quoted lines

> > diff --git a/builtin/reflog.c b/builtin/reflog.c
> > @@ -577,10 +585,18 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
> >         if (do_all) {
> >                 struct collect_reflog_cb collected;
> > +               struct worktree **worktrees, **p;
> >                 int i;
> >
> >                 memset(&collected, 0, sizeof(collected));
> > -               for_each_reflog(collect_reflog, &collected);
> > +               worktrees = get_worktrees(0);
> > +               for (p = worktrees; *p; p++) {
> > +                       if (!all_worktrees && !(*p)->is_current)
> > +                               continue;
> > +                       collected.wt = *p;
> > +                       for_each_reflog(collect_reflog, &collected);
> > +               }
> > +               free_worktrees(worktrees);
>
> Should this have a test in the test suite?

Of course. I was partly lazy/tired near the end, and anticipated more comments anyway so I did not do it :D

-- 
Duy