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

[Ævar Arnfjörð Bjarmason]

On Mon, Mar 20, 2017 at 8:52 PM, Jeff King  wrote:
> On Mon, Mar 20, 2017 at 10:32:47AM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> > I think the more relevant comparison is "--no-merged", and it behaves
>> > the same way as your new --no-contains. I don't think I saw this
>> > subtlety in the documentation, though. It might be worth mentioning
>> > (unless I just missed it).
>>
>> For --contains we explicitly document "contain the specified commit",
>> i.e. you couldn't expect this to list tree-test, and indeed it
>> doesn't:
>>
>> $ git tag tree-test master^{tree}
>> $ git tag -l --contains master '*test*'
>
> Right, "--contains" cannot have a commit inside a tree, so we were
> correct to skip the computation entirely. But does that mean that
> "--no-contains" should be the complement of that, or should it only
> include tags whose "contains" can be computed in the first place?
>
> IOW, I don't think --contains or --merged are interesting here; they
> give the right answer by skipping the computation. The question is what
> the "--no-" variants should do.

I think both should only ever find commits. I only came up with this
tree/blob scenario for the purposes of tests, but it would make the
command less useful & way slower in practice. E.g. now you want to
find what to revert to and some blob tag shows up.

>> However the --[no-]merged option says "reachable [...] from the
>> specified commit", which seems to me to be a bit more ambiguous as to
>> whether you could expect it to print tree/blob tags.
>
> I suspect that --no-merged behaves the way it does because it originally
> came from git-branch, where you only have commits in the first place.
> The other commands only learned about it during the move to ref-filter,
> and nobody thought about this corner case.
>
> So we could just treat it like a bug and fix it.  But I doubt anybody
> cares that much in practice either way, so documenting it as "any use of
> --contains, --no-contains, --no-merged, or --merged requires that the
> ref in question be a commit" is fine, too.

It's fixed in my soon-to-be resent series.

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

[Jeff King]

On Mon, Mar 20, 2017 at 10:32:47AM +0100, Ævar Arnfjörð Bjarmason wrote:

> > I think the more relevant comparison is "--no-merged", and it behaves
> > the same way as your new --no-contains. I don't think I saw this
> > subtlety in the documentation, though. It might be worth mentioning
> > (unless I just missed it).
> 
> For --contains we explicitly document "contain the specified commit",
> i.e. you couldn't expect this to list tree-test, and indeed it
> doesn't:
> 
> $ git tag tree-test master^{tree}
> $ git tag -l --contains master '*test*'

Right, "--contains" cannot have a commit inside a tree, so we were
correct to skip the computation entirely. But does that mean that
"--no-contains" should be the complement of that, or should it only
include tags whose "contains" can be computed in the first place?

IOW, I don't think --contains or --merged are interesting here; they
give the right answer by skipping the computation. The question is what
the "--no-" variants should do.

> However the --[no-]merged option says "reachable [...] from the
> specified commit", which seems to me to be a bit more ambiguous as to
> whether you could expect it to print tree/blob tags.

I suspect that --no-merged behaves the way it does because it originally
came from git-branch, where you only have commits in the first place.
The other commands only learned about it during the move to ref-filter,
and nobody thought about this corner case.

So we could just treat it like a bug and fix it.  But I doubt anybody
cares that much in practice either way, so documenting it as "any use of
--contains, --no-contains, --no-merged, or --merged requires that the
ref in question be a commit" is fine, too.

-Peff

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

[Ævar Arnfjörð Bjarmason]

On Mon, Mar 20, 2017 at 5:25 AM, Jeff King  wrote:
> On Sat, Mar 18, 2017 at 10:32:54AM +, Ævar Arnfjörð Bjarmason wrote:
>
>> Change the tag, branch & for-each-ref commands to have a --no-contains
>> option in addition to their longstanding --contains options.
>>
>> This allows for finding the last-good rollout tag given a known-bad
>> . Given a hypothetically bad commit cf5c7253e0 the git version
>> revert to can be found with this hacky two-liner:
>
> s/revert to/to &/, I think.
Done.
>
>> With this new --no-contains the same can be achieved with:
>> [..]
>
> The goal sounds good to me.
>
>> In addition to those tests, add a test for "tag" which asserts that
>> --no-contains won't find tree/blob tags, which is slightly
>> unintuitive, but consistent with how --contains works & is documented.
>
> Makes sense. In theory we could dig into commits to find trees and blobs
> when the user gives us one. But I kind of doubt anybody really wants it,
> and it's expensive to compute. For the simple cases, --points-at already
> does the right thing.
>
> [more on that below, though...]
>
>> @@ -604,7 +606,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;
>
> Could we wrap this long conditional?

Done. Will also go through the series as a whole & find other such occurances.

>> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
>> index df41fa0350..a11542c4fd 100644
>> --- a/builtin/for-each-ref.c
>> +++ b/builtin/for-each-ref.c
>> @@ -9,7 +9,7 @@ static char const * const for_each_ref_usage[] = {
>>   N_("git for-each-ref [] []"),
>>   N_("git for-each-ref [--points-at ]"),
>>   N_("git for-each-ref [(--merged | --no-merged) []]"),
>> - N_("git for-each-ref [--contains []]"),
>> + N_("git for-each-ref [(--contains | --no-contains) []]"),
>>   NULL
>
> I'm not sure if this presentation implies that the two cannot be used
> together. It copies "--merged/--no-merged", but I think those two
> _can't_ be used together (it probably wouldn't be hard to make it work,
> but if nobody cares it may not be worth spending time on).

Yeah this has been bothering me a bit too. I'll change the various
--help and synopsis entries to split them up, since they're not
mutually exclusive at all.

> I also wonder if we need to explicitly document that --contains and
> --no-contains can be used together and don't cancel each other. The
> other option is to pick a new name ("--omits" is the most concise one I
> could think of; maybe that is preferable anyway because it avoids
> negation).
>
>> @@ -457,7 +459,7 @@ int cmd_tag(int argc, const char **argv, const char 
>> *prefix)
>>   if (!cmdmode && !create_tag_object) {
>>   if (argc == 0)
>>   cmdmode = 'l';
>> - else if (filter.with_commit || filter.points_at.nr || 
>> filter.merge_commit || filter.lines != -1)
>> + else if (filter.with_commit || filter.no_commit || 
>> filter.points_at.nr || filter.merge_commit || filter.lines != -1)
>
> Ditto here on the wrapping. There were a few other long lines, but I
> won't point them all out.
>
>> - /* We perform the filtering for the '--contains' option */
>> + /* We perform the filtering for the '--contains' option... */
>>   if (filter->with_commit &&
>> - !commit_contains(filter, commit, 
>> _cbdata->contains_cache))
>> + !commit_contains(filter, commit, filter->with_commit, 
>> _cbdata->contains_cache))
>> + return 0;
>> + /* ...or for the `--no-contains' option */
>> + if (filter->no_commit &&
>> + commit_contains(filter, commit, filter->no_commit, 
>> _cbdata->no_contains_cache))
>>   return 0;
>
> This looks nice and simple. Good.
>
>> +# As the docs say, list tags which contain a specified *commit*. We
>> +# don't recurse down to tags for trees or blobs pointed to by *those*
>> +# commits.
>> +test_expect_success 'Does --[no-]contains stop at commits? Yes!' '
>> + cd no-contains &&
>> + blob=$(git rev-parse v0.3:v0.3.t) &&
>> + tree=$(git rev-parse v0.3^{tree}) &&
>> + git tag tag-blob $blob &&
>> + git tag tag-tree $tree &&
>> + git tag --contains v0.3 >actual &&
>> + cat >expected <<-\EOF &&
>> + v0.3
>> + v0.4
>> + v0.5
>> + EOF
>> + test_cmp expected actual &&
>> + git tag --no-contains v0.3 >actual &&
>> + cat >expected <<-\EOF &&
>> + v0.1
>> + v0.2
>> + EOF
>> + test_cmp expected actual
>> +'
>
> The 

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

[Jeff King]

On Sat, Mar 18, 2017 at 10:32:54AM +, Ævar Arnfjörð Bjarmason wrote:

> Change the tag, branch & for-each-ref commands to have a --no-contains
> option in addition to their longstanding --contains options.
> 
> This allows for finding the last-good rollout tag given a known-bad
> . Given a hypothetically bad commit cf5c7253e0 the git version
> revert to can be found with this hacky two-liner:

s/revert to/to &/, I think.


> With this new --no-contains the same can be achieved with:
> [..]

The goal sounds good to me.

> In addition to those tests, add a test for "tag" which asserts that
> --no-contains won't find tree/blob tags, which is slightly
> unintuitive, but consistent with how --contains works & is documented.

Makes sense. In theory we could dig into commits to find trees and blobs
when the user gives us one. But I kind of doubt anybody really wants it,
and it's expensive to compute. For the simple cases, --points-at already
does the right thing.

[more on that below, though...]

> @@ -604,7 +606,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;

Could we wrap this long conditional?

> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
> index df41fa0350..a11542c4fd 100644
> --- a/builtin/for-each-ref.c
> +++ b/builtin/for-each-ref.c
> @@ -9,7 +9,7 @@ static char const * const for_each_ref_usage[] = {
>   N_("git for-each-ref [] []"),
>   N_("git for-each-ref [--points-at ]"),
>   N_("git for-each-ref [(--merged | --no-merged) []]"),
> - N_("git for-each-ref [--contains []]"),
> + N_("git for-each-ref [(--contains | --no-contains) []]"),
>   NULL

I'm not sure if this presentation implies that the two cannot be used
together. It copies "--merged/--no-merged", but I think those two
_can't_ be used together (it probably wouldn't be hard to make it work,
but if nobody cares it may not be worth spending time on).

I also wonder if we need to explicitly document that --contains and
--no-contains can be used together and don't cancel each other. The
other option is to pick a new name ("--omits" is the most concise one I
could think of; maybe that is preferable anyway because it avoids
negation).

> @@ -457,7 +459,7 @@ int cmd_tag(int argc, const char **argv, const char 
> *prefix)
>   if (!cmdmode && !create_tag_object) {
>   if (argc == 0)
>   cmdmode = 'l';
> - else if (filter.with_commit || filter.points_at.nr || 
> filter.merge_commit || filter.lines != -1)
> + else if (filter.with_commit || filter.no_commit || 
> filter.points_at.nr || filter.merge_commit || filter.lines != -1)

Ditto here on the wrapping. There were a few other long lines, but I
won't point them all out.

> - /* We perform the filtering for the '--contains' option */
> + /* We perform the filtering for the '--contains' option... */
>   if (filter->with_commit &&
> - !commit_contains(filter, commit, 
> _cbdata->contains_cache))
> + !commit_contains(filter, commit, filter->with_commit, 
> _cbdata->contains_cache))
> + return 0;
> + /* ...or for the `--no-contains' option */
> + if (filter->no_commit &&
> + commit_contains(filter, commit, filter->no_commit, 
> _cbdata->no_contains_cache))
>   return 0;

This looks nice and simple. Good.

> +# As the docs say, list tags which contain a specified *commit*. We
> +# don't recurse down to tags for trees or blobs pointed to by *those*
> +# commits.
> +test_expect_success 'Does --[no-]contains stop at commits? Yes!' '
> + cd no-contains &&
> + blob=$(git rev-parse v0.3:v0.3.t) &&
> + tree=$(git rev-parse v0.3^{tree}) &&
> + git tag tag-blob $blob &&
> + git tag tag-tree $tree &&
> + git tag --contains v0.3 >actual &&
> + cat >expected <<-\EOF &&
> + v0.3
> + v0.4
> + v0.5
> + EOF
> + test_cmp expected actual &&
> + git tag --no-contains v0.3 >actual &&
> + cat >expected <<-\EOF &&
> + v0.1
> + v0.2
> + EOF
> + test_cmp expected actual
> +'

The tests mostly look fine, but this one puzzled me. I guess we're
checking that tag-blob does not contain v0.3. But how could it?

The more interesting test to me is:

  git tag --contains $blob

which should barf on a non-commit.

For the --no-contains side, you could argue that the blob-tag doesn't
contain the commit, and it should be listed. But it looks like we just
drop all non-commit tags completely as soon as we start to do a

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

[Ævar Arnfjörð Bjarmason]

Change the tag, branch & for-each-ref commands to have a --no-contains
option in addition to their longstanding --contains options.

This allows for finding the last-good rollout tag given a known-bad
. Given a hypothetically bad commit cf5c7253e0 the git version
revert to can be found with this hacky two-liner:

(git tag -l 'v[0-9]*'; git tag -l --contains cf5c7253e0 'v[0-9]*') |
sort | uniq -c | grep -E '^ *1 ' | awk '{print $2}' | tail -n 10

With this new --no-contains the same can be achieved with:

git tag -l --no-contains cf5c7253e0 'v[0-9]*' | sort | tail -n 10

As the filtering machinery is shared between the tag, branch &
for-each-ref commands, implement this for those commands too. A
practical use for this with "branch" is e.g. finding branches which
diverged between v2.8 & v2.10:

git branch --contains v2.8.0 --no-contains v2.10.0

The "describe" command also has a --contains option, but its semantics
are unrelated to what tag/branch/for-each-ref use --contains for. A
--no-contains option for "describe" wouldn't make any sense, other
than being exactly equivalent to not supplying --contains at all,
which would be confusing at best.

Add a --without option to "tag" as an alias for --no-contains, for
consistency with --with and --contains.  The --with option is
undocumented, and possibly the only user of it is
Junio (). But it's
trivial to support, so let's do that.

The additions to the the test suite are inverse copies of the
corresponding --contains tests. With this change --no-contains for
tag, branch & for-each-ref is just as well tested as the existing
--contains option.

In addition to those tests, add a test for "tag" which asserts that
--no-contains won't find tree/blob tags, which is slightly
unintuitive, but consistent with how --contains works & is documented.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 Documentation/git-branch.txt   |  15 ++--
 Documentation/git-for-each-ref.txt |   6 +-
 Documentation/git-tag.txt  |   6 +-
 builtin/branch.c   |   4 +-
 builtin/for-each-ref.c |   3 +-
 builtin/tag.c  |   8 ++-
 contrib/completion/git-completion.bash |   4 +-
 parse-options.h|   4 +-
 ref-filter.c   |  19 +++--
 ref-filter.h   |   1 +
 t/t3201-branch-contains.sh |  51 -
 t/t6302-for-each-ref-filter.sh |  16 +
 t/t7004-tag.sh | 128 +++--
 13 files changed, 240 insertions(+), 25 deletions(-)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 092f1bcf9f..5e52fc9b91 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 'git branch' [--color[=] | --no-color] [-r | -a]
[--list] [-v [--abbrev= | --no-abbrev]]
[--column[=] | --no-column]
-   [(--merged | --no-merged | --contains) []] [--sort=]
+   [(--merged | --no-merged | --contains | --no-contains) []] 
[--sort=]
[--points-at ] [--format=] [...]
 'git branch' [--set-upstream | --track | --no-track] [-l] [-f]  
[]
 'git branch' (--set-upstream-to= | -u ) []
@@ -35,7 +35,7 @@ as branch creation.
 
 With `--contains`, shows only the branches that contain the named commit
 (in other words, the branches whose tip commits are descendants of the
-named commit).  With `--merged`, only branches merged into the named
+named commit), `--no-contains` inverts it. With `--merged`, only branches 
merged into the named
 commit (i.e. the branches whose tip commits are reachable from the named
 commit) will be listed.  With `--no-merged` only branches not merged into
 the named commit will be listed.  If the  argument is missing it
@@ -213,6 +213,10 @@ start-point is either a local or remote-tracking branch.
Only list branches which contain the specified commit (HEAD
if not specified). Implies `--list`.
 
+--no-contains []::
+   Only list branches which don't contain the specified commit
+   (HEAD if not specified). Implies `--list`.
+
 --merged []::
Only list branches whose tips are reachable from the
specified commit (HEAD if not specified). Implies `--list`.
@@ -296,13 +300,16 @@ If you are creating a branch that you want to checkout 
immediately, it is
 easier to use the git checkout command with its `-b` option to create
 a branch and check it out with a single command.
 
-The options `--contains`, `--merged` and `--no-merged` serve three related
-but different purposes:
+The options `--contains`, `--no-contains`, `--merged` and `--no-merged`
+serve four related but different purposes:
 
 - `--contains ` is used to find all branches which will need
   special attention if  were to be rebased or amended, since those
   branches contain the specified .
 
+- `--no-contains ` is the