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

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