On Wed, 17 Oct 2018 at 11:12, Jeff King <peff@xxxxxxxx> 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?
> > 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)>
Ok.
> > +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.
Right, I've reworked that.
> > 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.
Ok.
> > 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
> '
I don't really want to get hung up on such details. test_write_lines
doesn't seem bad, either.
Thanks,
Andreas
