[PATCH] log: new option decorate reflog of remote refs

[PATCH] log: new option decorate reflog of remote refs

[Nguyễn Thái Ngọc Duy]

This is most useful when you fork your branches off a remote ref and
rely on ref decoration to show your fork points in `git log`. Then you
do a "git fetch" and suddenly the remote decoration is gone because
remote refs are moved forward. With this, we can still see something
like "origin/foo@{1}"

This is for remote refs only because based on my experience, docorating
local reflog is just too noisy. You will most likely see HEAD@{1},
HEAD@{2} and so on. We can add that as a separate option in future if we
see a need for it.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 I've been using this for many weeks and it has proven its usefulness
 (to me). Looks like good material to send upstream.

 Documentation/git-log.txt |  5 +++++
 builtin/log.c             | 10 +++++++++-
 log-tree.c                | 43 +++++++++++++++++++++++++++++++++++++++----
 log-tree.h                |  2 +-
 pretty.c                  |  4 ++--
 revision.c                |  2 +-
 6 files changed, 57 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
index 32246fd..f5ee575 100644
--- a/Documentation/git-log.txt
+++ b/Documentation/git-log.txt
@@ -38,6 +38,11 @@ OPTIONS
 	are shown as if 'short' were given, otherwise no ref names are
 	shown. The default option is 'short'.
 
+--decorate-remote-reflog[=<n>]::
+	Decorate `<n>` most recent reflog entries on remote refs, up
+	to the specified number of entries. By default, only the most
+	recent reflog entry is decorated.
+
 --source::
 	Print out the ref name given on the command line by which each
 	commit was reached.
diff --git a/builtin/log.c b/builtin/log.c
index 55d20cc..c208703 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -36,6 +36,7 @@ static int default_follow;
 static int default_show_signature;
 static int decoration_style;
 static int decoration_given;
+static int decorate_remote_reflog;
 static int use_mailmap_config;
 static const char *fmt_patch_subject_prefix = "PATCH";
 static const char *fmt_pretty;
@@ -141,6 +142,10 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
 		OPT_BOOL(0, "use-mailmap", &mailmap, N_("Use mail map file")),
 		{ OPTION_CALLBACK, 0, "decorate", NULL, NULL, N_("decorate options"),
 		  PARSE_OPT_OPTARG, decorate_callback},
+		{ OPTION_INTEGER, 0, "decorate-remote-reflog",
+		  &decorate_remote_reflog, N_("n"),
+		  N_("decorate the last <n> reflog entries of remote refs"),
+		  PARSE_OPT_OPTARG | PARSE_OPT_NONEG, NULL, 1 },
 		OPT_CALLBACK('L', NULL, &line_cb, "n,m:file",
 			     N_("Process line range n,m in file, counting from 1"),
 			     log_line_range_callback),
@@ -195,9 +200,12 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
 			rev->abbrev_commit = 0;
 	}
 
+	if (decorate_remote_reflog > 0 && !decoration_style)
+		decoration_style = DECORATE_SHORT_REFS;
 	if (decoration_style) {
 		rev->show_decorations = 1;
-		load_ref_decorations(decoration_style);
+		load_ref_decorations(decoration_style,
+				     decorate_remote_reflog);
 	}
 
 	if (rev->line_level_traverse)
diff --git a/log-tree.c b/log-tree.c
index 8c24157..3d85ebc 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -88,14 +88,37 @@ const struct name_decoration *get_name_decoration(const struct object *obj)
 	return lookup_decoration(&name_decoration, obj);
 }
 
+struct reflog_cb {
+	int type;
+	int count;
+	int nth;
+	const char *refname;
+};
+
+static int add_nth_reflog(unsigned char *osha1, unsigned char *nsha1,
+			  const char *email, unsigned long timestamp, int tz,
+			  const char *message, void *cb_data)
+{
+	struct reflog_cb *cb = cb_data;
+	struct commit *commit;
+
+	commit = lookup_commit(nsha1);
+	if (commit && cb->nth) {
+		struct strbuf sb = STRBUF_INIT;
+		strbuf_addf(&sb, "%s@{%d}", cb->refname, cb->nth);
+		add_name_decoration(cb->type, sb.buf, &commit->object);
+		strbuf_release(&sb);
+	}
+	cb->nth++;
+	return cb->nth >= cb->count;
+}
+
 static int add_ref_decoration(const char *refname, const struct object_id *oid,
 			      int flags, void *cb_data)
 {
 	struct object *obj;
 	enum decoration_type type = DECORATION_NONE;
 
-	assert(cb_data == NULL);
-
 	if (starts_with(refname, git_replace_ref_base)) {
 		struct object_id original_oid;
 		if (!check_replace_refs)
@@ -135,6 +158,17 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid,
 			parse_object(obj->oid.hash);
 		add_name_decoration(DECORATION_REF_TAG, refname, obj);
 	}
+
+	if (cb_data && type == DECORATION_REF_REMOTE) {
+		struct reflog_cb cb;
+
+		memset(&cb, 0, sizeof(cb));
+		cb.refname = refname;
+		cb.type = type;
+		cb.count = *(int *)cb_data + 1 /* for @{0} */;
+
+		for_each_reflog_ent_reverse(refname, add_nth_reflog, &cb);
+	}
 	return 0;
 }
 
@@ -147,13 +181,14 @@ static int add_graft_decoration(const struct commit_graft *graft, void *cb_data)
 	return 0;
 }
 
-void load_ref_decorations(int flags)
+void load_ref_decorations(int flags, int remote_reflog)
 {
 	if (!decoration_loaded) {
+		void *cb = remote_reflog ? &remote_reflog : NULL;
 
 		decoration_loaded = 1;
 		decoration_flags = flags;
-		for_each_ref(add_ref_decoration, NULL);
+		for_each_ref(add_ref_decoration, cb);
 		head_ref(add_ref_decoration, NULL);
 		for_each_commit_graft(add_graft_decoration, NULL);
 	}
diff --git a/log-tree.h b/log-tree.h
index c8116e6..bb46c53 100644
--- a/log-tree.h
+++ b/log-tree.h
@@ -25,7 +25,7 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,
 			     const char **subject_p,
 			     const char **extra_headers_p,
 			     int *need_8bit_cte_p);
-void load_ref_decorations(int flags);
+void load_ref_decorations(int flags, int reflog);
 
 #define FORMAT_PATCH_NAME_MAX 64
 void fmt_output_commit(struct strbuf *, struct commit *, struct rev_info *);
diff --git a/pretty.c b/pretty.c
index 5e68383..ec8e1cc 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1189,11 +1189,11 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 		strbuf_addstr(sb, get_revision_mark(NULL, commit));
 		return 1;
 	case 'd':
-		load_ref_decorations(DECORATE_SHORT_REFS);
+		load_ref_decorations(DECORATE_SHORT_REFS, 0);
 		format_decorations(sb, commit, c->auto_color);
 		return 1;
 	case 'D':
-		load_ref_decorations(DECORATE_SHORT_REFS);
+		load_ref_decorations(DECORATE_SHORT_REFS, 0);
 		format_decorations_extended(sb, commit, c->auto_color, "", ", ", "");
 		return 1;
 	case 'g':		/* reflog info */
diff --git a/revision.c b/revision.c
index b37dbec..4d5cbf5 100644
--- a/revision.c
+++ b/revision.c
@@ -1743,7 +1743,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->simplify_by_decoration = 1;
 		revs->limited = 1;
 		revs->prune = 1;
-		load_ref_decorations(DECORATE_SHORT_REFS);
+		load_ref_decorations(DECORATE_SHORT_REFS, 0);
 	} else if (!strcmp(arg, "--date-order")) {
 		revs->sort_order = REV_SORT_BY_COMMIT_DATE;
 		revs->topo_order = 1;
-- 
2.8.2.524.g6ff3d78

Re: [PATCH] log: new option decorate reflog of remote refs

[Jeff King]

On Thu, Jan 19, 2017 at 07:26:30PM +0700, Nguyễn Thái Ngọc Duy wrote:

> This is most useful when you fork your branches off a remote ref and
> rely on ref decoration to show your fork points in `git log`. Then you
> do a "git fetch" and suddenly the remote decoration is gone because
> remote refs are moved forward. With this, we can still see something
> like "origin/foo@{1}"
> 
> This is for remote refs only because based on my experience, docorating
> local reflog is just too noisy. You will most likely see HEAD@{1},
> HEAD@{2} and so on. We can add that as a separate option in future if we
> see a need for it.
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  I've been using this for many weeks and it has proven its usefulness
>  (to me). Looks like good material to send upstream.

I think it's a neat idea, but the actual option:

> +--decorate-remote-reflog[=<n>]::
> +	Decorate `<n>` most recent reflog entries on remote refs, up
> +	to the specified number of entries. By default, only the most
> +	recent reflog entry is decorated.

seems weirdly limited and non-orthogonal. What happens when somebody
wants to decorate other reflogs besides refs/remotes?

We already have very flexible ref-selectors like --remotes, --branches,
etc. The generalization of this would perhaps be something like:

  git log --decorate-reflog --remotes --branches

where "--decorate-reflog" applies to the next ref selector and then is
reset, the same way --exclude is. And it includes those refs _only_ for
decoration, not for traversal. So you could do:

  git log --decorate-reflog --remotes --remotes

if you wanted to see use those as traversal roots, too (if this is
common, it might even merit another option for "decorate and show").

That's just off the top of my head, so maybe there are issues. I was
just surprised to see the "-remote" part in your option name.

-Peff

Re: [PATCH] log: new option decorate reflog of remote refs

[Duy Nguyen]

On Fri, Jan 20, 2017 at 12:23 AM, Jeff King <peff@peff.net> wrote:
> I think it's a neat idea, but the actual option:
>
>> +--decorate-remote-reflog[=<n>]::
>> +     Decorate `<n>` most recent reflog entries on remote refs, up
>> +     to the specified number of entries. By default, only the most
>> +     recent reflog entry is decorated.
>
> seems weirdly limited and non-orthogonal. What happens when somebody
> wants to decorate other reflogs besides refs/remotes?
>
> We already have very flexible ref-selectors like --remotes, --branches,
> etc. The generalization of this would perhaps be something like:
>
>   git log --decorate-reflog --remotes --branches
>
> where "--decorate-reflog" applies to the next ref selector and then is
> reset, the same way --exclude is. And it includes those refs _only_ for
> decoration, not for traversal. So you could do:
>
>   git log --decorate-reflog --remotes --remotes
>
> if you wanted to see use those as traversal roots, too (if this is
> common, it might even merit another option for "decorate and show").
>
> That's just off the top of my head, so maybe there are issues. I was
> just surprised to see the "-remote" part in your option name.

Imposing order between options could cause confusion, I think, if you
remove --decorate-reflog leaving --remotes on by accident, now you get
--remotes with a new meaning. We could go with something like
--decodate-reflog=remote, but that clashes with the number of reflog
entries and we may need a separator, like --decorate-reflog=remote,3.
Or we could add something to --decorate= in addition to
short|full|auto|no. Something like --decorate=full,reflog or
--decorate=full,reflog=remote,entries=3 if I want 3 reflog entries.

My hesitant to go that far is because I suspect decorating reflog
won't be helpful for non-remotes. But I'm willing to make more changes
if it opens door to master.
-- 
Duy

Re: [PATCH] log: new option decorate reflog of remote refs

[Jeff King]

On Fri, Jan 20, 2017 at 05:55:21PM +0700, Duy Nguyen wrote:

> > We already have very flexible ref-selectors like --remotes, --branches,
> > etc. The generalization of this would perhaps be something like:
> >
> >   git log --decorate-reflog --remotes --branches
> >
> > where "--decorate-reflog" applies to the next ref selector and then is
> > reset, the same way --exclude is. And it includes those refs _only_ for
> > decoration, not for traversal. So you could do:
> >
> >   git log --decorate-reflog --remotes --remotes
> >
> > if you wanted to see use those as traversal roots, too (if this is
> > common, it might even merit another option for "decorate and show").
> >
> > That's just off the top of my head, so maybe there are issues. I was
> > just surprised to see the "-remote" part in your option name.
> 
> Imposing order between options could cause confusion, I think, if you
> remove --decorate-reflog leaving --remotes on by accident, now you get
> --remotes with a new meaning. We could go with something like
> --decodate-reflog=remote, but that clashes with the number of reflog
> entries and we may need a separator, like --decorate-reflog=remote,3.
> Or we could add something to --decorate= in addition to
> short|full|auto|no. Something like --decorate=full,reflog or
> --decorate=full,reflog=remote,entries=3 if I want 3 reflog entries.

I agree that making option-order important is potentially confusing. But
it does already exist with --exclude. It's necessary to specify some
sets of refs (e.g., all of A, except for those that match B, and then
all of C, including those that match B).

Having --decorate-reflog=remote would be similarly constrained. You
couldn't do "decorate all remotes except for these ones". For that
matter, I'm not sure how you would do "decorate just the refs from
origin".

I'll grant that those are going to be a lot less common than just "all
the remotes" (or all the tags, or whatever). I'd just hate to see us
revisiting this in a year to generalize it, and being stuck with
historical baggage.

> My hesitant to go that far is because I suspect decorating reflog
> won't be helpful for non-remotes. But I'm willing to make more changes
> if it opens door to master.

Forgetting reflogs for a moment, I'd actually find it useful to just
decorate tags and local branches, but not remotes. But right now there
isn't any way to select which refs are worthy of decoration (reflog or
not).

That's why I'm thinking so much about a general ref-selection system. I
agree the "--exclude=... --remotes" thing is complicated, but it's also
the ref-selection system we _already_ have, which to me is a slight
point in its favor.

-Peff

Re: [PATCH] log: new option decorate reflog of remote refs

[Jacob Keller]

On Fri, Jan 20, 2017 at 6:30 AM, Jeff King <peff@peff.net> wrote:
>> Imposing order between options could cause confusion, I think, if you
>> remove --decorate-reflog leaving --remotes on by accident, now you get
>> --remotes with a new meaning. We could go with something like
>> --decodate-reflog=remote, but that clashes with the number of reflog
>> entries and we may need a separator, like --decorate-reflog=remote,3.
>> Or we could add something to --decorate= in addition to
>> short|full|auto|no. Something like --decorate=full,reflog or
>> --decorate=full,reflog=remote,entries=3 if I want 3 reflog entries.
>
> I agree that making option-order important is potentially confusing. But
> it does already exist with --exclude. It's necessary to specify some
> sets of refs (e.g., all of A, except for those that match B, and then
> all of C, including those that match B).
>
> Having --decorate-reflog=remote would be similarly constrained. You
> couldn't do "decorate all remotes except for these ones". For that
> matter, I'm not sure how you would do "decorate just the refs from
> origin".
>
> I'll grant that those are going to be a lot less common than just "all
> the remotes" (or all the tags, or whatever). I'd just hate to see us
> revisiting this in a year to generalize it, and being stuck with
> historical baggage.
>
>> My hesitant to go that far is because I suspect decorating reflog
>> won't be helpful for non-remotes. But I'm willing to make more changes
>> if it opens door to master.
>
> Forgetting reflogs for a moment, I'd actually find it useful to just
> decorate tags and local branches, but not remotes. But right now there
> isn't any way to select which refs are worthy of decoration (reflog or
> not).
>
> That's why I'm thinking so much about a general ref-selection system. I
> agree the "--exclude=... --remotes" thing is complicated, but it's also
> the ref-selection system we _already_ have, which to me is a slight
> point in its favor.
>
> -Peff

I agree that the interaction between --exclude and --remotes/etc is
confusing, but I think it's reasonable enough because we already
support it, so it makes sense to extend it with this. I also think its
better to extend here than it is to hard-code it. We could provide a
single short-option that does the longer variant if it's that common.

Thanks,
Jake

Re: [PATCH] log: new option decorate reflog of remote refs

[Duy Nguyen]

On Sat, Jan 21, 2017 at 5:00 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
> On Fri, Jan 20, 2017 at 6:30 AM, Jeff King <peff@peff.net> wrote:
>>> Imposing order between options could cause confusion, I think, if you
>>> remove --decorate-reflog leaving --remotes on by accident, now you get
>>> --remotes with a new meaning. We could go with something like
>>> --decodate-reflog=remote, but that clashes with the number of reflog
>>> entries and we may need a separator, like --decorate-reflog=remote,3.
>>> Or we could add something to --decorate= in addition to
>>> short|full|auto|no. Something like --decorate=full,reflog or
>>> --decorate=full,reflog=remote,entries=3 if I want 3 reflog entries.
>>
>> I agree that making option-order important is potentially confusing. But
>> it does already exist with --exclude. It's necessary to specify some
>> sets of refs (e.g., all of A, except for those that match B, and then
>> all of C, including those that match B).
>>
>> Having --decorate-reflog=remote would be similarly constrained. You
>> couldn't do "decorate all remotes except for these ones". For that
>> matter, I'm not sure how you would do "decorate just the refs from
>> origin".
>>
>> I'll grant that those are going to be a lot less common than just "all
>> the remotes" (or all the tags, or whatever). I'd just hate to see us
>> revisiting this in a year to generalize it, and being stuck with
>> historical baggage.
>>
>>> My hesitant to go that far is because I suspect decorating reflog
>>> won't be helpful for non-remotes. But I'm willing to make more changes
>>> if it opens door to master.
>>
>> Forgetting reflogs for a moment, I'd actually find it useful to just
>> decorate tags and local branches, but not remotes. But right now there
>> isn't any way to select which refs are worthy of decoration (reflog or
>> not).
>>
>> That's why I'm thinking so much about a general ref-selection system. I
>> agree the "--exclude=... --remotes" thing is complicated, but it's also
>> the ref-selection system we _already_ have, which to me is a slight
>> point in its favor.
>>
>> -Peff
>
> I agree that the interaction between --exclude and --remotes/etc is
> confusing, but I think it's reasonable enough because we already
> support it, so it makes sense to extend it with this. I also think its
> better to extend here than it is to hard-code it.

OK. Next question, how do we deal with the reflog count (i..e the
argument of --decorate-remote-reflog). Should it be shared for all ref
type, or can be specified differently for remote, local and tags? I'm
leaning towards the former. But I'll wait a bit for ideas before
rewriting the patch.
--
Duy

Re: [PATCH] log: new option decorate reflog of remote refs

[Jeff King]

On Sat, Jan 21, 2017 at 07:48:50PM +0700, Duy Nguyen wrote:

> OK. Next question, how do we deal with the reflog count (i..e the
> argument of --decorate-remote-reflog). Should it be shared for all ref
> type, or can be specified differently for remote, local and tags? I'm
> leaning towards the former. But I'll wait a bit for ideas before
> rewriting the patch.

I doubt that anybody really cares about different reflog depths for
different refs. But I would say that the natural syntax ends up as:

  git log --decorate-reflog=10 --remotes \
          --decorate-reflog=10 --tags

anyway, so you get the ability to do it anyway "for free" (at the cost
of having to repeat yourself).

I guess the other option is:

  git log --decorate-reflog-depth=10 \
          --decorate-reflog --remotes
	  --decorate-reflog --tags

That's actually _more_ typing, and besides being less flexible just
muddles the "is this option for the next ref-selector or not" question.

(The whole thing is obviously a lot of typing; I wonder if people would
want a config option to do this all the time).

-Peff

[PATCH 0/5] Prep steps for --decorate-reflog

[Nguyễn Thái Ngọc Duy]

I'm still half way through implementing --decorate-reflog that can
select what refs to decorate using --branches, --remotes, --tags...
But I want to make sure I'm heading the right direction first since
I'm not really sure if this is the right way (implementation wise).

This series does not really implement --decorate-reflog. It shuffles
revision.c code around a bit so thay the option can be implemented
later. The most controversal patch would be 4/5 where --exclude
behavior is changed slighly.

Good? Bad? Horror hooorrrible?

Nguyễn Thái Ngọc Duy (5):
  rev-list-options.txt: delete an empty line
  revision.c: group ref selection options together
  revision.c: allow to change pseudo opt parsing function
  revision.c: refactor ref selection handler after --exclude
  revision.c: add --decorate-reflog

 Documentation/rev-list-options.txt |   1 -
 revision.c                         | 206 ++++++++++++++++++++++++++++++-------
 revision.h                         |   4 +
 3 files changed, 173 insertions(+), 38 deletions(-)

-- 
2.11.0.157.gd943d85

[PATCH 1/5] rev-list-options.txt: delete an empty line

[Nguyễn Thái Ngọc Duy]

The convention has been option name is followed immediately by its
description without a line break.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/rev-list-options.txt | 1 -
 1 file changed, 1 deletion(-)

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 5da7cf5a8d..1f948cfb13 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -161,7 +161,6 @@ parents) and `--max-parents=-1` (negative numbers denote no upper limit).
 	or '[', '/{asterisk}' at the end is implied.
 
 --exclude=<glob-pattern>::
-
 	Do not include refs matching '<glob-pattern>' that the next `--all`,
 	`--branches`, `--tags`, `--remotes`, or `--glob` would otherwise
 	consider. Repetitions of this option accumulate exclusion patterns
-- 
2.11.0.157.gd943d85

[PATCH 2/5] revision.c: group ref selection options together

[Nguyễn Thái Ngọc Duy]

These options have on thing in common: when specified right after
--exclude, they will de-select refs instead of selecting them by
default.

This change makes it possible to introduce new options that use these
options in the same way as --exclude. Such an option would just
implement something like handle_refs_pseudo_opt().

parse_ref_selector_option() is taken out of handle_refs_pseudo_opt() so
that similar functions like handle_refs_pseudo_opt() are forced to
handle all ref selector options, not skipping some by mistake, which may
revert the option back to default behavior (rev selection).

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 revision.c | 134 +++++++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 100 insertions(+), 34 deletions(-)

diff --git a/revision.c b/revision.c
index b37dbec378..6ebd38d1c8 100644
--- a/revision.c
+++ b/revision.c
@@ -2059,6 +2059,103 @@ static int for_each_good_bisect_ref(const char *submodule, each_ref_fn fn, void
 	return for_each_bisect_ref(submodule, fn, cb_data, term_good);
 }
 
+enum ref_selector {
+	REF_SELECT_NONE,
+	REF_SELECT_ALL,
+	REF_SELECT_BRANCHES,
+	REF_SELECT_TAGS,
+	REF_SELECT_REMOTES,
+	REF_SELECT_BY_GLOB
+};
+
+static enum ref_selector parse_ref_selector_option(int argc, const char **argv,
+						   const char **optarg,
+						   int *argcount)
+{
+	const char *arg = argv[0];
+
+	*optarg = NULL;
+
+	if (!strcmp(arg, "--all"))
+		return REF_SELECT_ALL;
+	else if (!strcmp(arg, "--branches") ||
+		 skip_prefix(arg, "--branches=", optarg))
+		return REF_SELECT_BRANCHES;
+	else if (!strcmp(arg, "--tags") ||
+		 skip_prefix(arg, "--tags=", optarg))
+		return REF_SELECT_TAGS;
+	else if (!strcmp(arg, "--remotes") ||
+		 skip_prefix(arg, "--remotes=", optarg))
+		return REF_SELECT_REMOTES;
+	else if ((*argcount = parse_long_opt("glob", argv, optarg)))
+		return REF_SELECT_BY_GLOB;
+
+	return REF_SELECT_NONE;
+}
+
+static int handle_refs_pseudo_opt(const char *submodule,
+				  struct rev_info *revs,
+				  int argc, const char **argv,
+				  int *flags, int *ret)
+{
+	struct all_refs_cb cb;
+	const char *optarg = NULL;
+	int argcount;
+	enum ref_selector selector;
+
+	selector = parse_ref_selector_option(argc, argv, &optarg, &argcount);
+
+	if (optarg)
+		init_all_refs_cb(&cb, revs, *flags);
+	*ret = 1;
+
+	switch (selector) {
+	case REF_SELECT_ALL:
+		handle_refs(submodule, revs, *flags, for_each_ref_submodule);
+		handle_refs(submodule, revs, *flags, head_ref_submodule);
+		break;
+
+	case REF_SELECT_BRANCHES:
+		if (optarg)
+			for_each_glob_ref_in(handle_one_ref, optarg,
+					     "refs/heads/", &cb);
+		else
+			handle_refs(submodule, revs, *flags,
+				    for_each_branch_ref_submodule);
+		break;
+
+	case REF_SELECT_TAGS:
+		if (optarg)
+			for_each_glob_ref_in(handle_one_ref, optarg,
+					     "refs/tags/", &cb);
+		else
+			handle_refs(submodule, revs, *flags,
+				    for_each_tag_ref_submodule);
+		break;
+
+	case REF_SELECT_REMOTES:
+		if (optarg)
+			for_each_glob_ref_in(handle_one_ref, optarg,
+					     "refs/remotes/", &cb);
+		else
+			handle_refs(submodule, revs, *flags,
+				    for_each_remote_ref_submodule);
+		break;
+
+	case REF_SELECT_BY_GLOB:
+		init_all_refs_cb(&cb, revs, *flags);
+		for_each_glob_ref(handle_one_ref, optarg, &cb);
+		*ret = argcount;
+		break;
+
+	case REF_SELECT_NONE:
+		return 0;
+	}
+
+	clear_ref_exclusion(&revs->ref_excludes);
+	return 1;
+}
+
 static int handle_revision_pseudo_opt(const char *submodule,
 				struct rev_info *revs,
 				int argc, const char **argv, int *flags)
@@ -2066,6 +2163,7 @@ static int handle_revision_pseudo_opt(const char *submodule,
 	const char *arg = argv[0];
 	const char *optarg;
 	int argcount;
+	int ret;
 
 	/*
 	 * NOTE!
@@ -2077,48 +2175,16 @@ static int handle_revision_pseudo_opt(const char *submodule,
 	 * When implementing your new pseudo-option, remember to
 	 * register it in the list at the top of handle_revision_opt.
 	 */
-	if (!strcmp(arg, "--all")) {
-		handle_refs(submodule, revs, *flags, for_each_ref_submodule);
-		handle_refs(submodule, revs, *flags, head_ref_submodule);
-		clear_ref_exclusion(&revs->ref_excludes);
-	} else if (!strcmp(arg, "--branches")) {
-		handle_refs(submodule, revs, *flags, for_each_branch_ref_submodule);
-		clear_ref_exclusion(&revs->ref_excludes);
+	if (handle_refs_pseudo_opt(submodule, revs, argc, argv, flags, &ret)) {
+		return ret;
 	} else if (!strcmp(arg, "--bisect")) {
 		read_bisect_terms(&term_bad, &term_good);
 		handle_refs(submodule, revs, *flags, for_each_bad_bisect_ref);
 		handle_refs(submodule, revs, *flags ^ (UNINTERESTING | BOTTOM), for_each_good_bisect_ref);
 		revs->bisect = 1;
-	} else if (!strcmp(arg, "--tags")) {
-		handle_refs(submodule, revs, *flags, for_each_tag_ref_submodule);
-		clear_ref_exclusion(&revs->ref_excludes);
-	} else if (!strcmp(arg, "--remotes")) {
-		handle_refs(submodule, revs, *flags, for_each_remote_ref_submodule);
-		clear_ref_exclusion(&revs->ref_excludes);
-	} else if ((argcount = parse_long_opt("glob", argv, &optarg))) {
-		struct all_refs_cb cb;
-		init_all_refs_cb(&cb, revs, *flags);
-		for_each_glob_ref(handle_one_ref, optarg, &cb);
-		clear_ref_exclusion(&revs->ref_excludes);
-		return argcount;
 	} else if ((argcount = parse_long_opt("exclude", argv, &optarg))) {
 		add_ref_exclusion(&revs->ref_excludes, optarg);
 		return argcount;
-	} else if (starts_with(arg, "--branches=")) {
-		struct all_refs_cb cb;
-		init_all_refs_cb(&cb, revs, *flags);
-		for_each_glob_ref_in(handle_one_ref, arg + 11, "refs/heads/", &cb);
-		clear_ref_exclusion(&revs->ref_excludes);
-	} else if (starts_with(arg, "--tags=")) {
-		struct all_refs_cb cb;
-		init_all_refs_cb(&cb, revs, *flags);
-		for_each_glob_ref_in(handle_one_ref, arg + 7, "refs/tags/", &cb);
-		clear_ref_exclusion(&revs->ref_excludes);
-	} else if (starts_with(arg, "--remotes=")) {
-		struct all_refs_cb cb;
-		init_all_refs_cb(&cb, revs, *flags);
-		for_each_glob_ref_in(handle_one_ref, arg + 10, "refs/remotes/", &cb);
-		clear_ref_exclusion(&revs->ref_excludes);
 	} else if (!strcmp(arg, "--reflog")) {
 		add_reflogs_to_pending(revs, *flags);
 	} else if (!strcmp(arg, "--indexed-objects")) {
-- 
2.11.0.157.gd943d85

[PATCH 3/5] revision.c: allow to change pseudo opt parsing function

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 revision.c | 11 ++++++++---
 revision.h |  4 ++++
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/revision.c b/revision.c
index 6ebd38d1c8..cda2606c66 100644
--- a/revision.c
+++ b/revision.c
@@ -2273,10 +2273,15 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 		const char *arg = argv[i];
 		if (*arg == '-') {
 			int opts;
+			handle_pseudo_opt_cb handle_pseudo_opt =
+				handle_revision_pseudo_opt;
 
-			opts = handle_revision_pseudo_opt(submodule,
-						revs, argc - i, argv + i,
-						&flags);
+			if (revs->handle_pseudo_opt)
+				handle_pseudo_opt = revs->handle_pseudo_opt;
+
+			opts = handle_pseudo_opt(submodule,
+						 revs, argc - i, argv + i,
+						 &flags);
 			if (opts > 0) {
 				i += opts - 1;
 				continue;
diff --git a/revision.h b/revision.h
index 9fac1a607d..9080eaf381 100644
--- a/revision.h
+++ b/revision.h
@@ -52,6 +52,8 @@ struct rev_cmdline_info {
 #define REVISION_WALK_NO_WALK_SORTED 1
 #define REVISION_WALK_NO_WALK_UNSORTED 2
 
+typedef int (*handle_pseudo_opt_cb)(const char *, struct rev_info *, int, const char **, int *);
+
 struct rev_info {
 	/* Starting list */
 	struct commit_list *commits;
@@ -213,6 +215,8 @@ struct rev_info {
 
 	struct commit_list *previous_parents;
 	const char *break_bar;
+
+	handle_pseudo_opt_cb handle_pseudo_opt;
 };
 
 extern int ref_excluded(struct string_list *, const char *path);
-- 
2.11.0.157.gd943d85

[PATCH 4/5] revision.c: refactor ref selection handler after --exclude

[Nguyễn Thái Ngọc Duy]

Behavior change: "--exclude --blah --remotes" will not exclude remote
branches any more. Only "--exclude --remotes" does.

This is because --exclude is going to have a new friend --decorate-reflog
who haves the same way. When you allow a distant --remotes to complement
a previous option, things get complicated. In

    --exclude .. --decorate-reflog ... --remotes

Does it mean decorate remote reflogs, or does it mean exclude remotes
from the selected revisions?

Granted, there may be valid use cases for such a combination (e.g.
"decorate all reflogs except remote ones") but I feel option order is
not a good fit to express them.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 revision.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/revision.c b/revision.c
index cda2606c66..45cffcab44 100644
--- a/revision.c
+++ b/revision.c
@@ -2152,10 +2152,24 @@ static int handle_refs_pseudo_opt(const char *submodule,
 		return 0;
 	}
 
-	clear_ref_exclusion(&revs->ref_excludes);
 	return 1;
 }
 
+static int handle_revision_pseudo_opt(const char *, struct rev_info *, int, const char **, int *);
+
+static int handle_revision_pseudo_opt_after_exclude(const char *submodule,
+						    struct rev_info *revs,
+						    int argc, const char **argv,
+						    int *flags)
+{
+	int ret;
+
+	ret = handle_revision_pseudo_opt(submodule, revs, argc, argv, flags);
+	clear_ref_exclusion(&revs->ref_excludes);
+	revs->handle_pseudo_opt = NULL;
+	return ret;
+}
+
 static int handle_revision_pseudo_opt(const char *submodule,
 				struct rev_info *revs,
 				int argc, const char **argv, int *flags)
@@ -2184,6 +2198,7 @@ static int handle_revision_pseudo_opt(const char *submodule,
 		revs->bisect = 1;
 	} else if ((argcount = parse_long_opt("exclude", argv, &optarg))) {
 		add_ref_exclusion(&revs->ref_excludes, optarg);
+		revs->handle_pseudo_opt = handle_revision_pseudo_opt_after_exclude;
 		return argcount;
 	} else if (!strcmp(arg, "--reflog")) {
 		add_reflogs_to_pending(revs, *flags);
-- 
2.11.0.157.gd943d85

[PATCH 5/5] revision.c: add --decorate-reflog

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 revision.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/revision.c b/revision.c
index 45cffcab44..b77face513 100644
--- a/revision.c
+++ b/revision.c
@@ -2157,6 +2157,49 @@ static int handle_refs_pseudo_opt(const char *submodule,
 
 static int handle_revision_pseudo_opt(const char *, struct rev_info *, int, const char **, int *);
 
+static int handle_revision_pseudo_opt_after_decorate_reflog(
+	const char *submodule, struct rev_info *revs,
+	int argc, const char **argv, int *flags)
+{
+	struct all_refs_cb cb;
+	const char *optarg = NULL;
+	int argcount;
+	enum ref_selector selector;
+
+	selector = parse_ref_selector_option(argc, argv, &optarg, &argcount);
+
+	if (optarg)
+		init_all_refs_cb(&cb, revs, *flags);
+
+	switch (selector) {
+	case REF_SELECT_ALL:
+		/* keep the info for load_ref_decorations() later */
+		return 1;
+
+	case REF_SELECT_BRANCHES:
+		/* keep the info for load_ref_decorations() later */
+		return 1;
+
+	case REF_SELECT_TAGS:
+		/* keep the info for load_ref_decorations() later */
+		return 1;
+
+	case REF_SELECT_REMOTES:
+		/* keep the info for load_ref_decorations() later */
+		return 1;
+
+	case REF_SELECT_BY_GLOB:
+		/* keep the info for load_ref_decorations() later */
+		return 1;
+
+	case REF_SELECT_NONE:
+		break;
+	}
+
+	revs->handle_pseudo_opt = NULL;
+	return handle_revision_pseudo_opt(submodule, revs, argc, argv, flags);
+}
+
 static int handle_revision_pseudo_opt_after_exclude(const char *submodule,
 						    struct rev_info *revs,
 						    int argc, const char **argv,
@@ -2200,6 +2243,9 @@ static int handle_revision_pseudo_opt(const char *submodule,
 		add_ref_exclusion(&revs->ref_excludes, optarg);
 		revs->handle_pseudo_opt = handle_revision_pseudo_opt_after_exclude;
 		return argcount;
+	} else if ((argcount = parse_long_opt("decorate-reflog", argv, &optarg))) {
+		revs->handle_pseudo_opt = handle_revision_pseudo_opt_after_decorate_reflog;
+		return argcount;
 	} else if (!strcmp(arg, "--reflog")) {
 		add_reflogs_to_pending(revs, *flags);
 	} else if (!strcmp(arg, "--indexed-objects")) {
-- 
2.11.0.157.gd943d85

Re: [PATCH 4/5] revision.c: refactor ref selection handler after --exclude

[Jacob Keller]

On Wed, Jan 25, 2017 at 4:50 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> Behavior change: "--exclude --blah --remotes" will not exclude remote
> branches any more. Only "--exclude --remotes" does.
>
> This is because --exclude is going to have a new friend --decorate-reflog
> who haves the same way. When you allow a distant --remotes to complement
> a previous option, things get complicated. In
>
>     --exclude .. --decorate-reflog ... --remotes
>
> Does it mean decorate remote reflogs, or does it mean exclude remotes
> from the selected revisions?
>
> Granted, there may be valid use cases for such a combination (e.g.
> "decorate all reflogs except remote ones") but I feel option order is
> not a good fit to express them.
>

Limiting the scope of the exclude seems somewhat reasonable to me,
because it makes it much easier to explain and show the user. We do
need to make sure it's not going to break any scripts or other issues.
Is it possible for us to produce an error if the user does "--exclude"
without a necessary connecting option?

Thanks,
Jake

Re: [PATCH 2/5] revision.c: group ref selection options together

[Jeff King]

On Wed, Jan 25, 2017 at 07:50:51PM +0700, Nguyễn Thái Ngọc Duy wrote:

> These options have on thing in common: when specified right after
> --exclude, they will de-select refs instead of selecting them by
> default.
> 
> This change makes it possible to introduce new options that use these
> options in the same way as --exclude. Such an option would just
> implement something like handle_refs_pseudo_opt().
> 
> parse_ref_selector_option() is taken out of handle_refs_pseudo_opt() so
> that similar functions like handle_refs_pseudo_opt() are forced to
> handle all ref selector options, not skipping some by mistake, which may
> revert the option back to default behavior (rev selection).
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  revision.c | 134 +++++++++++++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 100 insertions(+), 34 deletions(-)

Hmm. I see what you're trying to do here, and abstract the repeated
bits. But I'm not sure the line-count reflects a real simplification.
Everything ends up converted to an enum, and then that enum just expands
to similar C code.

I kind of expected that clear_ref_exclusion() would just become a more
abstract clear_ref_selection(). For now it would clear exclusions, and
then later learn to clear the decoration flags.

Maybe I am missing something in the later patches, though.

-Peff

Re: [PATCH 4/5] revision.c: refactor ref selection handler after --exclude

[Jeff King]

On Wed, Jan 25, 2017 at 07:50:53PM +0700, Nguyễn Thái Ngọc Duy wrote:

> Behavior change: "--exclude --blah --remotes" will not exclude remote
> branches any more. Only "--exclude --remotes" does.
> 
> This is because --exclude is going to have a new friend --decorate-reflog
> who haves the same way. When you allow a distant --remotes to complement
> a previous option, things get complicated. In
> 
>     --exclude .. --decorate-reflog ... --remotes
> 
> Does it mean decorate remote reflogs, or does it mean exclude remotes
> from the selected revisions?

I don't think it means either. It means to include remotes in the
selected revisions, but excluding the entries mentioned by --exclude.

IOW:

  --exclude=foo --remotes
	include all remotes except refs/remotes/foo

  --exclude=foo --unrelated --remotes
        same

  --exclude=foo --decorate-reflog --remotes
        decorate reflogs of all remotes except "foo". Do _not_ use them
	as traversal tips.

  --decorate-reflog --exclude=foo --remotes
        same

IOW, the ref-selector options build up until a group option is given,
which acts on the built-up options (over that group) and then resets the
built-up options. Doing "--unrelated" as above is orthogonal (though I
think in practice nobody would do that, because it's hard to read).

> Granted, there may be valid use cases for such a combination (e.g.
> "decorate all reflogs except remote ones") but I feel option order is
> not a good fit to express them.

That would be spelled:

  --exclude=refs/remotes --decorate-reflogs --all

(or you could swap the first two options).

Again, I'm not sure if I'm missing something subtle, or if you are
confused about how --exclude works. :)

-Peff

Re: [PATCH 2/5] revision.c: group ref selection options together

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> These options have on thing in common: when specified right after

one thing.

> --exclude, they will de-select refs instead of selecting them by
> default.
>
> This change makes it possible to introduce new options that use these
> options in the same way as --exclude. Such an option would just
> implement something like handle_refs_pseudo_opt().
>
> parse_ref_selector_option() is taken out of handle_refs_pseudo_opt() so
> that similar functions like handle_refs_pseudo_opt() are forced to
> handle all ref selector options, not skipping some by mistake, which may
> revert the option back to default behavior (rev selection).

I am not sure about these two refactorings, for at least two reasons.

 * Naming.  The function is all about handling "refs options" and I
   do not see anything "pseudo" about the options handled by the
   handle_refs_pseudo_opt() function.  This is minor.

 * I am expecting that the new one yet to be introduced will not
   share the huge "switch (selector)" part, but does its own things
   in a separate function with a similar structure.  The only thing
   common between these two functions would be the structure
   (i.e. it has a big "switch(selector)" that does different things
   depending on REF_SELECT_*) and a call to clear_* function.

   If we were to add a new kind of REF_SELECT_* (say
   REF_SELECT_NOTES just for the sake of being concrete), what
   changes will be needed to the code if the addition of "use reflog
   from this class of refs for decoration" feature was done with or
   without this step?  I have a suspicion that the change will be
   simpler without this step.

   The above comments will be invalid if the new "use reflog for
   decoration" feature will be done by extending this pseudo_opt()
   function by extending each of the switch/case arms.  If that is
   the case, please disregard the above.  I just didn't get that
   impression from the above paragraph.

Re: [PATCH 4/5] revision.c: refactor ref selection handler after --exclude

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Behavior change: "--exclude --blah --remotes" will not exclude remote
> branches any more. Only "--exclude --remotes" does.
>
> This is because --exclude is going to have a new friend --decorate-reflog
> who haves the same way. When you allow a distant --remotes to complement
> a previous option, things get complicated. In
>
>     --exclude .. --decorate-reflog ... --remotes
>
> Does it mean decorate remote reflogs, or does it mean exclude remotes
> from the selected revisions?

I would expect that the effect of exclude, decorate-reflog and
friends will extend until the next occurrence of --remotes (or --all
or whatever you catch in parse_ref_selector_option() function).

So, I'd read it as "add all remote tracking refs, but (1) exclude
exclude the refs matching pattern .. and (2) use reflog of them if
they match pattern ...".

Or did you mean by "..." something other than a single argument that
is a pattern?

Re: [PATCH 4/5] revision.c: refactor ref selection handler after --exclude

[Jeff King]

On Wed, Jan 25, 2017 at 03:57:18PM -0500, Jeff King wrote:

> IOW, the ref-selector options build up until a group option is given,
> which acts on the built-up options (over that group) and then resets the
> built-up options. Doing "--unrelated" as above is orthogonal (though I
> think in practice nobody would do that, because it's hard to read).

So here's what I would have expected your series to look more like (with
probably one patch adding clear_ref_selection_options, and the other
adding the decorate stuff):

diff --git a/revision.c b/revision.c
index b37dbec37..2f67707c7 100644
--- a/revision.c
+++ b/revision.c
@@ -1156,6 +1156,11 @@ static int handle_one_ref(const char *path, const struct object_id *oid,
 
 	if (ref_excluded(cb->all_revs->ref_excludes, path))
 	    return 0;
+	if (cb->all_revs->decorate_reflog) {
+		/* TODO actually do it for real */
+		warning("would decorate %s", path);
+		return 0; /* do not add it as a tip */
+	}
 
 	object = get_reference(cb->all_revs, path, oid->hash, cb->all_flags);
 	add_rev_cmdline(cb->all_revs, object, path, REV_CMD_REF, cb->all_flags);
@@ -1188,6 +1193,12 @@ void add_ref_exclusion(struct string_list **ref_excludes_p, const char *exclude)
 	string_list_append(*ref_excludes_p, exclude);
 }
 
+static void clear_ref_selection_options(struct rev_info *revs)
+{
+	clear_ref_exclusion(&revs->ref_excludes);
+	revs->decorate_reflog = 0;
+}
+
 static void handle_refs(const char *submodule, struct rev_info *revs, unsigned flags,
 		int (*for_each)(const char *, each_ref_fn, void *))
 {
@@ -2080,10 +2091,10 @@ static int handle_revision_pseudo_opt(const char *submodule,
 	if (!strcmp(arg, "--all")) {
 		handle_refs(submodule, revs, *flags, for_each_ref_submodule);
 		handle_refs(submodule, revs, *flags, head_ref_submodule);
-		clear_ref_exclusion(&revs->ref_excludes);
+		clear_ref_selection_options(revs);
 	} else if (!strcmp(arg, "--branches")) {
 		handle_refs(submodule, revs, *flags, for_each_branch_ref_submodule);
-		clear_ref_exclusion(&revs->ref_excludes);
+		clear_ref_selection_options(revs);
 	} else if (!strcmp(arg, "--bisect")) {
 		read_bisect_terms(&term_bad, &term_good);
 		handle_refs(submodule, revs, *flags, for_each_bad_bisect_ref);
@@ -2091,15 +2102,15 @@ static int handle_revision_pseudo_opt(const char *submodule,
 		revs->bisect = 1;
 	} else if (!strcmp(arg, "--tags")) {
 		handle_refs(submodule, revs, *flags, for_each_tag_ref_submodule);
-		clear_ref_exclusion(&revs->ref_excludes);
+		clear_ref_selection_options(revs);
 	} else if (!strcmp(arg, "--remotes")) {
 		handle_refs(submodule, revs, *flags, for_each_remote_ref_submodule);
-		clear_ref_exclusion(&revs->ref_excludes);
+		clear_ref_selection_options(revs);
 	} else if ((argcount = parse_long_opt("glob", argv, &optarg))) {
 		struct all_refs_cb cb;
 		init_all_refs_cb(&cb, revs, *flags);
 		for_each_glob_ref(handle_one_ref, optarg, &cb);
-		clear_ref_exclusion(&revs->ref_excludes);
+		clear_ref_selection_options(revs);
 		return argcount;
 	} else if ((argcount = parse_long_opt("exclude", argv, &optarg))) {
 		add_ref_exclusion(&revs->ref_excludes, optarg);
@@ -2108,17 +2119,19 @@ static int handle_revision_pseudo_opt(const char *submodule,
 		struct all_refs_cb cb;
 		init_all_refs_cb(&cb, revs, *flags);
 		for_each_glob_ref_in(handle_one_ref, arg + 11, "refs/heads/", &cb);
-		clear_ref_exclusion(&revs->ref_excludes);
+		clear_ref_selection_options(revs);
 	} else if (starts_with(arg, "--tags=")) {
 		struct all_refs_cb cb;
 		init_all_refs_cb(&cb, revs, *flags);
 		for_each_glob_ref_in(handle_one_ref, arg + 7, "refs/tags/", &cb);
-		clear_ref_exclusion(&revs->ref_excludes);
+		clear_ref_selection_options(revs);
 	} else if (starts_with(arg, "--remotes=")) {
 		struct all_refs_cb cb;
 		init_all_refs_cb(&cb, revs, *flags);
 		for_each_glob_ref_in(handle_one_ref, arg + 10, "refs/remotes/", &cb);
-		clear_ref_exclusion(&revs->ref_excludes);
+		clear_ref_selection_options(revs);
+	} else if (!strcmp(arg, "--decorate-reflog")) {
+		revs->decorate_reflog = 1;
 	} else if (!strcmp(arg, "--reflog")) {
 		add_reflogs_to_pending(revs, *flags);
 	} else if (!strcmp(arg, "--indexed-objects")) {
diff --git a/revision.h b/revision.h
index 9fac1a607..c74879829 100644
--- a/revision.h
+++ b/revision.h
@@ -66,6 +66,8 @@ struct rev_info {
 	/* excluding from --branches, --refs, etc. expansion */
 	struct string_list *ref_excludes;
 
+	int decorate_reflog;
+
 	/* Basic information */
 	const char *prefix;
 	const char *def;

Re: [PATCH 4/5] revision.c: refactor ref selection handler after --exclude

[Jacob Keller]

On Wed, Jan 25, 2017 at 1:27 PM, Jeff King <peff@peff.net> wrote:
> On Wed, Jan 25, 2017 at 03:57:18PM -0500, Jeff King wrote:
>
>> IOW, the ref-selector options build up until a group option is given,
>> which acts on the built-up options (over that group) and then resets the
>> built-up options. Doing "--unrelated" as above is orthogonal (though I
>> think in practice nobody would do that, because it's hard to read).
>
> So here's what I would have expected your series to look more like (with
> probably one patch adding clear_ref_selection_options, and the other
> adding the decorate stuff):
>

I agree that this is how I would have expected it to work as well.

Thanks,
Jake

> diff --git a/revision.c b/revision.c
> index b37dbec37..2f67707c7 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1156,6 +1156,11 @@ static int handle_one_ref(const char *path, const struct object_id *oid,
>
>         if (ref_excluded(cb->all_revs->ref_excludes, path))
>             return 0;
> +       if (cb->all_revs->decorate_reflog) {
> +               /* TODO actually do it for real */
> +               warning("would decorate %s", path);
> +               return 0; /* do not add it as a tip */
> +       }
>
>         object = get_reference(cb->all_revs, path, oid->hash, cb->all_flags);
>         add_rev_cmdline(cb->all_revs, object, path, REV_CMD_REF, cb->all_flags);
> @@ -1188,6 +1193,12 @@ void add_ref_exclusion(struct string_list **ref_excludes_p, const char *exclude)
>         string_list_append(*ref_excludes_p, exclude);
>  }
>
> +static void clear_ref_selection_options(struct rev_info *revs)
> +{
> +       clear_ref_exclusion(&revs->ref_excludes);
> +       revs->decorate_reflog = 0;
> +}
> +
>  static void handle_refs(const char *submodule, struct rev_info *revs, unsigned flags,
>                 int (*for_each)(const char *, each_ref_fn, void *))
>  {
> @@ -2080,10 +2091,10 @@ static int handle_revision_pseudo_opt(const char *submodule,
>         if (!strcmp(arg, "--all")) {
>                 handle_refs(submodule, revs, *flags, for_each_ref_submodule);
>                 handle_refs(submodule, revs, *flags, head_ref_submodule);
> -               clear_ref_exclusion(&revs->ref_excludes);
> +               clear_ref_selection_options(revs);
>         } else if (!strcmp(arg, "--branches")) {
>                 handle_refs(submodule, revs, *flags, for_each_branch_ref_submodule);
> -               clear_ref_exclusion(&revs->ref_excludes);
> +               clear_ref_selection_options(revs);
>         } else if (!strcmp(arg, "--bisect")) {
>                 read_bisect_terms(&term_bad, &term_good);
>                 handle_refs(submodule, revs, *flags, for_each_bad_bisect_ref);
> @@ -2091,15 +2102,15 @@ static int handle_revision_pseudo_opt(const char *submodule,
>                 revs->bisect = 1;
>         } else if (!strcmp(arg, "--tags")) {
>                 handle_refs(submodule, revs, *flags, for_each_tag_ref_submodule);
> -               clear_ref_exclusion(&revs->ref_excludes);
> +               clear_ref_selection_options(revs);
>         } else if (!strcmp(arg, "--remotes")) {
>                 handle_refs(submodule, revs, *flags, for_each_remote_ref_submodule);
> -               clear_ref_exclusion(&revs->ref_excludes);
> +               clear_ref_selection_options(revs);
>         } else if ((argcount = parse_long_opt("glob", argv, &optarg))) {
>                 struct all_refs_cb cb;
>                 init_all_refs_cb(&cb, revs, *flags);
>                 for_each_glob_ref(handle_one_ref, optarg, &cb);
> -               clear_ref_exclusion(&revs->ref_excludes);
> +               clear_ref_selection_options(revs);
>                 return argcount;
>         } else if ((argcount = parse_long_opt("exclude", argv, &optarg))) {
>                 add_ref_exclusion(&revs->ref_excludes, optarg);
> @@ -2108,17 +2119,19 @@ static int handle_revision_pseudo_opt(const char *submodule,
>                 struct all_refs_cb cb;
>                 init_all_refs_cb(&cb, revs, *flags);
>                 for_each_glob_ref_in(handle_one_ref, arg + 11, "refs/heads/", &cb);
> -               clear_ref_exclusion(&revs->ref_excludes);
> +               clear_ref_selection_options(revs);
>         } else if (starts_with(arg, "--tags=")) {
>                 struct all_refs_cb cb;
>                 init_all_refs_cb(&cb, revs, *flags);
>                 for_each_glob_ref_in(handle_one_ref, arg + 7, "refs/tags/", &cb);
> -               clear_ref_exclusion(&revs->ref_excludes);
> +               clear_ref_selection_options(revs);
>         } else if (starts_with(arg, "--remotes=")) {
>                 struct all_refs_cb cb;
>                 init_all_refs_cb(&cb, revs, *flags);
>                 for_each_glob_ref_in(handle_one_ref, arg + 10, "refs/remotes/", &cb);
> -               clear_ref_exclusion(&revs->ref_excludes);
> +               clear_ref_selection_options(revs);
> +       } else if (!strcmp(arg, "--decorate-reflog")) {
> +               revs->decorate_reflog = 1;
>         } else if (!strcmp(arg, "--reflog")) {
>                 add_reflogs_to_pending(revs, *flags);
>         } else if (!strcmp(arg, "--indexed-objects")) {
> diff --git a/revision.h b/revision.h
> index 9fac1a607..c74879829 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -66,6 +66,8 @@ struct rev_info {
>         /* excluding from --branches, --refs, etc. expansion */
>         struct string_list *ref_excludes;
>
> +       int decorate_reflog;
> +
>         /* Basic information */
>         const char *prefix;
>         const char *def;

Re: [PATCH 4/5] revision.c: refactor ref selection handler after --exclude

[Junio C Hamano]

Jacob Keller <jacob.keller@gmail.com> writes:

> On Wed, Jan 25, 2017 at 1:27 PM, Jeff King <peff@peff.net> wrote:
>> On Wed, Jan 25, 2017 at 03:57:18PM -0500, Jeff King wrote:
>>
>>> IOW, the ref-selector options build up until a group option is given,
>>> which acts on the built-up options (over that group) and then resets the
>>> built-up options. Doing "--unrelated" as above is orthogonal (though I
>>> think in practice nobody would do that, because it's hard to read).
>>
>> So here's what I would have expected your series to look more like (with
>> probably one patch adding clear_ref_selection_options, and the other
>> adding the decorate stuff):
>>
>
> I agree that this is how I would have expected it to work as well.

That makes three of us ;-)

Re: [PATCH 2/5] revision.c: group ref selection options together

[Duy Nguyen]

On Thu, Jan 26, 2017 at 4:11 AM, Junio C Hamano <gitster@pobox.com> wrote:
>  * I am expecting that the new one yet to be introduced will not
>    share the huge "switch (selector)" part, but does its own things
>    in a separate function with a similar structure.  The only thing
>    common between these two functions would be the structure
>    (i.e. it has a big "switch(selector)" that does different things
>    depending on REF_SELECT_*) and a call to clear_* function.

Yep. The "new one" is demonstrated in 5/5.

>    If we were to add a new kind of REF_SELECT_* (say
>    REF_SELECT_NOTES just for the sake of being concrete), what
>    changes will be needed to the code if the addition of "use reflog
>    from this class of refs for decoration" feature was done with or
>    without this step?  I have a suspicion that the change will be
>    simpler without this step.

The switch/case is to deal with new REF_SELECT_* (at least it's how I
imagine it). What I was worried about was, when a user adds
--select-notes, they may not be aware that it's in the same
all/branches/tags/remotes group that's supposed to work with
--decorate-reflog as well, and as a result "--decorate-reflog
--select-notes" is the same as "--select-notes".

With the switch/case, when you add a new enum item, at the least the
compiler should warn about unhandled cases. And we can have a new
"case REF_SELECT_NOTES:" for both --exclude and --decorate-reflog.
Without the switch/case, I guess it's still possible to do something
like

if (!strcmp(arg, "--select-notes")) {
    if (preceded_by_exclude())
        does_one_thing();
    else if (preceded_by_decorate_reflog())
       does_another_thing();
}

It's probably easier to maintain though, if all
decorate-reflog-related things are grouped together, rather than
spread out per option like the above.
-- 
Duy

Re: [PATCH 2/5] revision.c: group ref selection options together

[Duy Nguyen]

On Thu, Jan 26, 2017 at 3:50 AM, Jeff King <peff@peff.net> wrote:
> On Wed, Jan 25, 2017 at 07:50:51PM +0700, Nguyễn Thái Ngọc Duy wrote:
>
>> These options have on thing in common: when specified right after
>> --exclude, they will de-select refs instead of selecting them by
>> default.
>>
>> This change makes it possible to introduce new options that use these
>> options in the same way as --exclude. Such an option would just
>> implement something like handle_refs_pseudo_opt().
>>
>> parse_ref_selector_option() is taken out of handle_refs_pseudo_opt() so
>> that similar functions like handle_refs_pseudo_opt() are forced to
>> handle all ref selector options, not skipping some by mistake, which may
>> revert the option back to default behavior (rev selection).
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>>  revision.c | 134 +++++++++++++++++++++++++++++++++++++++++++++----------------
>>  1 file changed, 100 insertions(+), 34 deletions(-)
>
> Hmm. I see what you're trying to do here, and abstract the repeated
> bits. But I'm not sure the line-count reflects a real simplification.
> Everything ends up converted to an enum, and then that enum just expands
> to similar C code.

It's not simplification, but hopefully for better maintainability. This

if (strcmp(arg, "--remotes")) {
   if (preceded_by_exclide())
      does_something();
   else if (preceded_by_decorate())
      does_another()
} else if (strcmp(arg, "--branches")) {
   if (preceded_by_exclide())
      does_something();
   else if (preceded_by_decorate())
      does_another()
}

starts to look ugly especially when the third "preceded_by_" comes
into picture. Putting all "does_something" in one group and
"does_another" in another, I think, gives us a better view how ref
selection is handled for a specific operation like --exclude or
--decorate-ref.

> I kind of expected that clear_ref_exclusion() would just become a more
> abstract clear_ref_selection(). For now it would clear exclusions, and
> then later learn to clear the decoration flags.

It may go that way, depending on how we handle these options for
decorate-reflog. The current load_ref_decorations() is not really
suited for fine-grained ref selection yet.
--
Duy

Re: [PATCH 4/5] revision.c: refactor ref selection handler after --exclude

[Duy Nguyen]

On Thu, Jan 26, 2017 at 3:57 AM, Jeff King <peff@peff.net> wrote:
> I don't think it means either. It means to include remotes in the
> selected revisions, but excluding the entries mentioned by --exclude.
>
> IOW:
>
>   --exclude=foo --remotes
>         include all remotes except refs/remotes/foo
>
>   --exclude=foo --unrelated --remotes
>         same
>
>   --exclude=foo --decorate-reflog --remotes
>         decorate reflogs of all remotes except "foo". Do _not_ use them
>         as traversal tips.
>
>   --decorate-reflog --exclude=foo --remotes
>         same
>
> IOW, the ref-selector options build up until a group option is given,
> which acts on the built-up options (over that group) and then resets the
> built-up options. Doing "--unrelated" as above is orthogonal (though I
> think in practice nobody would do that, because it's hard to read).

This is because it makes sense to combine --exclude and
--decorate-reflog. But what about a new --something that conflicts
with either --exclude or --decorate-reflog? Should we simply catch
such combinations and error out (which may be a bit more complicated
than this patch, or maybe not)?
-- 
Duy

Re: [PATCH 2/5] revision.c: group ref selection options together

[Jeff King]

On Thu, Jan 26, 2017 at 04:18:06PM +0700, Duy Nguyen wrote:

> > Hmm. I see what you're trying to do here, and abstract the repeated
> > bits. But I'm not sure the line-count reflects a real simplification.
> > Everything ends up converted to an enum, and then that enum just expands
> > to similar C code.
> 
> It's not simplification, but hopefully for better maintainability. This
> 
> if (strcmp(arg, "--remotes")) {
>    if (preceded_by_exclide())
>       does_something();
>    else if (preceded_by_decorate())
>       does_another()
> } else if (strcmp(arg, "--branches")) {
>    if (preceded_by_exclide())
>       does_something();
>    else if (preceded_by_decorate())
>       does_another()
> }
> 
> starts to look ugly especially when the third "preceded_by_" comes
> into picture. Putting all "does_something" in one group and
> "does_another" in another, I think, gives us a better view how ref
> selection is handled for a specific operation like --exclude or
> --decorate-ref.

I agree that would be ugly. But the current structure, which is:

  if (strcmp(arg, "--remotes")) {
          handle_refs(...);
	  cleanup();
  } else if(...) {
          handle_refs(...);
	  cleanup();
  }

does not seem so bad, and pushes those conditionals into the
handle_refs() function, where they only need to be expressed once (I
didn't look, but I wonder if you could push the cleanup steps in there,
too, or if there is a caller who wants to handle() multiple times before
cleaning up).

-Peff

Re: [PATCH 4/5] revision.c: refactor ref selection handler after --exclude

[Jeff King]

On Thu, Jan 26, 2017 at 04:28:17PM +0700, Duy Nguyen wrote:

> On Thu, Jan 26, 2017 at 3:57 AM, Jeff King <peff@peff.net> wrote:
> > I don't think it means either. It means to include remotes in the
> > selected revisions, but excluding the entries mentioned by --exclude.
> >
> > IOW:
> >
> >   --exclude=foo --remotes
> >         include all remotes except refs/remotes/foo
> >
> >   --exclude=foo --unrelated --remotes
> >         same
> >
> >   --exclude=foo --decorate-reflog --remotes
> >         decorate reflogs of all remotes except "foo". Do _not_ use them
> >         as traversal tips.
> >
> >   --decorate-reflog --exclude=foo --remotes
> >         same
> >
> > IOW, the ref-selector options build up until a group option is given,
> > which acts on the built-up options (over that group) and then resets the
> > built-up options. Doing "--unrelated" as above is orthogonal (though I
> > think in practice nobody would do that, because it's hard to read).
> 
> This is because it makes sense to combine --exclude and
> --decorate-reflog. But what about a new --something that conflicts
> with either --exclude or --decorate-reflog? Should we simply catch
> such combinations and error out (which may be a bit more complicated
> than this patch, or maybe not)?

I'd cross that bridge when we see what the option is. But my gut is that
rules would be:

  - apply all non-conflicting relevant options. So:

      --exclude=foo/* --decorate-refs --decorate-reflog --remotes

    would presumably decorate both ref tips _and_ reflogs for all
    remotes (except ones in refs/remotes/foo/*)

  - for ones that are directly related and override each other,
    use the usual last-one-wins rule. So:

      --decorate-reflog --no-decorate-reflog --remotes

    would countermand the original --decorate-reflog.

  - for ones that really have complex interactions, notice and complain
    in handle_refs().

That just seems to me like it follows our usual option parsing
procedure. The only difference here is that process and reset some
subset of the flags when we hit a special marker option ("--remotes" in
these examples) instead of doing it at the end.

-Peff

Re: [PATCH 4/5] revision.c: refactor ref selection handler after --exclude

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Thu, Jan 26, 2017 at 3:57 AM, Jeff King <peff@peff.net> wrote:
>> I don't think it means either. It means to include remotes in the
>> selected revisions, but excluding the entries mentioned by --exclude.
>>
>> IOW:
>>
>>   --exclude=foo --remotes
>>         include all remotes except refs/remotes/foo
>>
>>   --exclude=foo --unrelated --remotes
>>         same
>>
>>   --exclude=foo --decorate-reflog --remotes
>>         decorate reflogs of all remotes except "foo". Do _not_ use them
>>         as traversal tips.
>>
>>   --decorate-reflog --exclude=foo --remotes
>>         same
>>
>> IOW, the ref-selector options build up until a group option is given,
>> which acts on the built-up options (over that group) and then resets the
>> built-up options. Doing "--unrelated" as above is orthogonal (though I
>> think in practice nobody would do that, because it's hard to read).
>
> This is because it makes sense to combine --exclude and
> --decorate-reflog. But what about a new --something that conflicts
> with either --exclude or --decorate-reflog?

I would think that "--exclude=foo --something --remotes" 

 * should be diagnosed as an error if "--something" is not compatible
   with "--exclude";

 * should take effect at the concluding "--remotes" if "--something"
   is similar to "--decorate-reflog" whose effect ends at a
   concluding --remotes/--branches/etc.; and

 * should work independently if "--something" is neither.