[RFC] revision: Add --sticky-default option

> here's a long-overdue update of my proposal from August 29:
> 
>   [RFC] revision: Don't let ^<rev> cancel out the default <rev>
> 
> Does this look more acceptable that my first shot?

> Some commands like 'log' default to HEAD if no other revisions are
> specified on the command line or otherwise.  Currently, excludes
> (^<rev>) cancel out that default, so when a command only excludes
> revisions (e.g., 'git log ^origin/master'), it will produce no output.
> 
> With the --sticky-default option, the default becomes more "sticky" and
> is no longer canceled out by excludes.
> 
> This is useful in wrappers that exclude certain revisions: for example,
> a simple alias l='git log --sticky-default ^origin/master' will show the
> revisions between origin/master and HEAD when invoked without arguments,
> and 'l foo' will show the revisions between origin/master and foo.

> +static int has_revisions(struct rev_info *revs)
> +{
> +	struct rev_cmdline_info *info = &revs->cmdline;
> +
> +	return info->nr != 0;
> +}

> @@ -2401,8 +2423,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
>  			argv_array_pushv(&prune_data, argv + i);
>  			break;
>  		}
> -		else
> -			got_rev_arg = 1;
>  	}

> @@ -2431,7 +2451,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
>  		opt->tweak(revs, opt);
>  	if (revs->show_merge)
>  		prepare_show_merge(revs);
> -	if (revs->def && !revs->pending.nr && !revs->rev_input_given && !got_rev_arg) {
> +	cancel_default = revs->sticky_default ?
> +			 has_interesting_revisions(revs) :
> +			 has_revisions(revs);
> +	if (revs->def && !revs->rev_input_given && !cancel_default) {

> diff --git a/revision.h b/revision.h
> index 2b30ac270..570fa1a6d 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -92,6 +92,7 @@ struct rev_info {
>  
>  	unsigned int early_output;
>  
> +	unsigned int	sticky_default:1;
>  	unsigned int	ignore_missing:1,
>  			ignore_missing_links:1;

> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> index 153a50615..9517a65da 100755
> --- a/t/t4202-log.sh
> +++ b/t/t4202-log.sh
> @@ -213,6 +213,12 @@ test_expect_success 'git show <commits> leaves list of commits as given' '
>  	test_cmp expect actual
>  '
>  
> +printf "sixth\nfifth\n" > expect
> +test_expect_success '--sticky-default ^<rev>' '
> +	git log --pretty="tformat:%s" --sticky-default ^HEAD~2 > actual &&
> +	test_cmp expect actual
> +'

>
> Yuck, t4202 is a mix of older and newer styles. I'm OK with this as-is
> because you've matched the surrounding code, but these days I'd probably
> write:
>
>  test_expect_success '--sticky-default ^<rev>' '
> 	{
> 		echo sixth
> 		echo fifth
> 	} >expect &&
> 	git log --format=%s --sticky-default ^HEAD~2 >actual &&
> 	test_cmp expect actual
>  '
>

> > Yuck, t4202 is a mix of older and newer styles. I'm OK with this as-is
> > because you've matched the surrounding code, but these days I'd probably
> > write:
> > 
> >  test_expect_success '--sticky-default ^<rev>' '
> > 	{
> > 		echo sixth
> > 		echo fifth
> > 	} >expect &&
> > 	git log --format=%s --sticky-default ^HEAD~2 >actual &&
> > 	test_cmp expect actual
> >  '
> > 
> 
> How about test_write_lines? That is a little more readable to me than
> the echos in a subshell. A patch was recently queued with a usage of
> that function:

> On Tue, Oct 16, 2018 at 11:24:38PM +0200, Andreas Gruenbacher wrote:
> > here's a long-overdue update of my proposal from August 29:
> >
> >   [RFC] revision: Don't let ^<rev> cancel out the default <rev>
> >
> > Does this look more acceptable that my first shot?
>
> I think it's going in the right direction.
>
> The name "--sticky-default" did not immediately make clear to me what it
> does. Is there some name that would be more obvious?

> > Some commands like 'log' default to HEAD if no other revisions are
> > specified on the command line or otherwise.  Currently, excludes
> > (^<rev>) cancel out that default, so when a command only excludes
> > revisions (e.g., 'git log ^origin/master'), it will produce no output.
> >
> > With the --sticky-default option, the default becomes more "sticky" and
> > is no longer canceled out by excludes.
> >
> > This is useful in wrappers that exclude certain revisions: for example,
> > a simple alias l='git log --sticky-default ^origin/master' will show the
> > revisions between origin/master and HEAD when invoked without arguments,
> > and 'l foo' will show the revisions between origin/master and foo.
>
> Your explanation makes sense.
>
> > ---
> >  revision.c     | 31 +++++++++++++++++++++++++++----
> >  revision.h     |  1 +
> >  t/t4202-log.sh |  6 ++++++
>
> We'd probably want an update to Documentation/rev-list-options.txt (I
> notice that "--default" is not covered there; that might be worth
> fixing, too)>

> > +static int has_revisions(struct rev_info *revs)
> > +{
> > +     struct rev_cmdline_info *info = &revs->cmdline;
> > +
> > +     return info->nr != 0;
> > +}
>
> So this function is going to replace this flag:
>
> > @@ -2401,8 +2423,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
> >                       argv_array_pushv(&prune_data, argv + i);
> >                       break;
> >               }
> > -             else
> > -                     got_rev_arg = 1;
> >       }
>
> Are we sure that every that hits that "else" is going to trigger
> info->nr (and vice versa)?
>
> If I say "--tags", I think we may end up with entries in revs->cmdline,
> but would not have set got_rev_arg. That's captured separately in
> revs->rev_input_given. But your cancel_default logic:
>
> > @@ -2431,7 +2451,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
> >               opt->tweak(revs, opt);
> >       if (revs->show_merge)
> >               prepare_show_merge(revs);
> > -     if (revs->def && !revs->pending.nr && !revs->rev_input_given && !got_rev_arg) {
> > +     cancel_default = revs->sticky_default ?
> > +                      has_interesting_revisions(revs) :
> > +                      has_revisions(revs);
> > +     if (revs->def && !revs->rev_input_given && !cancel_default) {
>
> doesn't handle that. So if I do:
>
>   git rev-list --count --sticky-default --default HEAD --not --tags
>
> I should see everything in HEAD that's not tagged. But we don't even
> look at cancel_default, because !revs->rev_input_given is not true.
>
> I think you could solve that by making the logic more like:
>
>   if (revs->sticky_default)
>         cancel_default = has_interesting_revisions();
>   else
>         cancel_default = !revs->rev_input_given && !got_rev_arg;
>
> which leaves the non-sticky case exactly as it is today.

> > diff --git a/revision.h b/revision.h
> > index 2b30ac270..570fa1a6d 100644
> > --- a/revision.h
> > +++ b/revision.h
> > @@ -92,6 +92,7 @@ struct rev_info {
> >
> >       unsigned int early_output;
> >
> > +     unsigned int    sticky_default:1;
> >       unsigned int    ignore_missing:1,
> >                       ignore_missing_links:1;
>
> Maybe it would make sense to put this next to "const char *def"?
>
> The bitfield would not be as efficient, but I don't think we care about
> packing rev_info tightly.

> > diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> > index 153a50615..9517a65da 100755
> > --- a/t/t4202-log.sh
> > +++ b/t/t4202-log.sh
> > @@ -213,6 +213,12 @@ test_expect_success 'git show <commits> leaves list of commits as given' '
> >       test_cmp expect actual
> >  '
> >
> > +printf "sixth\nfifth\n" > expect
> > +test_expect_success '--sticky-default ^<rev>' '
> > +     git log --pretty="tformat:%s" --sticky-default ^HEAD~2 > actual &&
> > +     test_cmp expect actual
> > +'
>
> Yuck, t4202 is a mix of older and newer styles. I'm OK with this as-is
> because you've matched the surrounding code, but these days I'd probably
> write:
>
>   test_expect_success '--sticky-default ^<rev>' '
>         {
>                 echo sixth
>                 echo fifth
>         } >expect &&
>         git log --format=%s --sticky-default ^HEAD~2 >actual &&
>         test_cmp expect actual
>   '

> On Wed, 17 Oct 2018 at 11:12, Jeff King <peff@peff.net> wrote:
> > On Tue, Oct 16, 2018 at 11:24:38PM +0200, Andreas Gruenbacher wrote:
> > > here's a long-overdue update of my proposal from August 29:
> > >
> > >   [RFC] revision: Don't let ^<rev> cancel out the default <rev>
> > >
> > > Does this look more acceptable that my first shot?
> >
> > I think it's going in the right direction.
> >
> > The name "--sticky-default" did not immediately make clear to me what it
> > does. Is there some name that would be more obvious?
> 
> It's the best I could think of. Better ideas, anyone?

> Jeff King <peff@peff.net> writes:
> 
> > I'd probably call it something verbose and boring like
> > --use-default-with-uninteresting or --default-on-negative.
> > I dunno.
> 
> These two names are improvement, but there needs a hint that the
> change we are interested in is to use default even when revs are
> given as long as ALL of them are negative ones.  Which in turn means
> there is NO positive ones given.
> 
> So perhaps "--use-default-without-any-positive".

> Having said that, I have to wonder how serious a breakage we are
> going to cause to established users and scripts if we made this
> change without any explicit option.  After all, it would be rather
> obvious that people will get a history with some commits (or none at
> all) when they were expecting no output that the "default behaviour"
> has changed.  I also wonder how would scripts take advantage of the
> current "defeat --default as soon as we see any rev, even a negative
> one"---in short, I am not sure if the theoretical regression this
> new "option" is trying to avoid is worth avoiding in the first
> place.

> Just to play devil's advocate, how about this:
>
>   git log --branches=jk/* --not origin/master
>
> Right now that shows nothing if there are no matching branches. But I
> think under the proposed behavior, it would start showing HEAD, which
> seems counter-intuitive.
>
> Or are we going to count any positive selector as a positive ref, even
> if it matches nothing? 

> I could buy that, though it means that the
> command above is subtly different from one or both of:
>
>   branches() {
>     git for-each-ref --format='%(refname)' refs/heads/jk/*
>   }
>
>   # is --stdin a selector, too?
>   branches | git log --stdin --not origin/master
>
>   # here we have no idea that the user did a query and must show HEAD
>   git log $(branches) --not origin/master

> Jeff King <peff@peff.net> writes:
> > Just to play devil's advocate, how about this:
> >
> >   git log --branches=jk/* --not origin/master
> >
> > Right now that shows nothing if there are no matching branches. But I
> > think under the proposed behavior, it would start showing HEAD, which
> > seems counter-intuitive.
> >
> > Or are we going to count any positive selector as a positive ref, even
> > if it matches nothing?
>
> That sounds like an intuitive behaviour of the command, but I may
> change my mind when I look at other examples.
>
> When viewing that "--branches=jk/*" example in isolation, yes, these
> positive selectors that could produce positive revs should defeat
> the --default, especially when it is built-in (like "log").

> When given by the user, I am not sure.  With something like this:
>
>         git rev-list --default=HEAD --branches=jk/* ^master
>
> clearly the user anticipates that jk/* may or may not produce any
> positive refs; otherwise there is no point specifying the default.

> But anyway...
>
> > I could buy that, though it means that the
> > command above is subtly different from one or both of:
> >
> >   branches() {
> >     git for-each-ref --format='%(refname)' refs/heads/jk/*
> >   }
> >
> >   # is --stdin a selector, too?
> >   branches | git log --stdin --not origin/master

> OK, scrap that---just as I predicted a few minutes ago, I now think
> that "do we have a positive selector that can produce zero or more
> result?" is an ill-defined question X-<.
>
> Thanks for a doze of sanity.

> Jeff King <peff@peff.net> writes:
>
> > I'd probably call it something verbose and boring like
> > --use-default-with-uninteresting or --default-on-negative.
> > I dunno.
>
> These two names are improvement, but there needs a hint that the
> change we are interested in is to use default even when revs are
> given as long as ALL of them are negative ones.  Which in turn means
> there is NO positive ones given.
>
> So perhaps "--use-default-without-any-positive".
>
> Having said that, I have to wonder how serious a breakage we are
> going to cause to established users and scripts if we made this
> change without any explicit option.  After all, it would be rather
> obvious that people will get a history with some commits (or none at
> all) when they were expecting no output that the "default behaviour"
> has changed.  I also wonder how would scripts take advantage of the
> current "defeat --default as soon as we see any rev, even a negative
> one"---in short, I am not sure if the theoretical regression this
> new "option" is trying to avoid is worth avoiding in the first
> place.
>
> Is there a way to say "usually this command has built-in --default=HEAD
> behaviour, but I am declining that" already, i.e.
>
>     $ git log --no-default $REVS
>
> that will result in an empty set if we accept the change proposed
> here but make it unconditional?  If so "This and future versions of
> Git will honor the --default even when there are other revisions
> given on the command line, as long as they are ALL negative ones.
> This is a backward incompatibile change, but you can update your
> scripts with '--no-default' if you do not like the new behaviour" in
> the release notes may be a viable alternative way forward.