Re: [PATCH v4] ref-filter: Add --no-contains option to tag/branch/for-each-ref
From: Junio C Hamano <hidden>
Date: 2017-03-12 04:44:56
Ævar Arnfjörð Bjarmason [off-list ref] 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.
quoted hunk
@@ -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.
quoted hunk
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?
quoted hunk
@@ -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.