{"thread":{"id":"49580","subject":"[RFC] revision: Add --sticky-default option","startedAt":"2018-10-16T21:24:44Z","lastAt":"2018-10-18T12:26:46Z","messageCount":12,"participants":["Andreas Gruenbacher","Jeff King","Matthew DeVore","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"360673","messageId":"20181016212438.30176-1-agruenba@redhat.com","threadId":"49580","inReplyTo":null,"subject":"[RFC] revision: Add --sticky-default option","fromName":"Andreas Gruenbacher","fromEmail":"agruenba@redhat.com","sentAt":"2018-10-16T21:24:38Z","receivedAt":"2018-10-16T21:24:44Z","isPatch":false,"sender":{"key":"agruenba@redhat.com","avatar":null},"body":"Hi,\n\nhere's a long-overdue update of my proposal from August 29:\n\n [RFC] revision: Don't let ^ cancel out the default \n\nDoes this look more acceptable that my first shot?\n\nThanks,\nAndreas\n\n--\n\nSome commands like 'log' default to HEAD if no other revisions are\nspecified on the command line or otherwise. Currently, excludes\n(^) cancel out that default, so when a command only excludes\nrevisions (e.g., 'git log ^origin/master'), it will produce no output.\n\nWith the --sticky-default option, the default becomes more \"sticky\" and\nis no longer canceled out by excludes.\n\nThis is useful in wrappers that exclude certain revisions: for example,\na simple alias l='git log --sticky-default ^origin/master' will show the\nrevisions between origin/master and HEAD when invoked without arguments,\nand 'l foo' will show the revisions between origin/master and foo.\n\nSigned-off-by: Andreas Gruenbacher \n---\n revision.c | 31 +++++++++++++++++++++++++++----\n revision.h | 1 +\n t/t4202-log.sh | 6 ++++++\n 3 files changed, 34 insertions(+), 4 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex e18bd530e..6c93ec74b 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1159,6 +1159,25 @@ static void add_rev_cmdline(struct rev_info *revs,\n \tinfo->nr++;\n }\n \n+static int has_revisions(struct rev_info *revs)\n+{\n+\tstruct rev_cmdline_info *info = &revs->cmdline;\n+\n+\treturn info->nr != 0;\n+}\n+\n+static int has_interesting_revisions(struct rev_info *revs)\n+{\n+\tstruct rev_cmdline_info *info = &revs->cmdline;\n+\tunsigned int n;\n+\n+\tfor (n = 0; n < info->nr; n++) {\n+\t\tif (!(info->rev[n].flags & UNINTERESTING))\n+\t\t\treturn 1;\n+\t}\n+\treturn 0;\n+}\n+\n static void add_rev_cmdline_list(struct rev_info *revs,\n \t\t\t\t struct commit_list *commit_list,\n \t\t\t\t int whence,\n@@ -2132,6 +2151,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t} else if (!strcmp(arg, \"--children\")) {\n \t\trevs->children.name = \"children\";\n \t\trevs->limited = 1;\n+\t} else if (!strcmp(arg, \"--sticky-default\")) {\n+\t\trevs->sticky_default = 1;\n \t} else if (!strcmp(arg, \"--ignore-missing\")) {\n \t\trevs->ignore_missing = 1;\n \t} else if (!strcmp(arg, \"--exclude-promisor-objects\")) {\n@@ -2319,7 +2340,8 @@ static void NORETURN diagnose_missing_default(const char *def)\n */\n int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct setup_revision_opt *opt)\n {\n-\tint i, flags, left, seen_dashdash, got_rev_arg = 0, revarg_opt;\n+\tint i, flags, left, seen_dashdash, revarg_opt;\n+\tint cancel_default;\n \tstruct argv_array prune_data = ARGV_ARRAY_INIT;\n \tconst char *submodule = NULL;\n \n@@ -2401,8 +2423,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t\targv_array_pushv(&prune_data, argv + i);\n \t\t\tbreak;\n \t\t}\n-\t\telse\n-\t\t\tgot_rev_arg = 1;\n \t}\n \n \tif (prune_data.argc) {\n@@ -2431,7 +2451,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\topt->tweak(revs, opt);\n \tif (revs->show_merge)\n \t\tprepare_show_merge(revs);\n-\tif (revs->def && !revs->pending.nr && !revs->rev_input_given && !got_rev_arg) {\n+\tcancel_default = revs->sticky_default ?\n+\t\t\t has_interesting_revisions(revs) :\n+\t\t\t has_revisions(revs);\n+\tif (revs->def && !revs->rev_input_given && !cancel_default) {\n \t\tstruct object_id oid;\n \t\tstruct object *object;\n \t\tstruct object_context oc;\ndiff --git a/revision.h b/revision.h\nindex 2b30ac270..570fa1a6d 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -92,6 +92,7 @@ struct rev_info {\n \n \tunsigned int early_output;\n \n+\tunsigned int\tsticky_default:1;\n \tunsigned int\tignore_missing:1,\n \t\t\tignore_missing_links:1;\n \ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 153a50615..9517a65da 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -213,6 +213,12 @@ test_expect_success 'git show leaves list of commits as given' '\n \ttest_cmp expect actual\n '\n \n+printf \"sixth\\nfifth\\n\" > expect\n+test_expect_success '--sticky-default ^' '\n+\tgit log --pretty=\"tformat:%s\" --sticky-default ^HEAD~2 > actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'setup case sensitivity tests' '\n \techo case >one &&\n \ttest_tick &&\n-- \n2.17.2\n\n"},{"id":"360734","messageId":"20181017091215.GA2052@sigill.intra.peff.net","threadId":"49580","inReplyTo":"20181016212438.30176-1-agruenba@redhat.com","subject":"Re: [RFC] revision: Add --sticky-default option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-17T09:12:15Z","receivedAt":"2018-10-17T09:12:19Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 16, 2018 at 11:24:38PM +0200, Andreas Gruenbacher wrote:\n\n> here's a long-overdue update of my proposal from August 29:\n> \n> [RFC] revision: Don't let ^ cancel out the default \n> \n> Does this look more acceptable that my first shot?\n\nI think it's going in the right direction.\n\nThe name \"--sticky-default\" did not immediately make clear to me what it\ndoes. Is there some name that would be more obvious?\n\n> Some commands like 'log' default to HEAD if no other revisions are\n> specified on the command line or otherwise. Currently, excludes\n> (^) cancel out that default, so when a command only excludes\n> revisions (e.g., 'git log ^origin/master'), it will produce no output.\n> \n> With the --sticky-default option, the default becomes more \"sticky\" and\n> is no longer canceled out by excludes.\n> \n> This is useful in wrappers that exclude certain revisions: for example,\n> a simple alias l='git log --sticky-default ^origin/master' will show the\n> revisions between origin/master and HEAD when invoked without arguments,\n> and 'l foo' will show the revisions between origin/master and foo.\n\nYour explanation makes sense.\n\n> ---\n> revision.c | 31 +++++++++++++++++++++++++++----\n> revision.h | 1 +\n> t/t4202-log.sh | 6 ++++++\n\nWe'd probably want an update to Documentation/rev-list-options.txt (I\nnotice that \"--default\" is not covered there; that might be worth\nfixing, too)>\n\n> +static int has_revisions(struct rev_info *revs)\n> +{\n> +\tstruct rev_cmdline_info *info = &revs->cmdline;\n> +\n> +\treturn info->nr != 0;\n> +}\n\nSo this function is going to replace this flag:\n\n> @@ -2401,8 +2423,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n> \t\t\targv_array_pushv(&prune_data, argv + i);\n> \t\t\tbreak;\n> \t\t}\n> -\t\telse\n> -\t\t\tgot_rev_arg = 1;\n> \t}\n\nAre we sure that every that hits that \"else\" is going to trigger\ninfo->nr (and vice versa)?\n\nIf I say \"--tags\", I think we may end up with entries in revs->cmdline,\nbut would not have set got_rev_arg. That's captured separately in\nrevs->rev_input_given. But your cancel_default logic:\n\n> @@ -2431,7 +2451,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n> \t\topt->tweak(revs, opt);\n> \tif (revs->show_merge)\n> \t\tprepare_show_merge(revs);\n> -\tif (revs->def && !revs->pending.nr && !revs->rev_input_given && !got_rev_arg) {\n> +\tcancel_default = revs->sticky_default ?\n> +\t\t\t has_interesting_revisions(revs) :\n> +\t\t\t has_revisions(revs);\n> +\tif (revs->def && !revs->rev_input_given && !cancel_default) {\n\ndoesn't handle that. So if I do:\n\n git rev-list --count --sticky-default --default HEAD --not --tags\n\nI should see everything in HEAD that's not tagged. But we don't even\nlook at cancel_default, because !revs->rev_input_given is not true.\n\nI think you could solve that by making the logic more like:\n\n if (revs->sticky_default)\n\tcancel_default = has_interesting_revisions();\n else\n\tcancel_default = !revs->rev_input_given && !got_rev_arg;\n\nwhich leaves the non-sticky case exactly as it is today.\n\n> diff --git a/revision.h b/revision.h\n> index 2b30ac270..570fa1a6d 100644\n> --- a/revision.h\n> +++ b/revision.h\n> @@ -92,6 +92,7 @@ struct rev_info {\n> \n> \tunsigned int early_output;\n> \n> +\tunsigned int\tsticky_default:1;\n> \tunsigned int\tignore_missing:1,\n> \t\t\tignore_missing_links:1;\n\nMaybe it would make sense to put this next to \"const char *def\"?\n\nThe bitfield would not be as efficient, but I don't think we care about\npacking rev_info tightly.\n\n> diff --git a/t/t4202-log.sh b/t/t4202-log.sh\n> index 153a50615..9517a65da 100755\n> --- a/t/t4202-log.sh\n> +++ b/t/t4202-log.sh\n> @@ -213,6 +213,12 @@ test_expect_success 'git show leaves list of commits as given' '\n> \ttest_cmp expect actual\n> '\n> \n> +printf \"sixth\\nfifth\\n\" > expect\n> +test_expect_success '--sticky-default ^' '\n> +\tgit log --pretty=\"tformat:%s\" --sticky-default ^HEAD~2 > actual &&\n> +\ttest_cmp expect actual\n> +'\n\nYuck, t4202 is a mix of older and newer styles. I'm OK with this as-is\nbecause you've matched the surrounding code, but these days I'd probably\nwrite:\n\n test_expect_success '--sticky-default ^' '\n\t{\n\t\techo sixth\n\t\techo fifth\n\t} >expect &&\n\tgit log --format=%s --sticky-default ^HEAD~2 >actual &&\n\ttest_cmp expect actual\n '\n\n-Peff\n"},{"id":"360749","messageId":"alpine.DEB.2.21.1810170620260.166761@matvore-linux.svl.corp.google.com","threadId":"49580","inReplyTo":"20181017091215.GA2052@sigill.intra.peff.net","subject":"Re: [RFC] revision: Add --sticky-default option","fromName":"Matthew DeVore","fromEmail":"matvore@comcast.net","sentAt":"2018-10-17T13:24:05Z","receivedAt":"2018-10-17T13:32:42Z","isPatch":false,"sender":{"key":"matvore@comcast.net","avatar":"https://gravatar.com/avatar/550c64ce544f82818ad931e244dfb08bbb1febfa6d1ce3cfd65e76215ca0ac8a?d=mp&s=160"},"body":"\nOn Wed, 17 Oct 2018, Jeff King wrote:\n\n>\n> Yuck, t4202 is a mix of older and newer styles. I'm OK with this as-is\n> because you've matched the surrounding code, but these days I'd probably\n> write:\n>\n> test_expect_success '--sticky-default ^' '\n> \t{\n> \t\techo sixth\n> \t\techo fifth\n> \t} >expect &&\n> \tgit log --format=%s --sticky-default ^HEAD~2 >actual &&\n> \ttest_cmp expect actual\n> '\n>\n\nHow about test_write_lines? That is a little more readable to me than\nthe echos in a subshell. A patch was recently queued with a usage of\nthat function:\n\nhttps://public-inbox.org/git/CAMfpvhK4a15gd-PT3W+4YJmpe6c7HyhJE5N_UqOzu8gsYYej4A@mail.gmail.com/T/#m9b5ade1551722938ac97b75af58fec195f246c01\n\n- Matt\n"},{"id":"360752","messageId":"CAHc6FU5mXL2j=jL=LqtRt30uBt8tGop350FnwK845fci-Qc=tg@mail.gmail.com","threadId":"49580","inReplyTo":"20181017091215.GA2052@sigill.intra.peff.net","subject":"Re: [RFC] revision: Add --sticky-default option","fromName":"Andreas Gruenbacher","fromEmail":"agruenba@redhat.com","sentAt":"2018-10-17T13:53:41Z","receivedAt":"2018-10-17T13:53:56Z","isPatch":false,"sender":{"key":"agruenba@redhat.com","avatar":null},"body":"On Wed, 17 Oct 2018 at 11:12, Jeff King wrote:\n> On Tue, Oct 16, 2018 at 11:24:38PM +0200, Andreas Gruenbacher wrote:\n> > here's a long-overdue update of my proposal from August 29:\n> >\n> > [RFC] revision: Don't let ^ cancel out the default \n> >\n> > Does this look more acceptable that my first shot?\n>\n> I think it's going in the right direction.\n>\n> The name \"--sticky-default\" did not immediately make clear to me what it\n> does. Is there some name that would be more obvious?\n\nIt's the best I could think of. Better ideas, anyone?\n\n> > Some commands like 'log' default to HEAD if no other revisions are\n> > specified on the command line or otherwise. Currently, excludes\n> > (^) cancel out that default, so when a command only excludes\n> > revisions (e.g., 'git log ^origin/master'), it will produce no output.\n> >\n> > With the --sticky-default option, the default becomes more \"sticky\" and\n> > is no longer canceled out by excludes.\n> >\n> > This is useful in wrappers that exclude certain revisions: for example,\n> > a simple alias l='git log --sticky-default ^origin/master' will show the\n> > revisions between origin/master and HEAD when invoked without arguments,\n> > and 'l foo' will show the revisions between origin/master and foo.\n>\n> Your explanation makes sense.\n>\n> > ---\n> > revision.c | 31 +++++++++++++++++++++++++++----\n> > revision.h | 1 +\n> > t/t4202-log.sh | 6 ++++++\n>\n> We'd probably want an update to Documentation/rev-list-options.txt (I\n> notice that \"--default\" is not covered there; that might be worth\n> fixing, too)>\n\nOk.\n\n> > +static int has_revisions(struct rev_info *revs)\n> > +{\n> > + struct rev_cmdline_info *info = &revs->cmdline;\n> > +\n> > + return info->nr != 0;\n> > +}\n>\n> So this function is going to replace this flag:\n>\n> > @@ -2401,8 +2423,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n> > argv_array_pushv(&prune_data, argv + i);\n> > break;\n> > }\n> > - else\n> > - got_rev_arg = 1;\n> > }\n>\n> Are we sure that every that hits that \"else\" is going to trigger\n> info->nr (and vice versa)?\n>\n> If I say \"--tags\", I think we may end up with entries in revs->cmdline,\n> but would not have set got_rev_arg. That's captured separately in\n> revs->rev_input_given. But your cancel_default logic:\n>\n> > @@ -2431,7 +2451,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n> > opt->tweak(revs, opt);\n> > if (revs->show_merge)\n> > prepare_show_merge(revs);\n> > - if (revs->def && !revs->pending.nr && !revs->rev_input_given && !got_rev_arg) {\n> > + cancel_default = revs->sticky_default ?\n> > + has_interesting_revisions(revs) :\n> > + has_revisions(revs);\n> > + if (revs->def && !revs->rev_input_given && !cancel_default) {\n>\n> doesn't handle that. So if I do:\n>\n> git rev-list --count --sticky-default --default HEAD --not --tags\n>\n> I should see everything in HEAD that's not tagged. But we don't even\n> look at cancel_default, because !revs->rev_input_given is not true.\n>\n> I think you could solve that by making the logic more like:\n>\n> if (revs->sticky_default)\n> cancel_default = has_interesting_revisions();\n> else\n> cancel_default = !revs->rev_input_given && !got_rev_arg;\n>\n> which leaves the non-sticky case exactly as it is today.\n\nRight, I've reworked that.\n\n> > diff --git a/revision.h b/revision.h\n> > index 2b30ac270..570fa1a6d 100644\n> > --- a/revision.h\n> > +++ b/revision.h\n> > @@ -92,6 +92,7 @@ struct rev_info {\n> >\n> > unsigned int early_output;\n> >\n> > + unsigned int sticky_default:1;\n> > unsigned int ignore_missing:1,\n> > ignore_missing_links:1;\n>\n> Maybe it would make sense to put this next to \"const char *def\"?\n>\n> The bitfield would not be as efficient, but I don't think we care about\n> packing rev_info tightly.\n\nOk.\n\n> > diff --git a/t/t4202-log.sh b/t/t4202-log.sh\n> > index 153a50615..9517a65da 100755\n> > --- a/t/t4202-log.sh\n> > +++ b/t/t4202-log.sh\n> > @@ -213,6 +213,12 @@ test_expect_success 'git show leaves list of commits as given' '\n> > test_cmp expect actual\n> > '\n> >\n> > +printf \"sixth\\nfifth\\n\" > expect\n> > +test_expect_success '--sticky-default ^' '\n> > + git log --pretty=\"tformat:%s\" --sticky-default ^HEAD~2 > actual &&\n> > + test_cmp expect actual\n> > +'\n>\n> Yuck, t4202 is a mix of older and newer styles. I'm OK with this as-is\n> because you've matched the surrounding code, but these days I'd probably\n> write:\n>\n> test_expect_success '--sticky-default ^' '\n> {\n> echo sixth\n> echo fifth\n> } >expect &&\n> git log --format=%s --sticky-default ^HEAD~2 >actual &&\n> test_cmp expect actual\n> '\n\nI don't really want to get hung up on such details. test_write_lines\ndoesn't seem bad, either.\n\nThanks,\nAndreas\n"},{"id":"360768","messageId":"20181017181156.GA28326@sigill.intra.peff.net","threadId":"49580","inReplyTo":"alpine.DEB.2.21.1810170620260.166761@matvore-linux.svl.corp.google.com","subject":"Re: [RFC] revision: Add --sticky-default option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-17T18:11:56Z","receivedAt":"2018-10-17T18:12:00Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 17, 2018 at 06:24:05AM -0700, Matthew DeVore wrote:\n\n> > Yuck, t4202 is a mix of older and newer styles. I'm OK with this as-is\n> > because you've matched the surrounding code, but these days I'd probably\n> > write:\n> > \n> > test_expect_success '--sticky-default ^' '\n> > \t{\n> > \t\techo sixth\n> > \t\techo fifth\n> > \t} >expect &&\n> > \tgit log --format=%s --sticky-default ^HEAD~2 >actual &&\n> > \ttest_cmp expect actual\n> > '\n> > \n> \n> How about test_write_lines? That is a little more readable to me than\n> the echos in a subshell. A patch was recently queued with a usage of\n> that function:\n\nAh, yeah, that would be fine. I was trying to avoid a cat/here-doc combo\nsince it incurs a process, but test_write_lines is readable and fast.\n\nThe main style things I wanted to show are:\n\n - setting up the expect file should go in the test_expect block\n\n - no space between \">\" and the filename\n\nThose are both present in the surrounding code, but we're slowly\ncleaning them up.\n\n-Peff\n"},{"id":"360769","messageId":"20181017181350.GB28326@sigill.intra.peff.net","threadId":"49580","inReplyTo":"CAHc6FU5mXL2j=jL=LqtRt30uBt8tGop350FnwK845fci-Qc=tg@mail.gmail.com","subject":"Re: [RFC] revision: Add --sticky-default option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-17T18:13:51Z","receivedAt":"2018-10-17T18:13:54Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 17, 2018 at 03:53:41PM +0200, Andreas Gruenbacher wrote:\n\n> On Wed, 17 Oct 2018 at 11:12, Jeff King wrote:\n> > On Tue, Oct 16, 2018 at 11:24:38PM +0200, Andreas Gruenbacher wrote:\n> > > here's a long-overdue update of my proposal from August 29:\n> > >\n> > > [RFC] revision: Don't let ^ cancel out the default \n> > >\n> > > Does this look more acceptable that my first shot?\n> >\n> > I think it's going in the right direction.\n> >\n> > The name \"--sticky-default\" did not immediately make clear to me what it\n> > does. Is there some name that would be more obvious?\n> \n> It's the best I could think of. Better ideas, anyone?\n\nI'd probably call it something verbose and boring like\n--use-default-with-uninteresting or --default-on-negative.\nI dunno.\n\n-Peff\n"},{"id":"360809","messageId":"xmqqva60uedt.fsf@gitster-ct.c.googlers.com","threadId":"49580","inReplyTo":"20181017181350.GB28326@sigill.intra.peff.net","subject":"Re: [RFC] revision: Add --sticky-default option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-18T03:23:26Z","receivedAt":"2018-10-18T03:23:32Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I'd probably call it something verbose and boring like\n> --use-default-with-uninteresting or --default-on-negative.\n> I dunno.\n\nThese two names are improvement, but there needs a hint that the\nchange we are interested in is to use default even when revs are\ngiven as long as ALL of them are negative ones. Which in turn means\nthere is NO positive ones given. \n\nSo perhaps \"--use-default-without-any-positive\".\n\nHaving said that, I have to wonder how serious a breakage we are\ngoing to cause to established users and scripts if we made this\nchange without any explicit option. After all, it would be rather\nobvious that people will get a history with some commits (or none at\nall) when they were expecting no output that the \"default behaviour\"\nhas changed. I also wonder how would scripts take advantage of the\ncurrent \"defeat --default as soon as we see any rev, even a negative\none\"---in short, I am not sure if the theoretical regression this\nnew \"option\" is trying to avoid is worth avoiding in the first\nplace.\n\nIs there a way to say \"usually this command has built-in --default=HEAD\nbehaviour, but I am declining that\" already, i.e. \n\n $ git log --no-default $REVS\n\nthat will result in an empty set if we accept the change proposed\nhere but make it unconditional? If so \"This and future versions of\nGit will honor the --default even when there are other revisions\ngiven on the command line, as long as they are ALL negative ones.\nThis is a backward incompatibile change, but you can update your\nscripts with '--no-default' if you do not like the new behaviour\" in\nthe release notes may be a viable alternative way forward.\n\nIf there is no such way in the released versions of Git, then that\nwould not work, and a strict opt-in like the approach taken by the\nproposed patch would become necessary.\n"},{"id":"360819","messageId":"20181018064845.GB23537@sigill.intra.peff.net","threadId":"49580","inReplyTo":"xmqqva60uedt.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC] revision: Add --sticky-default option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-18T06:48:46Z","receivedAt":"2018-10-18T06:48:50Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 18, 2018 at 12:23:26PM +0900, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I'd probably call it something verbose and boring like\n> > --use-default-with-uninteresting or --default-on-negative.\n> > I dunno.\n> \n> These two names are improvement, but there needs a hint that the\n> change we are interested in is to use default even when revs are\n> given as long as ALL of them are negative ones. Which in turn means\n> there is NO positive ones given.\n> \n> So perhaps \"--use-default-without-any-positive\".\n\nYeah, though that's getting pretty long. Another way is probably to say\n\"only\" more clearly. Like \"--default-on-only-negative\" or something.\n\n> Having said that, I have to wonder how serious a breakage we are\n> going to cause to established users and scripts if we made this\n> change without any explicit option. After all, it would be rather\n> obvious that people will get a history with some commits (or none at\n> all) when they were expecting no output that the \"default behaviour\"\n> has changed. I also wonder how would scripts take advantage of the\n> current \"defeat --default as soon as we see any rev, even a negative\n> one\"---in short, I am not sure if the theoretical regression this\n> new \"option\" is trying to avoid is worth avoiding in the first\n> place.\n\nJust to play devil's advocate, how about this:\n\n git log --branches=jk/* --not origin/master\n\nRight now that shows nothing if there are no matching branches. But I\nthink under the proposed behavior, it would start showing HEAD, which\nseems counter-intuitive.\n\nOr are we going to count any positive selector as a positive ref, even\nif it matches nothing? I could buy that, though it means that the\ncommand above is subtly different from one or both of:\n\n branches() {\n git for-each-ref --format='%(refname)' refs/heads/jk/*\n }\n\n # is --stdin a selector, too?\n branches | git log --stdin --not origin/master\n\n # here we have no idea that the user did a query and must show HEAD\n git log $(branches) --not origin/master\n\n> Is there a way to say \"usually this command has built-in --default=HEAD\n> behaviour, but I am declining that\" already, i.e. \n> \n> $ git log --no-default $REVS\n\nI don't think we have that, but regardless of this patch, it seems like\na potentially useful thing. I think we mostly get around it by the fact\nthat scripts ought to be using \"rev-list\", and it does not have such a\ndefault that needs to be overridden.\n\n-Peff\n"},{"id":"360822","messageId":"xmqqlg6vu4dw.fsf@gitster-ct.c.googlers.com","threadId":"49580","inReplyTo":"20181018064845.GB23537@sigill.intra.peff.net","subject":"Re: [RFC] revision: Add --sticky-default option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-18T06:59:23Z","receivedAt":"2018-10-18T06:59:29Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Just to play devil's advocate, how about this:\n>\n> git log --branches=jk/* --not origin/master\n>\n> Right now that shows nothing if there are no matching branches. But I\n> think under the proposed behavior, it would start showing HEAD, which\n> seems counter-intuitive.\n>\n> Or are we going to count any positive selector as a positive ref, even\n> if it matches nothing? \n\nThat sounds like an intuitive behaviour of the command, but I may\nchange my mind when I look at other examples. \n\nWhen viewing that \"--branches=jk/*\" example in isolation, yes, these\npositive selectors that could produce positive revs should defeat\nthe --default, especially when it is built-in (like \"log\"). When\ngiven by the user, I am not sure. With something like this:\n\n\tgit rev-list --default=HEAD --branches=jk/* ^master\n\nclearly the user anticipates that jk/* may or may not produce any\npositive refs; otherwise there is no point specifying the default.\n\nBut anyway...\n\n> I could buy that, though it means that the\n> command above is subtly different from one or both of:\n>\n> branches() {\n> git for-each-ref --format='%(refname)' refs/heads/jk/*\n> }\n>\n> # is --stdin a selector, too?\n> branches | git log --stdin --not origin/master\n>\n> # here we have no idea that the user did a query and must show HEAD\n> git log $(branches) --not origin/master\n\nOK, scrap that---just as I predicted a few minutes ago, I now think\nthat \"do we have a positive selector that can produce zero or more\nresult?\" is an ill-defined question X-<.\n\nThanks for a doze of sanity.\n"},{"id":"360839","messageId":"CAHc6FU7m=ikPyvzh0ztyOgfQjGD0oKEfGVxeRPRBy1Lh0VD+Fg@mail.gmail.com","threadId":"49580","inReplyTo":"xmqqlg6vu4dw.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC] revision: Add --sticky-default option","fromName":"Andreas Gruenbacher","fromEmail":"agruenba@redhat.com","sentAt":"2018-10-18T12:17:58Z","receivedAt":"2018-10-18T12:18:12Z","isPatch":false,"sender":{"key":"agruenba@redhat.com","avatar":null},"body":"On Thu, 18 Oct 2018 at 08:59, Junio C Hamano wrote:\n> Jeff King writes:\n> > Just to play devil's advocate, how about this:\n> >\n> > git log --branches=jk/* --not origin/master\n> >\n> > Right now that shows nothing if there are no matching branches. But I\n> > think under the proposed behavior, it would start showing HEAD, which\n> > seems counter-intuitive.\n> >\n> > Or are we going to count any positive selector as a positive ref, even\n> > if it matches nothing?\n>\n> That sounds like an intuitive behaviour of the command, but I may\n> change my mind when I look at other examples.\n>\n> When viewing that \"--branches=jk/*\" example in isolation, yes, these\n> positive selectors that could produce positive revs should defeat\n> the --default, especially when it is built-in (like \"log\").\n\nI agree, that's the kind of behavior I had in mind. (And I think\nthat's also what the code implements.)\n\n> When given by the user, I am not sure. With something like this:\n>\n> git rev-list --default=HEAD --branches=jk/* ^master\n>\n> clearly the user anticipates that jk/* may or may not produce any\n> positive refs; otherwise there is no point specifying the default.\n\nWith positive selectors, it is meaningful if the selectors match\nnothing. So in the above example, if jk/* matches nothing, I would\nreally expect no output, and the default should not be applied.\nSo we should just document that --default= only sets the default\nin case the default is used.\n\n> But anyway...\n>\n> > I could buy that, though it means that the\n> > command above is subtly different from one or both of:\n> >\n> > branches() {\n> > git for-each-ref --format='%(refname)' refs/heads/jk/*\n> > }\n> >\n> > # is --stdin a selector, too?\n> > branches | git log --stdin --not origin/master\n\nYes, it's a positive selector (since --not doesn't apply to --stdin).\n\n> > # here we have no idea that the user did a query and must show HEAD\n> > git log $(branches) --not origin/master\n\nIn this case, 'git log' is more used like git rev-list which doesn't\ndefault to HEAD. The 'git log --no-default' would neatly restore\nsanity here.\n\n> OK, scrap that---just as I predicted a few minutes ago, I now think\n> that \"do we have a positive selector that can produce zero or more\n> result?\" is an ill-defined question X-<.\n>\n> Thanks for a doze of sanity.\n\nAndreas\n"},{"id":"360840","messageId":"CAHc6FU4n0uiK64G_srb-B0b+Ut39fxMyyjZfYM_EogOtGXZzUQ@mail.gmail.com","threadId":"49580","inReplyTo":"xmqqva60uedt.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC] revision: Add --sticky-default option","fromName":"Andreas Gruenbacher","fromEmail":"agruenba@redhat.com","sentAt":"2018-10-18T12:23:54Z","receivedAt":"2018-10-18T12:24:08Z","isPatch":false,"sender":{"key":"agruenba@redhat.com","avatar":null},"body":"On Thu, 18 Oct 2018 at 05:23, Junio C Hamano wrote:\n> Jeff King writes:\n>\n> > I'd probably call it something verbose and boring like\n> > --use-default-with-uninteresting or --default-on-negative.\n> > I dunno.\n>\n> These two names are improvement, but there needs a hint that the\n> change we are interested in is to use default even when revs are\n> given as long as ALL of them are negative ones. Which in turn means\n> there is NO positive ones given.\n>\n> So perhaps \"--use-default-without-any-positive\".\n>\n> Having said that, I have to wonder how serious a breakage we are\n> going to cause to established users and scripts if we made this\n> change without any explicit option. After all, it would be rather\n> obvious that people will get a history with some commits (or none at\n> all) when they were expecting no output that the \"default behaviour\"\n> has changed. I also wonder how would scripts take advantage of the\n> current \"defeat --default as soon as we see any rev, even a negative\n> one\"---in short, I am not sure if the theoretical regression this\n> new \"option\" is trying to avoid is worth avoiding in the first\n> place.\n>\n> Is there a way to say \"usually this command has built-in --default=HEAD\n> behaviour, but I am declining that\" already, i.e.\n>\n> $ git log --no-default $REVS\n>\n> that will result in an empty set if we accept the change proposed\n> here but make it unconditional? If so \"This and future versions of\n> Git will honor the --default even when there are other revisions\n> given on the command line, as long as they are ALL negative ones.\n> This is a backward incompatibile change, but you can update your\n> scripts with '--no-default' if you do not like the new behaviour\" in\n> the release notes may be a viable alternative way forward.\n\nThat would certainly work for me.\n\nAndreas\n\n> If there is no such way in the released versions of Git, then that\n> would not work, and a strict opt-in like the approach taken by the\n> proposed patch would become necessary.\n"},{"id":"360841","messageId":"xmqq5zxz5tku.fsf@gitster-ct.c.googlers.com","threadId":"49580","inReplyTo":"CAHc6FU7m=ikPyvzh0ztyOgfQjGD0oKEfGVxeRPRBy1Lh0VD+Fg@mail.gmail.com","subject":"Re: [RFC] revision: Add --sticky-default option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-18T12:26:41Z","receivedAt":"2018-10-18T12:26:46Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andreas Gruenbacher writes:\n\n>> > # is --stdin a selector, too?\n>> > branches | git log --stdin --not origin/master\n>\n> Yes, it's a positive selector (since --not doesn't apply to --stdin).\n\nBut you should be able to do\n\n\tprintf \"%s\\n\" ^maint master | git rev-list --stdin\n\nReplace the second one with ^master and now you have nothing but\nnegative.\n\nSo, no, the line is much blurrier than you would think.\n"}]}