log: 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 <[email protected]>

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

Documentation/git-log.txt OPTIONS · line 44

Quoted 4 lines

+--decorate-remote-reflog[=<n>]::

Documentation/git-log.txt +3 -0

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

Quoted 9 lines

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:

Quoted 7 lines

  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.

Quoted 8 lines

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.

Quoted 3 lines

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

Quoted 38 lines · Jeff King

Quoted 8 lines

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.

Quoted 3 lines

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.

Quoted 1 line

-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 <[email protected]>

---
 revision.c | 134 +++++++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 100 insertions(+), 34 deletions(-)

Quoted 13 lines · 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).

Quoted 4 lines

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

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

Quoted 4 lines

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.

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

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

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.

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

Quoted 4 lines · Jeff King

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

revision.c +21 -8

index b37dbec37..2f67707c7 100644
@@ -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")) {

revision.h +2 -0

index 9fac1a607..c74879829 100644
@@ -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;

Quoted 7 lines · Jeff King

Quoted 4 lines · Jeff King

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

Quoted 4 lines

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

Quoted 15 lines · Jeff King

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.

Quoted 7 lines

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

Quoted 18 lines · Jeff King

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

Quoted 4 lines

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?