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

From: Jeff King <peff@peff.net>
Date: Oct 17, 2018, 09:12 UTC
Message-ID: <20181017091215.GA2052@sigill.intra.peff.net>
In-Reply-To: <20181016212438.30176-1-agruenba@redhat.com>

On Tue, Oct 16, 2018 at 11:24:38PM +0200, Andreas Gruenbacher wrote:

Show 5 quoted lines

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

Show 12 quoted lines

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

Show 6 quoted lines

> +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:

Show 7 quoted lines

> @@ -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:

Show 9 quoted lines

> @@ -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.

Show 11 quoted lines

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

Show 13 quoted lines

> 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