git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Junio C Hamano <gitster@pobox.com>
Date
Mar 12, 2017, 04:44 UTC
Message-ID
<xmqqwpbvumrk.fsf@gitster.mtv.corp.google.com>
In-Reply-To
<20170311201858.27555-1-avarab@gmail.com>
Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
Show 15 quoted lines
> 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.
Show 5 quoted lines
> 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.

Show 5 quoted lines
>  '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.

Show 7 quoted lines
> @@ -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.
Show 13 quoted lines
> 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?
Show 35 quoted lines
> @@ -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.

Previous: Ævar Arnfjörð BjarmasonNext: Ævar Arnfjörð Bjarmason
Message 18 of 28 in “branch & tag: Add a --no-contains option”
  1. branch & tag: Add a --no-contains optionÆvar Arnfjörð Bjarmason, Mar 8, 2017
  2. Junio C HamanoMar 8, 2017
  3. Jeff KingMar 9, 2017
  4. Ævar Arnfjörð BjarmasonMar 9, 2017
  5. Jeff KingMar 9, 2017
  6. Ævar Arnfjörð BjarmasonMar 9, 2017
  7. Jeff KingMar 9, 2017
  8. 0/4 fix object flag pollution in "tag --contains"Jeff King, Mar 9, 2017
  9. 1/4 ref-filter: move ref_cbdata definition into ref-filter.cJeff King, Mar 9, 2017
  10. 2/4 ref-filter: use contains_result enum consistentlyJeff King, Mar 9, 2017
  11. 3/4 ref-filter: die on parse_commit errorsJeff King, Mar 9, 2017
  12. 4/4 ref-filter: use separate cache for contains_tag_algoJeff King, Mar 9, 2017
  13. Ævar Arnfjörð BjarmasonMar 11, 2017
  14. Ævar Arnfjörð BjarmasonMar 11, 2017
  15. Jeff KingMar 12, 2017
  16. Ævar Arnfjörð BjarmasonMar 11, 2017
  17. ref-filter: Add --no-contains option to tag/branch/for-each-refÆvar Arnfjörð Bjarmason, Mar 11, 2017
  18. Junio C HamanoMar 12, 2017
  19. Ævar Arnfjörð BjarmasonMar 12, 2017
  20. Junio C HamanoMar 12, 2017
  21. Ævar Arnfjörð BjarmasonMar 9, 2017
  22. Jeff KingMar 9, 2017
  23. Ævar Arnfjörð BjarmasonMar 10, 2017
  24. ref-filter: Add --no-contains option to tag/branch/for-each-refÆvar Arnfjörð Bjarmason, Mar 9, 2017
  25. Christian CouderMar 9, 2017
  26. Ævar Arnfjörð BjarmasonMar 10, 2017
  27. Ævar Arnfjörð BjarmasonMar 10, 2017
  28. ref-filter: Add --no-contains option to tag/branch/for-each-refÆvar Arnfjörð Bjarmason, Mar 10, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.