[v2,8/8] reflog expire: cover reflog from all worktrees
diff mbox series

Message ID 20180929191029.13994-9-pclouds@gmail.com
State New
Headers show
Series
  • [v2,1/8] refs.c: indent with tabs, not spaces
Related show

Commit Message

Duy Nguyen Sept. 29, 2018, 7:10 p.m. UTC
Reported-by: Jeff King <peff@peff.net>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/git-reflog.txt |  7 ++++++-
 builtin/reflog.c             | 22 +++++++++++++++++++---
 2 files changed, 25 insertions(+), 4 deletions(-)

Comments

Eric Sunshine Sept. 30, 2018, 5:36 a.m. UTC | #1
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.)

> 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?
Duy Nguyen Oct. 2, 2018, 4:16 p.m. UTC | #2
On Sun, Sep 30, 2018 at 7:36 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> 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.

> > 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
Eric Sunshine Oct. 3, 2018, 7:49 a.m. UTC | #3
On Tue, Oct 2, 2018 at 12:16 PM Duy Nguyen <pclouds@gmail.com> wrote:
> On Sun, Sep 30, 2018 at 7:36 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > On Sat, Sep 29, 2018 at 3:11 PM Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> > > +--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.

Precedent overrides bikeshedding, so leaving it as-is is fine.

Patch
diff mbox series

diff --git a/Documentation/git-reflog.txt b/Documentation/git-reflog.txt
index 472a6808cd..ff487ff77d 100644
--- a/Documentation/git-reflog.txt
+++ b/Documentation/git-reflog.txt
@@ -20,7 +20,7 @@  depending on the subcommand:
 'git reflog' ['show'] [log-options] [<ref>]
 'git reflog expire' [--expire=<time>] [--expire-unreachable=<time>]
 	[--rewrite] [--updateref] [--stale-fix]
-	[--dry-run | -n] [--verbose] [--all | <refs>...]
+	[--dry-run | -n] [--verbose] [--all [--single-worktree] | <refs>...]
 'git reflog delete' [--rewrite] [--updateref]
 	[--dry-run | -n] [--verbose] ref@\{specifier\}...
 'git reflog exists' <ref>
@@ -72,6 +72,11 @@  Options for `expire`
 --all::
 	Process the reflogs of all references.
 
+--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.
+
 --expire=<time>::
 	Prune entries older than the specified time. If this option is
 	not specified, the expiration time is taken from the
diff --git a/builtin/reflog.c b/builtin/reflog.c
index 3acef5a0ab..eed956851e 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -10,6 +10,7 @@ 
 #include "diff.h"
 #include "revision.h"
 #include "reachable.h"
+#include "worktree.h"
 
 /* NEEDSWORK: switch to using parse_options */
 static const char reflog_expire_usage[] =
@@ -52,6 +53,7 @@  struct collect_reflog_cb {
 	struct collected_reflog **e;
 	int alloc;
 	int nr;
+	struct worktree *wt;
 };
 
 /* Remember to update object flag allocation in object.h */
@@ -388,8 +390,12 @@  static int collect_reflog(const char *ref, const struct object_id *oid, int unus
 {
 	struct collected_reflog *e;
 	struct collect_reflog_cb *cb = cb_data;
+	struct strbuf newref = STRBUF_INIT;
+
+	strbuf_worktree_ref(cb->wt, &newref, ref);
+	FLEX_ALLOC_STR(e, reflog, newref.buf);
+	strbuf_release(&newref);
 
-	FLEX_ALLOC_STR(e, reflog, ref);
 	oidcpy(&e->oid, oid);
 	ALLOC_GROW(cb->e, cb->nr + 1, cb->alloc);
 	cb->e[cb->nr++] = e;
@@ -512,7 +518,7 @@  static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 {
 	struct expire_reflog_policy_cb cb;
 	timestamp_t now = time(NULL);
-	int i, status, do_all;
+	int i, status, do_all, all_worktrees = 1;
 	int explicit_expiry = 0;
 	unsigned int flags = 0;
 
@@ -549,6 +555,8 @@  static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 			flags |= EXPIRE_REFLOGS_UPDATE_REF;
 		else if (!strcmp(arg, "--all"))
 			do_all = 1;
+		else if (!strcmp(arg, "--single-worktree"))
+			all_worktrees = 0;
 		else if (!strcmp(arg, "--verbose"))
 			flags |= EXPIRE_REFLOGS_VERBOSE;
 		else if (!strcmp(arg, "--")) {
@@ -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);
 		for (i = 0; i < collected.nr; i++) {
 			struct collected_reflog *e = collected.e[i];
 			set_reflog_expiry_param(&cb.cmd, explicit_expiry, e->reflog);