Re: [PATCH v4] ref-filter: Add --no-contains option to tag/branch/for-each-ref

[Junio C Hamano <gitster@pobox.com>]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Change the tag, branch & for-each-ref commands to have a --no-contains
> option in addition to their longstanding --contains options.
>
> The use-case I have for this is to find the last-good rollout tag
> given a known-bad <commit>. Right now, given a hypothetically bad
> commit v2.10.1-3-gcf5c7253e0, you can find which git version to revert
> to with this hacky two-liner:
>
>     (./git tag -l 'v[0-9]*'; ./git tag -l 'v[0-9]*' --contains v2.10.1-3-gcf5c7253e0) \
>         |sort|uniq -c|grep -E '^ *1 '|awk '{print $2}' | tail -n 10
>
> But with the --no-contains option you can now get the exact same
> output with:
>
>     ./git tag -l 'v[0-9]*' --no-contains v2.10.1-3-gcf5c7253e0|sort|tail -n 10

This command line, while it may happen to work, logically does not
make much sense.  Move the pattern to the end, i.e.

	git tag -l --no-contains v2.10.1-3-gcf5c7253e0 'v[0-9]*'

Also if an overlong line in an example disturbs you, do not solve it
by omitting SP around pipe.  If you are trying to make the result
readable, pick a readable solution, e.g.

    git tag -l --no-contains v2.10.1-3-gcf5c7253e0 'v[0-9]*' |
    sort | tail -n 10

Oh, drop ./ from ./git while at it ;-)

> The filtering machinery is generic between the tag, branch &
> for-each-ref commands, so once I'd implemented it for tag it was
> trivial to add support for this to the other two.

Also, we tend not to say "I did this, I do that".

	Because the filtering machinery is generic ..., support it
	for all three consistently.

> I'm adding a --without option to "tag" as an alias for --no-contains
> for consistency with --with and --contains. Since we don't even
> document --with anymore (or test it). The --with option is
> undocumented, and possibly the only user of it is Junio[1]. But it's
> trivial to support, so let's do that.

The sentence that begins "Since we don't" is unfinished.  I think
it can safely removed without losing any information (the next
sentence says the same thing).

> Where I'm changing existing documentation lines I'm mainly word
> wrapping at 75 columns to be consistent with the existing style.

Reviewers would appreciate you refrain from doing that in the same
patch.  Do a minimum patch so that the review can concentrate on
what got changed (i.e. contents), followed by a mechanical reflow as
a follow-up, or something like that, would be much nicer to handle.

> Most of the test changes I've made are just doing the inverse of the
> existing --contains tests, with this change --no-contains for tag,
> branch & for-each-ref is just as well tested as the existing
> --contains option.

Again, we tend to try our commits not about "I, my, me".

	Add --no-contains tests for tag, branch and for-each-ref
	that mostly do the inverse of the existing tests we have for
	--contains.

> This is now based on top of pu, which has Jeff King's "fix object flag
> pollution in "tag --contains" series.

Thanks for this note.  I obviously cannot queue on top of 'pu' ;-)
but will fork this topic off of the jk/ref-filter-flags-cleanup
topic.

>  'git for-each-ref' [--count=<count>] [--shell|--perl|--python|--tcl]
>  		   [(--sort=<key>)...] [--format=<format>] [<pattern>...]
>  		   [--points-at <object>] [(--merged | --no-merged) [<object>]]
> -		   [--contains [<object>]]
> +		   [(--contains | --no-contains) [<object>]]

THis notation makes sense.  We have to have one of these but
<object> at the end could be omitted (to default to HEAD).  I guess
the same notation can be used in the log for the other "filtering
implies --list mode for 'git tag'" topic.

> +--no-contains [<commit>]::
> +	Only list tags which don't contain the specified commit (HEAD if
> +	not specified).

Just being curious.  Can we do

	for-each-ref --contains --no-contains 

and have both default to HEAD?  I know that would not make sense as
a set operation, but I am curious what our command line parser
(which is oblivious to what the command is doing) does.  I am guessing
that it would barf saying "--contains" needs a commit but "--no-contains"
is not a commit (which is very sensible)?

> +
>  --points-at <object>::
>  	Only list tags of the given object.

This is not a new issue (and certainly not a problem caused by your
patch), but unlike "--contains", this does not default to HEAD when
<object> is not explicitly given?  It seems a bit inconsistent to me.

> @@ -618,7 +620,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>  	if (!delete && !rename && !edit_description && !new_upstream && !unset_upstream && argc == 0)
>  		list = 1;
>  
> -	if (filter.with_commit || filter.merge != REF_FILTER_MERGED_NONE || filter.points_at.nr)
> +	if (filter.with_commit || filter.no_commit || filter.merge != REF_FILTER_MERGED_NONE || filter.points_at.nr)
>  		list = 1;

OK.

> diff --git a/parse-options.h b/parse-options.h
> index dcd8a0926c..0eac90b510 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -258,7 +258,9 @@ extern int parse_opt_passthru_argv(const struct option *, const char *, int);
>  	  PARSE_OPT_LASTARG_DEFAULT | flag, \
>  	  parse_opt_commits, (intptr_t) "HEAD" \
>  	}
> -#define OPT_CONTAINS(v, h) _OPT_CONTAINS_OR_WITH("contains", v, h, 0)
> +#define OPT_CONTAINS(v, h) _OPT_CONTAINS_OR_WITH("contains", v, h, PARSE_OPT_NONEG)
> +#define OPT_NO_CONTAINS(v, h) _OPT_CONTAINS_OR_WITH("no-contains", v, h, PARSE_OPT_NONEG)
>  #define OPT_WITH(v, h) _OPT_CONTAINS_OR_WITH("with", v, h, PARSE_OPT_HIDDEN)
> +#define OPT_WITHOUT(v, h) _OPT_CONTAINS_OR_WITH("without", v, h, PARSE_OPT_HIDDEN)

Hmph, perhaps WITH/WITHOUT also do not take "--no-" form hence need OPT_NONEG?

> @@ -1586,11 +1587,11 @@ static enum contains_result contains_tag_algo(struct commit *candidate,
>  }
>  
>  static int commit_contains(struct ref_filter *filter, struct commit *commit,
> -			   struct contains_cache *cache)
> +			   struct commit_list *list, struct contains_cache *cache)
>  {
>  	if (filter->with_commit_tag_algo)
> -		return contains_tag_algo(commit, filter->with_commit, cache) == CONTAINS_YES;
> -	return is_descendant_of(commit, filter->with_commit);
> +		return contains_tag_algo(commit, list, cache) == CONTAINS_YES;
> +	return is_descendant_of(commit, list);
>  }
>  
>  /*
> @@ -1780,13 +1781,17 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
>  	 * obtain the commit using the 'oid' available and discard all
>  	 * non-commits early. The actual filtering is done later.
>  	 */
> -	if (filter->merge_commit || filter->with_commit || filter->verbose) {
> +	if (filter->merge_commit || filter->with_commit || filter->no_commit || filter->verbose) {
>  		commit = lookup_commit_reference_gently(oid->hash, 1);
>  		if (!commit)
>  			return 0;
> -		/* We perform the filtering for the '--contains' option */
> +		/* We perform the filtering for the '--contains' option... */
>  		if (filter->with_commit &&
> -		    !commit_contains(filter, commit, &ref_cbdata->contains_cache))
> +		    !commit_contains(filter, commit, filter->with_commit, &ref_cbdata->contains_cache))
> +			return 0;
> +		/* ...or for the `--no-contains' option */
> +		if (filter->no_commit &&
> +		    commit_contains(filter, commit, filter->no_commit, &ref_cbdata->no_contains_cache))
>  			return 0;
>  	}

When asking "--contains A --contains B", we show refs that contain
_EITHER_ A or B.  Two predicates are ORed together, and I think it
makes sense.

When asking "--contains A --no-contains B", we show refs that
contain A but exclude refs that contains B.  Two predicates are
ANDed together, and I think this also makes sense.

When asking "--no-contains A --no-contains B", what should we show?
This implementation makes the two predicates ANDed together [*1*].

The behaviour is sensible, but is it consistent with the way now
existing --no-merged works?

I think the rule is something like:

    A match with any positive selection criterion (like --contains
    A) makes a ref eligible for output, but then a match with any
    negatigve selection criterion (like --no-merged) filters it out.

Is it easy to explain to the users?  Do we need doc updates to
clarify, or does the description for existing --no-merged already
cover this?

Thanks.


[Footnote]

*1* ... because it uses the same commit_contains() machinery that
computes "contains either A or B" used for the first one and then
negates its result.