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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Sun, Mar 12, 2017 at 5:44 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Æ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]*'

Sure, if you'd like it like that I can change it.

But as an aside, I don't understand why you think it happens to work
and doesn't make much sense. To someone reading "git help tag" this
would be *exactly* how you'd expect to have to write this, since the
example given in the synopsis is:

    git tag [-n[<num>]] -l [--contains <commit>] [....]

And then later in the documentation:

    -l <pattern>, --list <pattern>

I.e. for git-branch this type of invocation wouldn't make sense and
would just happen to work, but for git-tag the --list option is
explicitly documented to immediately take a <pattern> argument.

But of course the whole branch v.s. tag difference is just more fodder
for my "tag: Implicitly supply --list given another list-like option"
patch.

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

FWIW I just write one-liners like this, i.e. I don't add the
semantically meaningless spaces around all pipes to save myself some
typing, and wasn't trying to squeeze this all on one line, but sure
I'll change it.

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

Although I'll add a \ to that so you can still paste it to a terminal.

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

Sure.

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

Ack.

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

Okey, so two patches, one where I potentially produce very long lines
& then re-flow them in a subsequent commit.

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

*Nod*

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

If I'd like to base on top of that to make things easier for you do
you publish  jk/ref-filter-flags-cleanup sowhere? I.e. as a git ref
rather than me also following that topic, applying it on top of
master, and then applying my topic on top of that.

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

I don't know what makes to list in the synopsis given the default
arguments to --contains and --no-contains, maybe "[<object>]?", but in
any case, I'm not changing how that part is documented in
for-each-ref, just adding an alternative to the existing --contains
option.

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

It'll spew out "error: malformed object name --no-contains".

You can do "--contains HEAD --no-contains HEAD" to get nothing.

In an earlier thread I was discussing with Jeff whether it would be
worthwhile to error out in that case, but his opinion was
(paraphrasing) "Nah, GIGO", which I think makes sense in this case.

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

Yeah, it doesn't default to HEAD. It's inconsistent, due to a
historical wart in parse-options.[ch]

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

I may be missing some subtlety here, but I think you've misread this
(admittedly dense) chunk. the /WITH/ options don't supply NONEG, just
HIDDEN.

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

I'm not sure, and wanting to get the rest of this e-mail out the door,
I didn't have time to fully investigate this. I.e. is it consistent
with how --merged works? It uses a slightly different codepath, maybe,
but it's definitely consistent with how everything in the ref-filter
machinery works in general, and that's used by multiple options.

So to the extent that this needs docs we should do that in some
general place that covers existing options that use it.