{"thread":{"id":"44914","subject":"[PATCH] log: new option decorate reflog of remote refs","startedAt":"2017-01-19T12:26:50Z","lastAt":"2017-01-26T18:47:10Z","messageCount":27,"participants":["Nguyễn Thái Ngọc Duy","Jeff King","Duy Nguyen","Jacob Keller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"309724","messageId":"20170119122630.27645-1-pclouds@gmail.com","threadId":"44914","inReplyTo":null,"subject":"[PATCH] log: new option decorate reflog of remote refs","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-01-19T12:26:30Z","receivedAt":"2017-01-19T12:26:50Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This is most useful when you fork your branches off a remote ref and\nrely on ref decoration to show your fork points in `git log`. Then you\ndo a \"git fetch\" and suddenly the remote decoration is gone because\nremote refs are moved forward. With this, we can still see something\nlike \"origin/foo@{1}\"\n\nThis is for remote refs only because based on my experience, docorating\nlocal reflog is just too noisy. You will most likely see HEAD@{1},\nHEAD@{2} and so on. We can add that as a separate option in future if we\nsee a need for it.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n I've been using this for many weeks and it has proven its usefulness\n (to me). Looks like good material to send upstream.\n\n Documentation/git-log.txt | 5 +++++\n builtin/log.c | 10 +++++++++-\n log-tree.c | 43 +++++++++++++++++++++++++++++++++++++++----\n log-tree.h | 2 +-\n pretty.c | 4 ++--\n revision.c | 2 +-\n 6 files changed, 57 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/git-log.txt b/Documentation/git-log.txt\nindex 32246fd..f5ee575 100644\n--- a/Documentation/git-log.txt\n+++ b/Documentation/git-log.txt\n@@ -38,6 +38,11 @@ OPTIONS\n \tare shown as if 'short' were given, otherwise no ref names are\n \tshown. The default option is 'short'.\n \n+--decorate-remote-reflog[=]::\n+\tDecorate `` most recent reflog entries on remote refs, up\n+\tto the specified number of entries. By default, only the most\n+\trecent reflog entry is decorated.\n+\n --source::\n \tPrint out the ref name given on the command line by which each\n \tcommit was reached.\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 55d20cc..c208703 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -36,6 +36,7 @@ static int default_follow;\n static int default_show_signature;\n static int decoration_style;\n static int decoration_given;\n+static int decorate_remote_reflog;\n static int use_mailmap_config;\n static const char *fmt_patch_subject_prefix = \"PATCH\";\n static const char *fmt_pretty;\n@@ -141,6 +142,10 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,\n \t\tOPT_BOOL(0, \"use-mailmap\", &mailmap, N_(\"Use mail map file\")),\n \t\t{ OPTION_CALLBACK, 0, \"decorate\", NULL, NULL, N_(\"decorate options\"),\n \t\t PARSE_OPT_OPTARG, decorate_callback},\n+\t\t{ OPTION_INTEGER, 0, \"decorate-remote-reflog\",\n+\t\t &decorate_remote_reflog, N_(\"n\"),\n+\t\t N_(\"decorate the last reflog entries of remote refs\"),\n+\t\t PARSE_OPT_OPTARG | PARSE_OPT_NONEG, NULL, 1 },\n \t\tOPT_CALLBACK('L', NULL, &line_cb, \"n,m:file\",\n \t\t\t N_(\"Process line range n,m in file, counting from 1\"),\n \t\t\t log_line_range_callback),\n@@ -195,9 +200,12 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,\n \t\t\trev->abbrev_commit = 0;\n \t}\n \n+\tif (decorate_remote_reflog > 0 && !decoration_style)\n+\t\tdecoration_style = DECORATE_SHORT_REFS;\n \tif (decoration_style) {\n \t\trev->show_decorations = 1;\n-\t\tload_ref_decorations(decoration_style);\n+\t\tload_ref_decorations(decoration_style,\n+\t\t\t\t decorate_remote_reflog);\n \t}\n \n \tif (rev->line_level_traverse)\ndiff --git a/log-tree.c b/log-tree.c\nindex 8c24157..3d85ebc 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -88,14 +88,37 @@ const struct name_decoration *get_name_decoration(const struct object *obj)\n \treturn lookup_decoration(&name_decoration, obj);\n }\n \n+struct reflog_cb {\n+\tint type;\n+\tint count;\n+\tint nth;\n+\tconst char *refname;\n+};\n+\n+static int add_nth_reflog(unsigned char *osha1, unsigned char *nsha1,\n+\t\t\t const char *email, unsigned long timestamp, int tz,\n+\t\t\t const char *message, void *cb_data)\n+{\n+\tstruct reflog_cb *cb = cb_data;\n+\tstruct commit *commit;\n+\n+\tcommit = lookup_commit(nsha1);\n+\tif (commit && cb->nth) {\n+\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\tstrbuf_addf(&sb, \"%s@{%d}\", cb->refname, cb->nth);\n+\t\tadd_name_decoration(cb->type, sb.buf, &commit->object);\n+\t\tstrbuf_release(&sb);\n+\t}\n+\tcb->nth++;\n+\treturn cb->nth >= cb->count;\n+}\n+\n static int add_ref_decoration(const char *refname, const struct object_id *oid,\n \t\t\t int flags, void *cb_data)\n {\n \tstruct object *obj;\n \tenum decoration_type type = DECORATION_NONE;\n \n-\tassert(cb_data == NULL);\n-\n \tif (starts_with(refname, git_replace_ref_base)) {\n \t\tstruct object_id original_oid;\n \t\tif (!check_replace_refs)\n@@ -135,6 +158,17 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid,\n \t\t\tparse_object(obj->oid.hash);\n \t\tadd_name_decoration(DECORATION_REF_TAG, refname, obj);\n \t}\n+\n+\tif (cb_data && type == DECORATION_REF_REMOTE) {\n+\t\tstruct reflog_cb cb;\n+\n+\t\tmemset(&cb, 0, sizeof(cb));\n+\t\tcb.refname = refname;\n+\t\tcb.type = type;\n+\t\tcb.count = *(int *)cb_data + 1 /* for @{0} */;\n+\n+\t\tfor_each_reflog_ent_reverse(refname, add_nth_reflog, &cb);\n+\t}\n \treturn 0;\n }\n \n@@ -147,13 +181,14 @@ static int add_graft_decoration(const struct commit_graft *graft, void *cb_data)\n \treturn 0;\n }\n \n-void load_ref_decorations(int flags)\n+void load_ref_decorations(int flags, int remote_reflog)\n {\n \tif (!decoration_loaded) {\n+\t\tvoid *cb = remote_reflog ? &remote_reflog : NULL;\n \n \t\tdecoration_loaded = 1;\n \t\tdecoration_flags = flags;\n-\t\tfor_each_ref(add_ref_decoration, NULL);\n+\t\tfor_each_ref(add_ref_decoration, cb);\n \t\thead_ref(add_ref_decoration, NULL);\n \t\tfor_each_commit_graft(add_graft_decoration, NULL);\n \t}\ndiff --git a/log-tree.h b/log-tree.h\nindex c8116e6..bb46c53 100644\n--- a/log-tree.h\n+++ b/log-tree.h\n@@ -25,7 +25,7 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n \t\t\t const char **subject_p,\n \t\t\t const char **extra_headers_p,\n \t\t\t int *need_8bit_cte_p);\n-void load_ref_decorations(int flags);\n+void load_ref_decorations(int flags, int reflog);\n \n #define FORMAT_PATCH_NAME_MAX 64\n void fmt_output_commit(struct strbuf *, struct commit *, struct rev_info *);\ndiff --git a/pretty.c b/pretty.c\nindex 5e68383..ec8e1cc 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1189,11 +1189,11 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\tstrbuf_addstr(sb, get_revision_mark(NULL, commit));\n \t\treturn 1;\n \tcase 'd':\n-\t\tload_ref_decorations(DECORATE_SHORT_REFS);\n+\t\tload_ref_decorations(DECORATE_SHORT_REFS, 0);\n \t\tformat_decorations(sb, commit, c->auto_color);\n \t\treturn 1;\n \tcase 'D':\n-\t\tload_ref_decorations(DECORATE_SHORT_REFS);\n+\t\tload_ref_decorations(DECORATE_SHORT_REFS, 0);\n \t\tformat_decorations_extended(sb, commit, c->auto_color, \"\", \", \", \"\");\n \t\treturn 1;\n \tcase 'g':\t\t/* reflog info */\ndiff --git a/revision.c b/revision.c\nindex b37dbec..4d5cbf5 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1743,7 +1743,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->simplify_by_decoration = 1;\n \t\trevs->limited = 1;\n \t\trevs->prune = 1;\n-\t\tload_ref_decorations(DECORATE_SHORT_REFS);\n+\t\tload_ref_decorations(DECORATE_SHORT_REFS, 0);\n \t} else if (!strcmp(arg, \"--date-order\")) {\n \t\trevs->sort_order = REV_SORT_BY_COMMIT_DATE;\n \t\trevs->topo_order = 1;\n-- \n2.8.2.524.g6ff3d78\n\n"},{"id":"309741","messageId":"20170119172310.6meuj6ksxloeee2t@sigill.intra.peff.net","threadId":"44914","inReplyTo":"20170119122630.27645-1-pclouds@gmail.com","subject":"Re: [PATCH] log: new option decorate reflog of remote refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-19T17:23:10Z","receivedAt":"2017-01-19T17:44:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 19, 2017 at 07:26:30PM +0700, Nguyễn Thái Ngọc Duy wrote:\n\n> This is most useful when you fork your branches off a remote ref and\n> rely on ref decoration to show your fork points in `git log`. Then you\n> do a \"git fetch\" and suddenly the remote decoration is gone because\n> remote refs are moved forward. With this, we can still see something\n> like \"origin/foo@{1}\"\n> \n> This is for remote refs only because based on my experience, docorating\n> local reflog is just too noisy. You will most likely see HEAD@{1},\n> HEAD@{2} and so on. We can add that as a separate option in future if we\n> see a need for it.\n> \n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> I've been using this for many weeks and it has proven its usefulness\n> (to me). Looks like good material to send upstream.\n\nI think it's a neat idea, but the actual option:\n\n> +--decorate-remote-reflog[=]::\n> +\tDecorate `` most recent reflog entries on remote refs, up\n> +\tto the specified number of entries. By default, only the most\n> +\trecent reflog entry is decorated.\n\nseems weirdly limited and non-orthogonal. What happens when somebody\nwants to decorate other reflogs besides refs/remotes?\n\nWe already have very flexible ref-selectors like --remotes, --branches,\netc. The generalization of this would perhaps be something like:\n\n git log --decorate-reflog --remotes --branches\n\nwhere \"--decorate-reflog\" applies to the next ref selector and then is\nreset, the same way --exclude is. And it includes those refs _only_ for\ndecoration, not for traversal. So you could do:\n\n git log --decorate-reflog --remotes --remotes\n\nif you wanted to see use those as traversal roots, too (if this is\ncommon, it might even merit another option for \"decorate and show\").\n\nThat's just off the top of my head, so maybe there are issues. I was\njust surprised to see the \"-remote\" part in your option name.\n\n-Peff\n"},{"id":"309818","messageId":"CACsJy8A_LkRMZYfoJuYEUok4r7Tw0VuMwVkG_Kr1o1hFcAUWNw@mail.gmail.com","threadId":"44914","inReplyTo":"20170119172310.6meuj6ksxloeee2t@sigill.intra.peff.net","subject":"Re: [PATCH] log: new option decorate reflog of remote refs","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-01-20T10:55:21Z","receivedAt":"2017-01-20T10:56:04Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Jan 20, 2017 at 12:23 AM, Jeff King wrote:\n> I think it's a neat idea, but the actual option:\n>\n>> +--decorate-remote-reflog[=]::\n>> + Decorate `` most recent reflog entries on remote refs, up\n>> + to the specified number of entries. By default, only the most\n>> + recent reflog entry is decorated.\n>\n> seems weirdly limited and non-orthogonal. What happens when somebody\n> wants to decorate other reflogs besides refs/remotes?\n>\n> We already have very flexible ref-selectors like --remotes, --branches,\n> etc. The generalization of this would perhaps be something like:\n>\n> git log --decorate-reflog --remotes --branches\n>\n> where \"--decorate-reflog\" applies to the next ref selector and then is\n> reset, the same way --exclude is. And it includes those refs _only_ for\n> decoration, not for traversal. So you could do:\n>\n> git log --decorate-reflog --remotes --remotes\n>\n> if you wanted to see use those as traversal roots, too (if this is\n> common, it might even merit another option for \"decorate and show\").\n>\n> That's just off the top of my head, so maybe there are issues. I was\n> just surprised to see the \"-remote\" part in your option name.\n\nImposing order between options could cause confusion, I think, if you\nremove --decorate-reflog leaving --remotes on by accident, now you get\n--remotes with a new meaning. We could go with something like\n--decodate-reflog=remote, but that clashes with the number of reflog\nentries and we may need a separator, like --decorate-reflog=remote,3.\nOr we could add something to --decorate= in addition to\nshort|full|auto|no. Something like --decorate=full,reflog or\n--decorate=full,reflog=remote,entries=3 if I want 3 reflog entries.\n\nMy hesitant to go that far is because I suspect decorating reflog\nwon't be helpful for non-remotes. But I'm willing to make more changes\nif it opens door to master.\n-- \nDuy\n"},{"id":"309833","messageId":"20170120143031.p2mux5uxxzniovkx@sigill.intra.peff.net","threadId":"44914","inReplyTo":"CACsJy8A_LkRMZYfoJuYEUok4r7Tw0VuMwVkG_Kr1o1hFcAUWNw@mail.gmail.com","subject":"Re: [PATCH] log: new option decorate reflog of remote refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-20T14:30:32Z","receivedAt":"2017-01-20T14:31:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 20, 2017 at 05:55:21PM +0700, Duy Nguyen wrote:\n\n> > We already have very flexible ref-selectors like --remotes, --branches,\n> > etc. The generalization of this would perhaps be something like:\n> >\n> > git log --decorate-reflog --remotes --branches\n> >\n> > where \"--decorate-reflog\" applies to the next ref selector and then is\n> > reset, the same way --exclude is. And it includes those refs _only_ for\n> > decoration, not for traversal. So you could do:\n> >\n> > git log --decorate-reflog --remotes --remotes\n> >\n> > if you wanted to see use those as traversal roots, too (if this is\n> > common, it might even merit another option for \"decorate and show\").\n> >\n> > That's just off the top of my head, so maybe there are issues. I was\n> > just surprised to see the \"-remote\" part in your option name.\n> \n> Imposing order between options could cause confusion, I think, if you\n> remove --decorate-reflog leaving --remotes on by accident, now you get\n> --remotes with a new meaning. We could go with something like\n> --decodate-reflog=remote, but that clashes with the number of reflog\n> entries and we may need a separator, like --decorate-reflog=remote,3.\n> Or we could add something to --decorate= in addition to\n> short|full|auto|no. Something like --decorate=full,reflog or\n> --decorate=full,reflog=remote,entries=3 if I want 3 reflog entries.\n\nI agree that making option-order important is potentially confusing. But\nit does already exist with --exclude. It's necessary to specify some\nsets of refs (e.g., all of A, except for those that match B, and then\nall of C, including those that match B).\n\nHaving --decorate-reflog=remote would be similarly constrained. You\ncouldn't do \"decorate all remotes except for these ones\". For that\nmatter, I'm not sure how you would do \"decorate just the refs from\norigin\".\n\nI'll grant that those are going to be a lot less common than just \"all\nthe remotes\" (or all the tags, or whatever). I'd just hate to see us\nrevisiting this in a year to generalize it, and being stuck with\nhistorical baggage.\n\n> My hesitant to go that far is because I suspect decorating reflog\n> won't be helpful for non-remotes. But I'm willing to make more changes\n> if it opens door to master.\n\nForgetting reflogs for a moment, I'd actually find it useful to just\ndecorate tags and local branches, but not remotes. But right now there\nisn't any way to select which refs are worthy of decoration (reflog or\nnot).\n\nThat's why I'm thinking so much about a general ref-selection system. I\nagree the \"--exclude=... --remotes\" thing is complicated, but it's also\nthe ref-selection system we _already_ have, which to me is a slight\npoint in its favor.\n\n-Peff\n"},{"id":"309861","messageId":"CA+P7+xqgyUW-Wt2oUSCc86HG-MfL-itAu2yVrZ219LNwqQnwKw@mail.gmail.com","threadId":"44914","inReplyTo":"20170120143031.p2mux5uxxzniovkx@sigill.intra.peff.net","subject":"Re: [PATCH] log: new option decorate reflog of remote refs","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2017-01-20T22:00:03Z","receivedAt":"2017-01-20T22:03:28Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Fri, Jan 20, 2017 at 6:30 AM, Jeff King wrote:\n>> Imposing order between options could cause confusion, I think, if you\n>> remove --decorate-reflog leaving --remotes on by accident, now you get\n>> --remotes with a new meaning. We could go with something like\n>> --decodate-reflog=remote, but that clashes with the number of reflog\n>> entries and we may need a separator, like --decorate-reflog=remote,3.\n>> Or we could add something to --decorate= in addition to\n>> short|full|auto|no. Something like --decorate=full,reflog or\n>> --decorate=full,reflog=remote,entries=3 if I want 3 reflog entries.\n>\n> I agree that making option-order important is potentially confusing. But\n> it does already exist with --exclude. It's necessary to specify some\n> sets of refs (e.g., all of A, except for those that match B, and then\n> all of C, including those that match B).\n>\n> Having --decorate-reflog=remote would be similarly constrained. You\n> couldn't do \"decorate all remotes except for these ones\". For that\n> matter, I'm not sure how you would do \"decorate just the refs from\n> origin\".\n>\n> I'll grant that those are going to be a lot less common than just \"all\n> the remotes\" (or all the tags, or whatever). I'd just hate to see us\n> revisiting this in a year to generalize it, and being stuck with\n> historical baggage.\n>\n>> My hesitant to go that far is because I suspect decorating reflog\n>> won't be helpful for non-remotes. But I'm willing to make more changes\n>> if it opens door to master.\n>\n> Forgetting reflogs for a moment, I'd actually find it useful to just\n> decorate tags and local branches, but not remotes. But right now there\n> isn't any way to select which refs are worthy of decoration (reflog or\n> not).\n>\n> That's why I'm thinking so much about a general ref-selection system. I\n> agree the \"--exclude=... --remotes\" thing is complicated, but it's also\n> the ref-selection system we _already_ have, which to me is a slight\n> point in its favor.\n>\n> -Peff\n\nI agree that the interaction between --exclude and --remotes/etc is\nconfusing, but I think it's reasonable enough because we already\nsupport it, so it makes sense to extend it with this. I also think its\nbetter to extend here than it is to hard-code it. We could provide a\nsingle short-option that does the longer variant if it's that common.\n\nThanks,\nJake\n"},{"id":"309887","messageId":"CACsJy8C41D0Qb0ZJoAPJ6Pwp904dVixUR9yXQmZr0NU9W-BU8A@mail.gmail.com","threadId":"44914","inReplyTo":"CA+P7+xqgyUW-Wt2oUSCc86HG-MfL-itAu2yVrZ219LNwqQnwKw@mail.gmail.com","subject":"Re: [PATCH] log: new option decorate reflog of remote refs","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-01-21T12:48:50Z","receivedAt":"2017-01-21T12:49:25Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Jan 21, 2017 at 5:00 AM, Jacob Keller wrote:\n> On Fri, Jan 20, 2017 at 6:30 AM, Jeff King wrote:\n>>> Imposing order between options could cause confusion, I think, if you\n>>> remove --decorate-reflog leaving --remotes on by accident, now you get\n>>> --remotes with a new meaning. We could go with something like\n>>> --decodate-reflog=remote, but that clashes with the number of reflog\n>>> entries and we may need a separator, like --decorate-reflog=remote,3.\n>>> Or we could add something to --decorate= in addition to\n>>> short|full|auto|no. Something like --decorate=full,reflog or\n>>> --decorate=full,reflog=remote,entries=3 if I want 3 reflog entries.\n>>\n>> I agree that making option-order important is potentially confusing. But\n>> it does already exist with --exclude. It's necessary to specify some\n>> sets of refs (e.g., all of A, except for those that match B, and then\n>> all of C, including those that match B).\n>>\n>> Having --decorate-reflog=remote would be similarly constrained. You\n>> couldn't do \"decorate all remotes except for these ones\". For that\n>> matter, I'm not sure how you would do \"decorate just the refs from\n>> origin\".\n>>\n>> I'll grant that those are going to be a lot less common than just \"all\n>> the remotes\" (or all the tags, or whatever). I'd just hate to see us\n>> revisiting this in a year to generalize it, and being stuck with\n>> historical baggage.\n>>\n>>> My hesitant to go that far is because I suspect decorating reflog\n>>> won't be helpful for non-remotes. But I'm willing to make more changes\n>>> if it opens door to master.\n>>\n>> Forgetting reflogs for a moment, I'd actually find it useful to just\n>> decorate tags and local branches, but not remotes. But right now there\n>> isn't any way to select which refs are worthy of decoration (reflog or\n>> not).\n>>\n>> That's why I'm thinking so much about a general ref-selection system. I\n>> agree the \"--exclude=... --remotes\" thing is complicated, but it's also\n>> the ref-selection system we _already_ have, which to me is a slight\n>> point in its favor.\n>>\n>> -Peff\n>\n> I agree that the interaction between --exclude and --remotes/etc is\n> confusing, but I think it's reasonable enough because we already\n> support it, so it makes sense to extend it with this. I also think its\n> better to extend here than it is to hard-code it.\n\nOK. Next question, how do we deal with the reflog count (i..e the\nargument of --decorate-remote-reflog). Should it be shared for all ref\ntype, or can be specified differently for remote, local and tags? I'm\nleaning towards the former. But I'll wait a bit for ideas before\nrewriting the patch.\n--\nDuy\n"},{"id":"309893","messageId":"20170121140806.tjs6wad3x4srdv3q@sigill.intra.peff.net","threadId":"44914","inReplyTo":"CACsJy8C41D0Qb0ZJoAPJ6Pwp904dVixUR9yXQmZr0NU9W-BU8A@mail.gmail.com","subject":"Re: [PATCH] log: new option decorate reflog of remote refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-21T14:08:06Z","receivedAt":"2017-01-21T14:08:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jan 21, 2017 at 07:48:50PM +0700, Duy Nguyen wrote:\n\n> OK. Next question, how do we deal with the reflog count (i..e the\n> argument of --decorate-remote-reflog). Should it be shared for all ref\n> type, or can be specified differently for remote, local and tags? I'm\n> leaning towards the former. But I'll wait a bit for ideas before\n> rewriting the patch.\n\nI doubt that anybody really cares about different reflog depths for\ndifferent refs. But I would say that the natural syntax ends up as:\n\n git log --decorate-reflog=10 --remotes \\\n --decorate-reflog=10 --tags\n\nanyway, so you get the ability to do it anyway \"for free\" (at the cost\nof having to repeat yourself).\n\nI guess the other option is:\n\n git log --decorate-reflog-depth=10 \\\n --decorate-reflog --remotes\n\t --decorate-reflog --tags\n\nThat's actually _more_ typing, and besides being less flexible just\nmuddles the \"is this option for the next ref-selector or not\" question.\n\n(The whole thing is obviously a lot of typing; I wonder if people would\nwant a config option to do this all the time).\n\n-Peff\n"},{"id":"310152","messageId":"20170125125054.7422-2-pclouds@gmail.com","threadId":"44914","inReplyTo":"20170125125054.7422-1-pclouds@gmail.com","subject":"[PATCH 1/5] rev-list-options.txt: delete an empty line","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-01-25T12:50:50Z","receivedAt":"2017-01-25T12:51:44Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"The convention has been option name is followed immediately by its\ndescription without a line break.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n Documentation/rev-list-options.txt | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex 5da7cf5a8d..1f948cfb13 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -161,7 +161,6 @@ parents) and `--max-parents=-1` (negative numbers denote no upper limit).\n \tor '[', '/{asterisk}' at the end is implied.\n \n --exclude=::\n-\n \tDo not include refs matching '' that the next `--all`,\n \t`--branches`, `--tags`, `--remotes`, or `--glob` would otherwise\n \tconsider. Repetitions of this option accumulate exclusion patterns\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"310153","messageId":"20170125125054.7422-1-pclouds@gmail.com","threadId":"44914","inReplyTo":"20170121140806.tjs6wad3x4srdv3q@sigill.intra.peff.net","subject":"[PATCH 0/5] Prep steps for --decorate-reflog","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-01-25T12:50:49Z","receivedAt":"2017-01-25T12:51:48Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"I'm still half way through implementing --decorate-reflog that can\nselect what refs to decorate using --branches, --remotes, --tags...\nBut I want to make sure I'm heading the right direction first since\nI'm not really sure if this is the right way (implementation wise).\n\nThis series does not really implement --decorate-reflog. It shuffles\nrevision.c code around a bit so thay the option can be implemented\nlater. The most controversal patch would be 4/5 where --exclude\nbehavior is changed slighly.\n\nGood? Bad? Horror hooorrrible?\n\nNguyễn Thái Ngọc Duy (5):\n rev-list-options.txt: delete an empty line\n revision.c: group ref selection options together\n revision.c: allow to change pseudo opt parsing function\n revision.c: refactor ref selection handler after --exclude\n revision.c: add --decorate-reflog\n\n Documentation/rev-list-options.txt | 1 -\n revision.c | 206 ++++++++++++++++++++++++++++++-------\n revision.h | 4 +\n 3 files changed, 173 insertions(+), 38 deletions(-)\n\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"310154","messageId":"20170125125054.7422-3-pclouds@gmail.com","threadId":"44914","inReplyTo":"20170125125054.7422-1-pclouds@gmail.com","subject":"[PATCH 2/5] revision.c: group ref selection options together","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-01-25T12:50:51Z","receivedAt":"2017-01-25T12:51:49Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"These options have on thing in common: when specified right after\n--exclude, they will de-select refs instead of selecting them by\ndefault.\n\nThis change makes it possible to introduce new options that use these\noptions in the same way as --exclude. Such an option would just\nimplement something like handle_refs_pseudo_opt().\n\nparse_ref_selector_option() is taken out of handle_refs_pseudo_opt() so\nthat similar functions like handle_refs_pseudo_opt() are forced to\nhandle all ref selector options, not skipping some by mistake, which may\nrevert the option back to default behavior (rev selection).\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n revision.c | 134 +++++++++++++++++++++++++++++++++++++++++++++----------------\n 1 file changed, 100 insertions(+), 34 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex b37dbec378..6ebd38d1c8 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2059,6 +2059,103 @@ static int for_each_good_bisect_ref(const char *submodule, each_ref_fn fn, void\n \treturn for_each_bisect_ref(submodule, fn, cb_data, term_good);\n }\n \n+enum ref_selector {\n+\tREF_SELECT_NONE,\n+\tREF_SELECT_ALL,\n+\tREF_SELECT_BRANCHES,\n+\tREF_SELECT_TAGS,\n+\tREF_SELECT_REMOTES,\n+\tREF_SELECT_BY_GLOB\n+};\n+\n+static enum ref_selector parse_ref_selector_option(int argc, const char **argv,\n+\t\t\t\t\t\t const char **optarg,\n+\t\t\t\t\t\t int *argcount)\n+{\n+\tconst char *arg = argv[0];\n+\n+\t*optarg = NULL;\n+\n+\tif (!strcmp(arg, \"--all\"))\n+\t\treturn REF_SELECT_ALL;\n+\telse if (!strcmp(arg, \"--branches\") ||\n+\t\t skip_prefix(arg, \"--branches=\", optarg))\n+\t\treturn REF_SELECT_BRANCHES;\n+\telse if (!strcmp(arg, \"--tags\") ||\n+\t\t skip_prefix(arg, \"--tags=\", optarg))\n+\t\treturn REF_SELECT_TAGS;\n+\telse if (!strcmp(arg, \"--remotes\") ||\n+\t\t skip_prefix(arg, \"--remotes=\", optarg))\n+\t\treturn REF_SELECT_REMOTES;\n+\telse if ((*argcount = parse_long_opt(\"glob\", argv, optarg)))\n+\t\treturn REF_SELECT_BY_GLOB;\n+\n+\treturn REF_SELECT_NONE;\n+}\n+\n+static int handle_refs_pseudo_opt(const char *submodule,\n+\t\t\t\t struct rev_info *revs,\n+\t\t\t\t int argc, const char **argv,\n+\t\t\t\t int *flags, int *ret)\n+{\n+\tstruct all_refs_cb cb;\n+\tconst char *optarg = NULL;\n+\tint argcount;\n+\tenum ref_selector selector;\n+\n+\tselector = parse_ref_selector_option(argc, argv, &optarg, &argcount);\n+\n+\tif (optarg)\n+\t\tinit_all_refs_cb(&cb, revs, *flags);\n+\t*ret = 1;\n+\n+\tswitch (selector) {\n+\tcase REF_SELECT_ALL:\n+\t\thandle_refs(submodule, revs, *flags, for_each_ref_submodule);\n+\t\thandle_refs(submodule, revs, *flags, head_ref_submodule);\n+\t\tbreak;\n+\n+\tcase REF_SELECT_BRANCHES:\n+\t\tif (optarg)\n+\t\t\tfor_each_glob_ref_in(handle_one_ref, optarg,\n+\t\t\t\t\t \"refs/heads/\", &cb);\n+\t\telse\n+\t\t\thandle_refs(submodule, revs, *flags,\n+\t\t\t\t for_each_branch_ref_submodule);\n+\t\tbreak;\n+\n+\tcase REF_SELECT_TAGS:\n+\t\tif (optarg)\n+\t\t\tfor_each_glob_ref_in(handle_one_ref, optarg,\n+\t\t\t\t\t \"refs/tags/\", &cb);\n+\t\telse\n+\t\t\thandle_refs(submodule, revs, *flags,\n+\t\t\t\t for_each_tag_ref_submodule);\n+\t\tbreak;\n+\n+\tcase REF_SELECT_REMOTES:\n+\t\tif (optarg)\n+\t\t\tfor_each_glob_ref_in(handle_one_ref, optarg,\n+\t\t\t\t\t \"refs/remotes/\", &cb);\n+\t\telse\n+\t\t\thandle_refs(submodule, revs, *flags,\n+\t\t\t\t for_each_remote_ref_submodule);\n+\t\tbreak;\n+\n+\tcase REF_SELECT_BY_GLOB:\n+\t\tinit_all_refs_cb(&cb, revs, *flags);\n+\t\tfor_each_glob_ref(handle_one_ref, optarg, &cb);\n+\t\t*ret = argcount;\n+\t\tbreak;\n+\n+\tcase REF_SELECT_NONE:\n+\t\treturn 0;\n+\t}\n+\n+\tclear_ref_exclusion(&revs->ref_excludes);\n+\treturn 1;\n+}\n+\n static int handle_revision_pseudo_opt(const char *submodule,\n \t\t\t\tstruct rev_info *revs,\n \t\t\t\tint argc, const char **argv, int *flags)\n@@ -2066,6 +2163,7 @@ static int handle_revision_pseudo_opt(const char *submodule,\n \tconst char *arg = argv[0];\n \tconst char *optarg;\n \tint argcount;\n+\tint ret;\n \n \t/*\n \t * NOTE!\n@@ -2077,48 +2175,16 @@ static int handle_revision_pseudo_opt(const char *submodule,\n \t * When implementing your new pseudo-option, remember to\n \t * register it in the list at the top of handle_revision_opt.\n \t */\n-\tif (!strcmp(arg, \"--all\")) {\n-\t\thandle_refs(submodule, revs, *flags, for_each_ref_submodule);\n-\t\thandle_refs(submodule, revs, *flags, head_ref_submodule);\n-\t\tclear_ref_exclusion(&revs->ref_excludes);\n-\t} else if (!strcmp(arg, \"--branches\")) {\n-\t\thandle_refs(submodule, revs, *flags, for_each_branch_ref_submodule);\n-\t\tclear_ref_exclusion(&revs->ref_excludes);\n+\tif (handle_refs_pseudo_opt(submodule, revs, argc, argv, flags, &ret)) {\n+\t\treturn ret;\n \t} else if (!strcmp(arg, \"--bisect\")) {\n \t\tread_bisect_terms(&term_bad, &term_good);\n \t\thandle_refs(submodule, revs, *flags, for_each_bad_bisect_ref);\n \t\thandle_refs(submodule, revs, *flags ^ (UNINTERESTING | BOTTOM), for_each_good_bisect_ref);\n \t\trevs->bisect = 1;\n-\t} else if (!strcmp(arg, \"--tags\")) {\n-\t\thandle_refs(submodule, revs, *flags, for_each_tag_ref_submodule);\n-\t\tclear_ref_exclusion(&revs->ref_excludes);\n-\t} else if (!strcmp(arg, \"--remotes\")) {\n-\t\thandle_refs(submodule, revs, *flags, for_each_remote_ref_submodule);\n-\t\tclear_ref_exclusion(&revs->ref_excludes);\n-\t} else if ((argcount = parse_long_opt(\"glob\", argv, &optarg))) {\n-\t\tstruct all_refs_cb cb;\n-\t\tinit_all_refs_cb(&cb, revs, *flags);\n-\t\tfor_each_glob_ref(handle_one_ref, optarg, &cb);\n-\t\tclear_ref_exclusion(&revs->ref_excludes);\n-\t\treturn argcount;\n \t} else if ((argcount = parse_long_opt(\"exclude\", argv, &optarg))) {\n \t\tadd_ref_exclusion(&revs->ref_excludes, optarg);\n \t\treturn argcount;\n-\t} else if (starts_with(arg, \"--branches=\")) {\n-\t\tstruct all_refs_cb cb;\n-\t\tinit_all_refs_cb(&cb, revs, *flags);\n-\t\tfor_each_glob_ref_in(handle_one_ref, arg + 11, \"refs/heads/\", &cb);\n-\t\tclear_ref_exclusion(&revs->ref_excludes);\n-\t} else if (starts_with(arg, \"--tags=\")) {\n-\t\tstruct all_refs_cb cb;\n-\t\tinit_all_refs_cb(&cb, revs, *flags);\n-\t\tfor_each_glob_ref_in(handle_one_ref, arg + 7, \"refs/tags/\", &cb);\n-\t\tclear_ref_exclusion(&revs->ref_excludes);\n-\t} else if (starts_with(arg, \"--remotes=\")) {\n-\t\tstruct all_refs_cb cb;\n-\t\tinit_all_refs_cb(&cb, revs, *flags);\n-\t\tfor_each_glob_ref_in(handle_one_ref, arg + 10, \"refs/remotes/\", &cb);\n-\t\tclear_ref_exclusion(&revs->ref_excludes);\n \t} else if (!strcmp(arg, \"--reflog\")) {\n \t\tadd_reflogs_to_pending(revs, *flags);\n \t} else if (!strcmp(arg, \"--indexed-objects\")) {\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"310155","messageId":"20170125125054.7422-4-pclouds@gmail.com","threadId":"44914","inReplyTo":"20170125125054.7422-1-pclouds@gmail.com","subject":"[PATCH 3/5] revision.c: allow to change pseudo opt parsing function","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-01-25T12:50:52Z","receivedAt":"2017-01-25T12:51:53Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Signed-off-by: Nguyễn Thái Ngọc Duy \n---\n revision.c | 11 ++++++++---\n revision.h | 4 ++++\n 2 files changed, 12 insertions(+), 3 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 6ebd38d1c8..cda2606c66 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2273,10 +2273,15 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\tconst char *arg = argv[i];\n \t\tif (*arg == '-') {\n \t\t\tint opts;\n+\t\t\thandle_pseudo_opt_cb handle_pseudo_opt =\n+\t\t\t\thandle_revision_pseudo_opt;\n \n-\t\t\topts = handle_revision_pseudo_opt(submodule,\n-\t\t\t\t\t\trevs, argc - i, argv + i,\n-\t\t\t\t\t\t&flags);\n+\t\t\tif (revs->handle_pseudo_opt)\n+\t\t\t\thandle_pseudo_opt = revs->handle_pseudo_opt;\n+\n+\t\t\topts = handle_pseudo_opt(submodule,\n+\t\t\t\t\t\t revs, argc - i, argv + i,\n+\t\t\t\t\t\t &flags);\n \t\t\tif (opts > 0) {\n \t\t\t\ti += opts - 1;\n \t\t\t\tcontinue;\ndiff --git a/revision.h b/revision.h\nindex 9fac1a607d..9080eaf381 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -52,6 +52,8 @@ struct rev_cmdline_info {\n #define REVISION_WALK_NO_WALK_SORTED 1\n #define REVISION_WALK_NO_WALK_UNSORTED 2\n \n+typedef int (*handle_pseudo_opt_cb)(const char *, struct rev_info *, int, const char **, int *);\n+\n struct rev_info {\n \t/* Starting list */\n \tstruct commit_list *commits;\n@@ -213,6 +215,8 @@ struct rev_info {\n \n \tstruct commit_list *previous_parents;\n \tconst char *break_bar;\n+\n+\thandle_pseudo_opt_cb handle_pseudo_opt;\n };\n \n extern int ref_excluded(struct string_list *, const char *path);\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"310156","messageId":"20170125125054.7422-6-pclouds@gmail.com","threadId":"44914","inReplyTo":"20170125125054.7422-1-pclouds@gmail.com","subject":"[PATCH 5/5] revision.c: add --decorate-reflog","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-01-25T12:50:54Z","receivedAt":"2017-01-25T12:51:54Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Signed-off-by: Nguyễn Thái Ngọc Duy \n---\n revision.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 46 insertions(+)\n\ndiff --git a/revision.c b/revision.c\nindex 45cffcab44..b77face513 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2157,6 +2157,49 @@ static int handle_refs_pseudo_opt(const char *submodule,\n \n static int handle_revision_pseudo_opt(const char *, struct rev_info *, int, const char **, int *);\n \n+static int handle_revision_pseudo_opt_after_decorate_reflog(\n+\tconst char *submodule, struct rev_info *revs,\n+\tint argc, const char **argv, int *flags)\n+{\n+\tstruct all_refs_cb cb;\n+\tconst char *optarg = NULL;\n+\tint argcount;\n+\tenum ref_selector selector;\n+\n+\tselector = parse_ref_selector_option(argc, argv, &optarg, &argcount);\n+\n+\tif (optarg)\n+\t\tinit_all_refs_cb(&cb, revs, *flags);\n+\n+\tswitch (selector) {\n+\tcase REF_SELECT_ALL:\n+\t\t/* keep the info for load_ref_decorations() later */\n+\t\treturn 1;\n+\n+\tcase REF_SELECT_BRANCHES:\n+\t\t/* keep the info for load_ref_decorations() later */\n+\t\treturn 1;\n+\n+\tcase REF_SELECT_TAGS:\n+\t\t/* keep the info for load_ref_decorations() later */\n+\t\treturn 1;\n+\n+\tcase REF_SELECT_REMOTES:\n+\t\t/* keep the info for load_ref_decorations() later */\n+\t\treturn 1;\n+\n+\tcase REF_SELECT_BY_GLOB:\n+\t\t/* keep the info for load_ref_decorations() later */\n+\t\treturn 1;\n+\n+\tcase REF_SELECT_NONE:\n+\t\tbreak;\n+\t}\n+\n+\trevs->handle_pseudo_opt = NULL;\n+\treturn handle_revision_pseudo_opt(submodule, revs, argc, argv, flags);\n+}\n+\n static int handle_revision_pseudo_opt_after_exclude(const char *submodule,\n \t\t\t\t\t\t struct rev_info *revs,\n \t\t\t\t\t\t int argc, const char **argv,\n@@ -2200,6 +2243,9 @@ static int handle_revision_pseudo_opt(const char *submodule,\n \t\tadd_ref_exclusion(&revs->ref_excludes, optarg);\n \t\trevs->handle_pseudo_opt = handle_revision_pseudo_opt_after_exclude;\n \t\treturn argcount;\n+\t} else if ((argcount = parse_long_opt(\"decorate-reflog\", argv, &optarg))) {\n+\t\trevs->handle_pseudo_opt = handle_revision_pseudo_opt_after_decorate_reflog;\n+\t\treturn argcount;\n \t} else if (!strcmp(arg, \"--reflog\")) {\n \t\tadd_reflogs_to_pending(revs, *flags);\n \t} else if (!strcmp(arg, \"--indexed-objects\")) {\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"310157","messageId":"20170125125054.7422-5-pclouds@gmail.com","threadId":"44914","inReplyTo":"20170125125054.7422-1-pclouds@gmail.com","subject":"[PATCH 4/5] revision.c: refactor ref selection handler after --exclude","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-01-25T12:50:53Z","receivedAt":"2017-01-25T12:51:59Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Behavior change: \"--exclude --blah --remotes\" will not exclude remote\nbranches any more. Only \"--exclude --remotes\" does.\n\nThis is because --exclude is going to have a new friend --decorate-reflog\nwho haves the same way. When you allow a distant --remotes to complement\na previous option, things get complicated. In\n\n --exclude .. --decorate-reflog ... --remotes\n\nDoes it mean decorate remote reflogs, or does it mean exclude remotes\nfrom the selected revisions?\n\nGranted, there may be valid use cases for such a combination (e.g.\n\"decorate all reflogs except remote ones\") but I feel option order is\nnot a good fit to express them.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n revision.c | 17 ++++++++++++++++-\n 1 file changed, 16 insertions(+), 1 deletion(-)\n\ndiff --git a/revision.c b/revision.c\nindex cda2606c66..45cffcab44 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2152,10 +2152,24 @@ static int handle_refs_pseudo_opt(const char *submodule,\n \t\treturn 0;\n \t}\n \n-\tclear_ref_exclusion(&revs->ref_excludes);\n \treturn 1;\n }\n \n+static int handle_revision_pseudo_opt(const char *, struct rev_info *, int, const char **, int *);\n+\n+static int handle_revision_pseudo_opt_after_exclude(const char *submodule,\n+\t\t\t\t\t\t struct rev_info *revs,\n+\t\t\t\t\t\t int argc, const char **argv,\n+\t\t\t\t\t\t int *flags)\n+{\n+\tint ret;\n+\n+\tret = handle_revision_pseudo_opt(submodule, revs, argc, argv, flags);\n+\tclear_ref_exclusion(&revs->ref_excludes);\n+\trevs->handle_pseudo_opt = NULL;\n+\treturn ret;\n+}\n+\n static int handle_revision_pseudo_opt(const char *submodule,\n \t\t\t\tstruct rev_info *revs,\n \t\t\t\tint argc, const char **argv, int *flags)\n@@ -2184,6 +2198,7 @@ static int handle_revision_pseudo_opt(const char *submodule,\n \t\trevs->bisect = 1;\n \t} else if ((argcount = parse_long_opt(\"exclude\", argv, &optarg))) {\n \t\tadd_ref_exclusion(&revs->ref_excludes, optarg);\n+\t\trevs->handle_pseudo_opt = handle_revision_pseudo_opt_after_exclude;\n \t\treturn argcount;\n \t} else if (!strcmp(arg, \"--reflog\")) {\n \t\tadd_reflogs_to_pending(revs, *flags);\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"310168","messageId":"CA+P7+xoL28hTdjbPfau_cgui3j73pMJqOBsM+5-kKSA369yJ1Q@mail.gmail.com","threadId":"44914","inReplyTo":"20170125125054.7422-5-pclouds@gmail.com","subject":"Re: [PATCH 4/5] revision.c: refactor ref selection handler after --exclude","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2017-01-25T17:41:46Z","receivedAt":"2017-01-25T17:42:13Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Wed, Jan 25, 2017 at 4:50 AM, Nguyễn Thái Ngọc Duy wrote:\n> Behavior change: \"--exclude --blah --remotes\" will not exclude remote\n> branches any more. Only \"--exclude --remotes\" does.\n>\n> This is because --exclude is going to have a new friend --decorate-reflog\n> who haves the same way. When you allow a distant --remotes to complement\n> a previous option, things get complicated. In\n>\n> --exclude .. --decorate-reflog ... --remotes\n>\n> Does it mean decorate remote reflogs, or does it mean exclude remotes\n> from the selected revisions?\n>\n> Granted, there may be valid use cases for such a combination (e.g.\n> \"decorate all reflogs except remote ones\") but I feel option order is\n> not a good fit to express them.\n>\n\nLimiting the scope of the exclude seems somewhat reasonable to me,\nbecause it makes it much easier to explain and show the user. We do\nneed to make sure it's not going to break any scripts or other issues.\nIs it possible for us to produce an error if the user does \"--exclude\"\nwithout a necessary connecting option?\n\nThanks,\nJake\n"},{"id":"310199","messageId":"20170125205037.cg3aebh5rsx4zb2l@sigill.intra.peff.net","threadId":"44914","inReplyTo":"20170125125054.7422-3-pclouds@gmail.com","subject":"Re: [PATCH 2/5] revision.c: group ref selection options together","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-25T20:50:38Z","receivedAt":"2017-01-25T20:50:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 25, 2017 at 07:50:51PM +0700, Nguyễn Thái Ngọc Duy wrote:\n\n> These options have on thing in common: when specified right after\n> --exclude, they will de-select refs instead of selecting them by\n> default.\n> \n> This change makes it possible to introduce new options that use these\n> options in the same way as --exclude. Such an option would just\n> implement something like handle_refs_pseudo_opt().\n> \n> parse_ref_selector_option() is taken out of handle_refs_pseudo_opt() so\n> that similar functions like handle_refs_pseudo_opt() are forced to\n> handle all ref selector options, not skipping some by mistake, which may\n> revert the option back to default behavior (rev selection).\n> \n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> revision.c | 134 +++++++++++++++++++++++++++++++++++++++++++++----------------\n> 1 file changed, 100 insertions(+), 34 deletions(-)\n\nHmm. I see what you're trying to do here, and abstract the repeated\nbits. But I'm not sure the line-count reflects a real simplification.\nEverything ends up converted to an enum, and then that enum just expands\nto similar C code.\n\nI kind of expected that clear_ref_exclusion() would just become a more\nabstract clear_ref_selection(). For now it would clear exclusions, and\nthen later learn to clear the decoration flags.\n\nMaybe I am missing something in the later patches, though.\n\n-Peff\n"},{"id":"310201","messageId":"20170125205718.ksqstdnazmgbkehy@sigill.intra.peff.net","threadId":"44914","inReplyTo":"20170125125054.7422-5-pclouds@gmail.com","subject":"Re: [PATCH 4/5] revision.c: refactor ref selection handler after --exclude","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-25T20:57:18Z","receivedAt":"2017-01-25T20:57:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 25, 2017 at 07:50:53PM +0700, Nguyễn Thái Ngọc Duy wrote:\n\n> Behavior change: \"--exclude --blah --remotes\" will not exclude remote\n> branches any more. Only \"--exclude --remotes\" does.\n> \n> This is because --exclude is going to have a new friend --decorate-reflog\n> who haves the same way. When you allow a distant --remotes to complement\n> a previous option, things get complicated. In\n> \n> --exclude .. --decorate-reflog ... --remotes\n> \n> Does it mean decorate remote reflogs, or does it mean exclude remotes\n> from the selected revisions?\n\nI don't think it means either. It means to include remotes in the\nselected revisions, but excluding the entries mentioned by --exclude.\n\nIOW:\n\n --exclude=foo --remotes\n\tinclude all remotes except refs/remotes/foo\n\n --exclude=foo --unrelated --remotes\n same\n\n --exclude=foo --decorate-reflog --remotes\n decorate reflogs of all remotes except \"foo\". Do _not_ use them\n\tas traversal tips.\n\n --decorate-reflog --exclude=foo --remotes\n same\n\nIOW, the ref-selector options build up until a group option is given,\nwhich acts on the built-up options (over that group) and then resets the\nbuilt-up options. Doing \"--unrelated\" as above is orthogonal (though I\nthink in practice nobody would do that, because it's hard to read).\n\n> Granted, there may be valid use cases for such a combination (e.g.\n> \"decorate all reflogs except remote ones\") but I feel option order is\n> not a good fit to express them.\n\nThat would be spelled:\n\n --exclude=refs/remotes --decorate-reflogs --all\n\n(or you could swap the first two options).\n\nAgain, I'm not sure if I'm missing something subtle, or if you are\nconfused about how --exclude works. :)\n\n-Peff\n"},{"id":"310203","messageId":"xmqqo9yualqn.fsf@gitster.mtv.corp.google.com","threadId":"44914","inReplyTo":"20170125125054.7422-3-pclouds@gmail.com","subject":"Re: [PATCH 2/5] revision.c: group ref selection options together","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-25T21:11:12Z","receivedAt":"2017-01-25T21:11:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> These options have on thing in common: when specified right after\n\none thing.\n\n> --exclude, they will de-select refs instead of selecting them by\n> default.\n>\n> This change makes it possible to introduce new options that use these\n> options in the same way as --exclude. Such an option would just\n> implement something like handle_refs_pseudo_opt().\n>\n> parse_ref_selector_option() is taken out of handle_refs_pseudo_opt() so\n> that similar functions like handle_refs_pseudo_opt() are forced to\n> handle all ref selector options, not skipping some by mistake, which may\n> revert the option back to default behavior (rev selection).\n\nI am not sure about these two refactorings, for at least two reasons.\n\n * Naming. The function is all about handling \"refs options\" and I\n do not see anything \"pseudo\" about the options handled by the\n handle_refs_pseudo_opt() function. This is minor.\n\n * I am expecting that the new one yet to be introduced will not\n share the huge \"switch (selector)\" part, but does its own things\n in a separate function with a similar structure. The only thing\n common between these two functions would be the structure\n (i.e. it has a big \"switch(selector)\" that does different things\n depending on REF_SELECT_*) and a call to clear_* function.\n\n If we were to add a new kind of REF_SELECT_* (say\n REF_SELECT_NOTES just for the sake of being concrete), what\n changes will be needed to the code if the addition of \"use reflog\n from this class of refs for decoration\" feature was done with or\n without this step? I have a suspicion that the change will be\n simpler without this step.\n\n The above comments will be invalid if the new \"use reflog for\n decoration\" feature will be done by extending this pseudo_opt()\n function by extending each of the switch/case arms. If that is\n the case, please disregard the above. I just didn't get that\n impression from the above paragraph.\n\n"},{"id":"310206","messageId":"xmqqk29ialip.fsf@gitster.mtv.corp.google.com","threadId":"44914","inReplyTo":"20170125125054.7422-5-pclouds@gmail.com","subject":"Re: [PATCH 4/5] revision.c: refactor ref selection handler after --exclude","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-25T21:15:58Z","receivedAt":"2017-01-25T21:16:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> Behavior change: \"--exclude --blah --remotes\" will not exclude remote\n> branches any more. Only \"--exclude --remotes\" does.\n>\n> This is because --exclude is going to have a new friend --decorate-reflog\n> who haves the same way. When you allow a distant --remotes to complement\n> a previous option, things get complicated. In\n>\n> --exclude .. --decorate-reflog ... --remotes\n>\n> Does it mean decorate remote reflogs, or does it mean exclude remotes\n> from the selected revisions?\n\nI would expect that the effect of exclude, decorate-reflog and\nfriends will extend until the next occurrence of --remotes (or --all\nor whatever you catch in parse_ref_selector_option() function).\n\nSo, I'd read it as \"add all remote tracking refs, but (1) exclude\nexclude the refs matching pattern .. and (2) use reflog of them if\nthey match pattern ...\".\n\nOr did you mean by \"...\" something other than a single argument that\nis a pattern?\n\n"},{"id":"310210","messageId":"20170125212721.7tbxkqsdtsv2n5mx@sigill.intra.peff.net","threadId":"44914","inReplyTo":"20170125205718.ksqstdnazmgbkehy@sigill.intra.peff.net","subject":"Re: [PATCH 4/5] revision.c: refactor ref selection handler after --exclude","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-25T21:27:21Z","receivedAt":"2017-01-25T21:27:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 25, 2017 at 03:57:18PM -0500, Jeff King wrote:\n\n> IOW, the ref-selector options build up until a group option is given,\n> which acts on the built-up options (over that group) and then resets the\n> built-up options. Doing \"--unrelated\" as above is orthogonal (though I\n> think in practice nobody would do that, because it's hard to read).\n\nSo here's what I would have expected your series to look more like (with\nprobably one patch adding clear_ref_selection_options, and the other\nadding the decorate stuff):\n\ndiff --git a/revision.c b/revision.c\nindex b37dbec37..2f67707c7 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1156,6 +1156,11 @@ static int handle_one_ref(const char *path, const struct object_id *oid,\n \n \tif (ref_excluded(cb->all_revs->ref_excludes, path))\n \t return 0;\n+\tif (cb->all_revs->decorate_reflog) {\n+\t\t/* TODO actually do it for real */\n+\t\twarning(\"would decorate %s\", path);\n+\t\treturn 0; /* do not add it as a tip */\n+\t}\n \n \tobject = get_reference(cb->all_revs, path, oid->hash, cb->all_flags);\n \tadd_rev_cmdline(cb->all_revs, object, path, REV_CMD_REF, cb->all_flags);\n@@ -1188,6 +1193,12 @@ void add_ref_exclusion(struct string_list **ref_excludes_p, const char *exclude)\n \tstring_list_append(*ref_excludes_p, exclude);\n }\n \n+static void clear_ref_selection_options(struct rev_info *revs)\n+{\n+\tclear_ref_exclusion(&revs->ref_excludes);\n+\trevs->decorate_reflog = 0;\n+}\n+\n static void handle_refs(const char *submodule, struct rev_info *revs, unsigned flags,\n \t\tint (*for_each)(const char *, each_ref_fn, void *))\n {\n@@ -2080,10 +2091,10 @@ static int handle_revision_pseudo_opt(const char *submodule,\n \tif (!strcmp(arg, \"--all\")) {\n \t\thandle_refs(submodule, revs, *flags, for_each_ref_submodule);\n \t\thandle_refs(submodule, revs, *flags, head_ref_submodule);\n-\t\tclear_ref_exclusion(&revs->ref_excludes);\n+\t\tclear_ref_selection_options(revs);\n \t} else if (!strcmp(arg, \"--branches\")) {\n \t\thandle_refs(submodule, revs, *flags, for_each_branch_ref_submodule);\n-\t\tclear_ref_exclusion(&revs->ref_excludes);\n+\t\tclear_ref_selection_options(revs);\n \t} else if (!strcmp(arg, \"--bisect\")) {\n \t\tread_bisect_terms(&term_bad, &term_good);\n \t\thandle_refs(submodule, revs, *flags, for_each_bad_bisect_ref);\n@@ -2091,15 +2102,15 @@ static int handle_revision_pseudo_opt(const char *submodule,\n \t\trevs->bisect = 1;\n \t} else if (!strcmp(arg, \"--tags\")) {\n \t\thandle_refs(submodule, revs, *flags, for_each_tag_ref_submodule);\n-\t\tclear_ref_exclusion(&revs->ref_excludes);\n+\t\tclear_ref_selection_options(revs);\n \t} else if (!strcmp(arg, \"--remotes\")) {\n \t\thandle_refs(submodule, revs, *flags, for_each_remote_ref_submodule);\n-\t\tclear_ref_exclusion(&revs->ref_excludes);\n+\t\tclear_ref_selection_options(revs);\n \t} else if ((argcount = parse_long_opt(\"glob\", argv, &optarg))) {\n \t\tstruct all_refs_cb cb;\n \t\tinit_all_refs_cb(&cb, revs, *flags);\n \t\tfor_each_glob_ref(handle_one_ref, optarg, &cb);\n-\t\tclear_ref_exclusion(&revs->ref_excludes);\n+\t\tclear_ref_selection_options(revs);\n \t\treturn argcount;\n \t} else if ((argcount = parse_long_opt(\"exclude\", argv, &optarg))) {\n \t\tadd_ref_exclusion(&revs->ref_excludes, optarg);\n@@ -2108,17 +2119,19 @@ static int handle_revision_pseudo_opt(const char *submodule,\n \t\tstruct all_refs_cb cb;\n \t\tinit_all_refs_cb(&cb, revs, *flags);\n \t\tfor_each_glob_ref_in(handle_one_ref, arg + 11, \"refs/heads/\", &cb);\n-\t\tclear_ref_exclusion(&revs->ref_excludes);\n+\t\tclear_ref_selection_options(revs);\n \t} else if (starts_with(arg, \"--tags=\")) {\n \t\tstruct all_refs_cb cb;\n \t\tinit_all_refs_cb(&cb, revs, *flags);\n \t\tfor_each_glob_ref_in(handle_one_ref, arg + 7, \"refs/tags/\", &cb);\n-\t\tclear_ref_exclusion(&revs->ref_excludes);\n+\t\tclear_ref_selection_options(revs);\n \t} else if (starts_with(arg, \"--remotes=\")) {\n \t\tstruct all_refs_cb cb;\n \t\tinit_all_refs_cb(&cb, revs, *flags);\n \t\tfor_each_glob_ref_in(handle_one_ref, arg + 10, \"refs/remotes/\", &cb);\n-\t\tclear_ref_exclusion(&revs->ref_excludes);\n+\t\tclear_ref_selection_options(revs);\n+\t} else if (!strcmp(arg, \"--decorate-reflog\")) {\n+\t\trevs->decorate_reflog = 1;\n \t} else if (!strcmp(arg, \"--reflog\")) {\n \t\tadd_reflogs_to_pending(revs, *flags);\n \t} else if (!strcmp(arg, \"--indexed-objects\")) {\ndiff --git a/revision.h b/revision.h\nindex 9fac1a607..c74879829 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -66,6 +66,8 @@ struct rev_info {\n \t/* excluding from --branches, --refs, etc. expansion */\n \tstruct string_list *ref_excludes;\n \n+\tint decorate_reflog;\n+\n \t/* Basic information */\n \tconst char *prefix;\n \tconst char *def;\n"},{"id":"310212","messageId":"CA+P7+xovOx9ebo6MU0e4v==+76jtoMXz45+LnBPFifHbjqFU4w@mail.gmail.com","threadId":"44914","inReplyTo":"20170125212721.7tbxkqsdtsv2n5mx@sigill.intra.peff.net","subject":"Re: [PATCH 4/5] revision.c: refactor ref selection handler after --exclude","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2017-01-25T21:30:20Z","receivedAt":"2017-01-25T21:31:02Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Wed, Jan 25, 2017 at 1:27 PM, Jeff King wrote:\n> On Wed, Jan 25, 2017 at 03:57:18PM -0500, Jeff King wrote:\n>\n>> IOW, the ref-selector options build up until a group option is given,\n>> which acts on the built-up options (over that group) and then resets the\n>> built-up options. Doing \"--unrelated\" as above is orthogonal (though I\n>> think in practice nobody would do that, because it's hard to read).\n>\n> So here's what I would have expected your series to look more like (with\n> probably one patch adding clear_ref_selection_options, and the other\n> adding the decorate stuff):\n>\n\nI agree that this is how I would have expected it to work as well.\n\nThanks,\nJake\n\n> diff --git a/revision.c b/revision.c\n> index b37dbec37..2f67707c7 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -1156,6 +1156,11 @@ static int handle_one_ref(const char *path, const struct object_id *oid,\n>\n> if (ref_excluded(cb->all_revs->ref_excludes, path))\n> return 0;\n> + if (cb->all_revs->decorate_reflog) {\n> + /* TODO actually do it for real */\n> + warning(\"would decorate %s\", path);\n> + return 0; /* do not add it as a tip */\n> + }\n>\n> object = get_reference(cb->all_revs, path, oid->hash, cb->all_flags);\n> add_rev_cmdline(cb->all_revs, object, path, REV_CMD_REF, cb->all_flags);\n> @@ -1188,6 +1193,12 @@ void add_ref_exclusion(struct string_list **ref_excludes_p, const char *exclude)\n> string_list_append(*ref_excludes_p, exclude);\n> }\n>\n> +static void clear_ref_selection_options(struct rev_info *revs)\n> +{\n> + clear_ref_exclusion(&revs->ref_excludes);\n> + revs->decorate_reflog = 0;\n> +}\n> +\n> static void handle_refs(const char *submodule, struct rev_info *revs, unsigned flags,\n> int (*for_each)(const char *, each_ref_fn, void *))\n> {\n> @@ -2080,10 +2091,10 @@ static int handle_revision_pseudo_opt(const char *submodule,\n> if (!strcmp(arg, \"--all\")) {\n> handle_refs(submodule, revs, *flags, for_each_ref_submodule);\n> handle_refs(submodule, revs, *flags, head_ref_submodule);\n> - clear_ref_exclusion(&revs->ref_excludes);\n> + clear_ref_selection_options(revs);\n> } else if (!strcmp(arg, \"--branches\")) {\n> handle_refs(submodule, revs, *flags, for_each_branch_ref_submodule);\n> - clear_ref_exclusion(&revs->ref_excludes);\n> + clear_ref_selection_options(revs);\n> } else if (!strcmp(arg, \"--bisect\")) {\n> read_bisect_terms(&term_bad, &term_good);\n> handle_refs(submodule, revs, *flags, for_each_bad_bisect_ref);\n> @@ -2091,15 +2102,15 @@ static int handle_revision_pseudo_opt(const char *submodule,\n> revs->bisect = 1;\n> } else if (!strcmp(arg, \"--tags\")) {\n> handle_refs(submodule, revs, *flags, for_each_tag_ref_submodule);\n> - clear_ref_exclusion(&revs->ref_excludes);\n> + clear_ref_selection_options(revs);\n> } else if (!strcmp(arg, \"--remotes\")) {\n> handle_refs(submodule, revs, *flags, for_each_remote_ref_submodule);\n> - clear_ref_exclusion(&revs->ref_excludes);\n> + clear_ref_selection_options(revs);\n> } else if ((argcount = parse_long_opt(\"glob\", argv, &optarg))) {\n> struct all_refs_cb cb;\n> init_all_refs_cb(&cb, revs, *flags);\n> for_each_glob_ref(handle_one_ref, optarg, &cb);\n> - clear_ref_exclusion(&revs->ref_excludes);\n> + clear_ref_selection_options(revs);\n> return argcount;\n> } else if ((argcount = parse_long_opt(\"exclude\", argv, &optarg))) {\n> add_ref_exclusion(&revs->ref_excludes, optarg);\n> @@ -2108,17 +2119,19 @@ static int handle_revision_pseudo_opt(const char *submodule,\n> struct all_refs_cb cb;\n> init_all_refs_cb(&cb, revs, *flags);\n> for_each_glob_ref_in(handle_one_ref, arg + 11, \"refs/heads/\", &cb);\n> - clear_ref_exclusion(&revs->ref_excludes);\n> + clear_ref_selection_options(revs);\n> } else if (starts_with(arg, \"--tags=\")) {\n> struct all_refs_cb cb;\n> init_all_refs_cb(&cb, revs, *flags);\n> for_each_glob_ref_in(handle_one_ref, arg + 7, \"refs/tags/\", &cb);\n> - clear_ref_exclusion(&revs->ref_excludes);\n> + clear_ref_selection_options(revs);\n> } else if (starts_with(arg, \"--remotes=\")) {\n> struct all_refs_cb cb;\n> init_all_refs_cb(&cb, revs, *flags);\n> for_each_glob_ref_in(handle_one_ref, arg + 10, \"refs/remotes/\", &cb);\n> - clear_ref_exclusion(&revs->ref_excludes);\n> + clear_ref_selection_options(revs);\n> + } else if (!strcmp(arg, \"--decorate-reflog\")) {\n> + revs->decorate_reflog = 1;\n> } else if (!strcmp(arg, \"--reflog\")) {\n> add_reflogs_to_pending(revs, *flags);\n> } else if (!strcmp(arg, \"--indexed-objects\")) {\n> diff --git a/revision.h b/revision.h\n> index 9fac1a607..c74879829 100644\n> --- a/revision.h\n> +++ b/revision.h\n> @@ -66,6 +66,8 @@ struct rev_info {\n> /* excluding from --branches, --refs, etc. expansion */\n> struct string_list *ref_excludes;\n>\n> + int decorate_reflog;\n> +\n> /* Basic information */\n> const char *prefix;\n> const char *def;\n"},{"id":"310261","messageId":"xmqqlgty7md8.fsf@gitster.mtv.corp.google.com","threadId":"44914","inReplyTo":"CA+P7+xovOx9ebo6MU0e4v==+76jtoMXz45+LnBPFifHbjqFU4w@mail.gmail.com","subject":"Re: [PATCH 4/5] revision.c: refactor ref selection handler after --exclude","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-25T23:25:55Z","receivedAt":"2017-01-25T23:26:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller writes:\n\n> On Wed, Jan 25, 2017 at 1:27 PM, Jeff King wrote:\n>> On Wed, Jan 25, 2017 at 03:57:18PM -0500, Jeff King wrote:\n>>\n>>> IOW, the ref-selector options build up until a group option is given,\n>>> which acts on the built-up options (over that group) and then resets the\n>>> built-up options. Doing \"--unrelated\" as above is orthogonal (though I\n>>> think in practice nobody would do that, because it's hard to read).\n>>\n>> So here's what I would have expected your series to look more like (with\n>> probably one patch adding clear_ref_selection_options, and the other\n>> adding the decorate stuff):\n>>\n>\n> I agree that this is how I would have expected it to work as well.\n\nThat makes three of us ;-)\n"},{"id":"310297","messageId":"CACsJy8Dwr5g6ZvemYgfg7Lecu=PsWz734tWgfFDCkWB_Htg7Hg@mail.gmail.com","threadId":"44914","inReplyTo":"xmqqo9yualqn.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/5] revision.c: group ref selection options together","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-01-26T09:12:11Z","receivedAt":"2017-01-26T09:12:51Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Jan 26, 2017 at 4:11 AM, Junio C Hamano wrote:\n> * I am expecting that the new one yet to be introduced will not\n> share the huge \"switch (selector)\" part, but does its own things\n> in a separate function with a similar structure. The only thing\n> common between these two functions would be the structure\n> (i.e. it has a big \"switch(selector)\" that does different things\n> depending on REF_SELECT_*) and a call to clear_* function.\n\nYep. The \"new one\" is demonstrated in 5/5.\n\n> If we were to add a new kind of REF_SELECT_* (say\n> REF_SELECT_NOTES just for the sake of being concrete), what\n> changes will be needed to the code if the addition of \"use reflog\n> from this class of refs for decoration\" feature was done with or\n> without this step? I have a suspicion that the change will be\n> simpler without this step.\n\nThe switch/case is to deal with new REF_SELECT_* (at least it's how I\nimagine it). What I was worried about was, when a user adds\n--select-notes, they may not be aware that it's in the same\nall/branches/tags/remotes group that's supposed to work with\n--decorate-reflog as well, and as a result \"--decorate-reflog\n--select-notes\" is the same as \"--select-notes\".\n\nWith the switch/case, when you add a new enum item, at the least the\ncompiler should warn about unhandled cases. And we can have a new\n\"case REF_SELECT_NOTES:\" for both --exclude and --decorate-reflog.\nWithout the switch/case, I guess it's still possible to do something\nlike\n\nif (!strcmp(arg, \"--select-notes\")) {\n if (preceded_by_exclude())\n does_one_thing();\n else if (preceded_by_decorate_reflog())\n does_another_thing();\n}\n\nIt's probably easier to maintain though, if all\ndecorate-reflog-related things are grouped together, rather than\nspread out per option like the above.\n-- \nDuy\n"},{"id":"310299","messageId":"CACsJy8CeUWD9ux3Q-VRhwR4-qbSD69CzH2mmztVH_-cx=31h8w@mail.gmail.com","threadId":"44914","inReplyTo":"20170125205037.cg3aebh5rsx4zb2l@sigill.intra.peff.net","subject":"Re: [PATCH 2/5] revision.c: group ref selection options together","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-01-26T09:18:06Z","receivedAt":"2017-01-26T09:18:46Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Jan 26, 2017 at 3:50 AM, Jeff King wrote:\n> On Wed, Jan 25, 2017 at 07:50:51PM +0700, Nguyễn Thái Ngọc Duy wrote:\n>\n>> These options have on thing in common: when specified right after\n>> --exclude, they will de-select refs instead of selecting them by\n>> default.\n>>\n>> This change makes it possible to introduce new options that use these\n>> options in the same way as --exclude. Such an option would just\n>> implement something like handle_refs_pseudo_opt().\n>>\n>> parse_ref_selector_option() is taken out of handle_refs_pseudo_opt() so\n>> that similar functions like handle_refs_pseudo_opt() are forced to\n>> handle all ref selector options, not skipping some by mistake, which may\n>> revert the option back to default behavior (rev selection).\n>>\n>> Signed-off-by: Nguyễn Thái Ngọc Duy \n>> ---\n>> revision.c | 134 +++++++++++++++++++++++++++++++++++++++++++++----------------\n>> 1 file changed, 100 insertions(+), 34 deletions(-)\n>\n> Hmm. I see what you're trying to do here, and abstract the repeated\n> bits. But I'm not sure the line-count reflects a real simplification.\n> Everything ends up converted to an enum, and then that enum just expands\n> to similar C code.\n\nIt's not simplification, but hopefully for better maintainability. This\n\nif (strcmp(arg, \"--remotes\")) {\n if (preceded_by_exclide())\n does_something();\n else if (preceded_by_decorate())\n does_another()\n} else if (strcmp(arg, \"--branches\")) {\n if (preceded_by_exclide())\n does_something();\n else if (preceded_by_decorate())\n does_another()\n}\n\nstarts to look ugly especially when the third \"preceded_by_\" comes\ninto picture. Putting all \"does_something\" in one group and\n\"does_another\" in another, I think, gives us a better view how ref\nselection is handled for a specific operation like --exclude or\n--decorate-ref.\n\n> I kind of expected that clear_ref_exclusion() would just become a more\n> abstract clear_ref_selection(). For now it would clear exclusions, and\n> then later learn to clear the decoration flags.\n\nIt may go that way, depending on how we handle these options for\ndecorate-reflog. The current load_ref_decorations() is not really\nsuited for fine-grained ref selection yet.\n--\nDuy\n"},{"id":"310300","messageId":"CACsJy8ATM_kc5SPY0dqprUefRy3vtpKW-4QEyJFK54jw0QgeJA@mail.gmail.com","threadId":"44914","inReplyTo":"20170125205718.ksqstdnazmgbkehy@sigill.intra.peff.net","subject":"Re: [PATCH 4/5] revision.c: refactor ref selection handler after --exclude","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-01-26T09:28:17Z","receivedAt":"2017-01-26T09:28:54Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Jan 26, 2017 at 3:57 AM, Jeff King wrote:\n> I don't think it means either. It means to include remotes in the\n> selected revisions, but excluding the entries mentioned by --exclude.\n>\n> IOW:\n>\n> --exclude=foo --remotes\n> include all remotes except refs/remotes/foo\n>\n> --exclude=foo --unrelated --remotes\n> same\n>\n> --exclude=foo --decorate-reflog --remotes\n> decorate reflogs of all remotes except \"foo\". Do _not_ use them\n> as traversal tips.\n>\n> --decorate-reflog --exclude=foo --remotes\n> same\n>\n> IOW, the ref-selector options build up until a group option is given,\n> which acts on the built-up options (over that group) and then resets the\n> built-up options. Doing \"--unrelated\" as above is orthogonal (though I\n> think in practice nobody would do that, because it's hard to read).\n\nThis is because it makes sense to combine --exclude and\n--decorate-reflog. But what about a new --something that conflicts\nwith either --exclude or --decorate-reflog? Should we simply catch\nsuch combinations and error out (which may be a bit more complicated\nthan this patch, or maybe not)?\n-- \nDuy\n"},{"id":"310316","messageId":"20170126141957.qcc75szbimqove2n@sigill.intra.peff.net","threadId":"44914","inReplyTo":"CACsJy8CeUWD9ux3Q-VRhwR4-qbSD69CzH2mmztVH_-cx=31h8w@mail.gmail.com","subject":"Re: [PATCH 2/5] revision.c: group ref selection options together","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-26T14:19:57Z","receivedAt":"2017-01-26T14:20:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 26, 2017 at 04:18:06PM +0700, Duy Nguyen wrote:\n\n> > Hmm. I see what you're trying to do here, and abstract the repeated\n> > bits. But I'm not sure the line-count reflects a real simplification.\n> > Everything ends up converted to an enum, and then that enum just expands\n> > to similar C code.\n> \n> It's not simplification, but hopefully for better maintainability. This\n> \n> if (strcmp(arg, \"--remotes\")) {\n> if (preceded_by_exclide())\n> does_something();\n> else if (preceded_by_decorate())\n> does_another()\n> } else if (strcmp(arg, \"--branches\")) {\n> if (preceded_by_exclide())\n> does_something();\n> else if (preceded_by_decorate())\n> does_another()\n> }\n> \n> starts to look ugly especially when the third \"preceded_by_\" comes\n> into picture. Putting all \"does_something\" in one group and\n> \"does_another\" in another, I think, gives us a better view how ref\n> selection is handled for a specific operation like --exclude or\n> --decorate-ref.\n\nI agree that would be ugly. But the current structure, which is:\n\n if (strcmp(arg, \"--remotes\")) {\n handle_refs(...);\n\t cleanup();\n } else if(...) {\n handle_refs(...);\n\t cleanup();\n }\n\ndoes not seem so bad, and pushes those conditionals into the\nhandle_refs() function, where they only need to be expressed once (I\ndidn't look, but I wonder if you could push the cleanup steps in there,\ntoo, or if there is a caller who wants to handle() multiple times before\ncleaning up).\n\n-Peff\n"},{"id":"310317","messageId":"20170126142423.6rrorahyzfo6z43q@sigill.intra.peff.net","threadId":"44914","inReplyTo":"CACsJy8ATM_kc5SPY0dqprUefRy3vtpKW-4QEyJFK54jw0QgeJA@mail.gmail.com","subject":"Re: [PATCH 4/5] revision.c: refactor ref selection handler after --exclude","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-26T14:24:24Z","receivedAt":"2017-01-26T14:24:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 26, 2017 at 04:28:17PM +0700, Duy Nguyen wrote:\n\n> On Thu, Jan 26, 2017 at 3:57 AM, Jeff King wrote:\n> > I don't think it means either. It means to include remotes in the\n> > selected revisions, but excluding the entries mentioned by --exclude.\n> >\n> > IOW:\n> >\n> > --exclude=foo --remotes\n> > include all remotes except refs/remotes/foo\n> >\n> > --exclude=foo --unrelated --remotes\n> > same\n> >\n> > --exclude=foo --decorate-reflog --remotes\n> > decorate reflogs of all remotes except \"foo\". Do _not_ use them\n> > as traversal tips.\n> >\n> > --decorate-reflog --exclude=foo --remotes\n> > same\n> >\n> > IOW, the ref-selector options build up until a group option is given,\n> > which acts on the built-up options (over that group) and then resets the\n> > built-up options. Doing \"--unrelated\" as above is orthogonal (though I\n> > think in practice nobody would do that, because it's hard to read).\n> \n> This is because it makes sense to combine --exclude and\n> --decorate-reflog. But what about a new --something that conflicts\n> with either --exclude or --decorate-reflog? Should we simply catch\n> such combinations and error out (which may be a bit more complicated\n> than this patch, or maybe not)?\n\nI'd cross that bridge when we see what the option is. But my gut is that\nrules would be:\n\n - apply all non-conflicting relevant options. So:\n\n --exclude=foo/* --decorate-refs --decorate-reflog --remotes\n\n would presumably decorate both ref tips _and_ reflogs for all\n remotes (except ones in refs/remotes/foo/*)\n\n - for ones that are directly related and override each other,\n use the usual last-one-wins rule. So:\n\n --decorate-reflog --no-decorate-reflog --remotes\n\n would countermand the original --decorate-reflog.\n\n - for ones that really have complex interactions, notice and complain\n in handle_refs().\n\nThat just seems to me like it follows our usual option parsing\nprocedure. The only difference here is that process and reset some\nsubset of the flags when we hit a special marker option (\"--remotes\" in\nthese examples) instead of doing it at the end.\n\n-Peff\n"},{"id":"310345","messageId":"xmqq8tpx64r5.fsf@gitster.mtv.corp.google.com","threadId":"44914","inReplyTo":"CACsJy8ATM_kc5SPY0dqprUefRy3vtpKW-4QEyJFK54jw0QgeJA@mail.gmail.com","subject":"Re: [PATCH 4/5] revision.c: refactor ref selection handler after --exclude","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-26T18:43:58Z","receivedAt":"2017-01-26T18:47:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Thu, Jan 26, 2017 at 3:57 AM, Jeff King wrote:\n>> I don't think it means either. It means to include remotes in the\n>> selected revisions, but excluding the entries mentioned by --exclude.\n>>\n>> IOW:\n>>\n>> --exclude=foo --remotes\n>> include all remotes except refs/remotes/foo\n>>\n>> --exclude=foo --unrelated --remotes\n>> same\n>>\n>> --exclude=foo --decorate-reflog --remotes\n>> decorate reflogs of all remotes except \"foo\". Do _not_ use them\n>> as traversal tips.\n>>\n>> --decorate-reflog --exclude=foo --remotes\n>> same\n>>\n>> IOW, the ref-selector options build up until a group option is given,\n>> which acts on the built-up options (over that group) and then resets the\n>> built-up options. Doing \"--unrelated\" as above is orthogonal (though I\n>> think in practice nobody would do that, because it's hard to read).\n>\n> This is because it makes sense to combine --exclude and\n> --decorate-reflog. But what about a new --something that conflicts\n> with either --exclude or --decorate-reflog?\n\nI would think that \"--exclude=foo --something --remotes\" \n\n * should be diagnosed as an error if \"--something\" is not compatible\n with \"--exclude\";\n\n * should take effect at the concluding \"--remotes\" if \"--something\"\n is similar to \"--decorate-reflog\" whose effect ends at a\n concluding --remotes/--branches/etc.; and\n\n * should work independently if \"--something\" is neither.\n\n"}]}