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