patchlog: new option decorate reflog of remote refs

> 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.

> > 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.

>> 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

> 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.

> 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(-)

> 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.

> > 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.

> --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 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.

> 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.
>

> 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?

> 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;

> 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.

> 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).

> 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)?

> 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?

> 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?