Re: [PATCH 1/2] refs: show --exclude failure with --branches/tags/remotes=glob

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Rafael Ascensão <rafa.almas@xxxxxxxxx> writes:

> The documentation of `--exclude=` option from rev-list and rev-parse
> explicitly states that exclude patterns *should not* start with 'refs/'
> when used with `--branches`, `--tags` or `--remotes`.
>
> However, following this advice results in refereces not being excluded
> if the next `--branches`, `--tags`, `--remotes` use the optional
> inclusive glob.
>
> Demonstrate this failure.
>
> Signed-off-by: Rafael Ascensão <rafa.almas@xxxxxxxxx>
> ---
>  t/t6018-rev-list-glob.sh | 60 ++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 57 insertions(+), 3 deletions(-)

For a trivially small change/fix like this (i.e. the real fix in 2/2
is just 4 lines), it is OK and even preferrable to make 1+2 a single
step, as applying t/ part only to try to see the breakage (or
"am"ing everything and then "diff | apply -R" the part outside t/
for the same purpose) is easy enough.

Often the patch 2 with your method ends up showing only the test
set-up part in the context by changing _failure to _success, without
showing what end-user visible breakage the step fixed, which usually
comes near the end of the added test piece.  For this particular
test, s/_failure/_success/ shows everything in the verification
phase, but the entire set-up for these tests cannot be seen while
reviewing 2/2.  Unlike that, a single patch that gives tests that
ought to succeed would not force the readers to switch between
patches 1 and 2 while reading the fix.

Of course, the above would not apply for a more involved case where
the actual fix to the code needs to span multiple patches.

> diff --git a/t/t6018-rev-list-glob.sh b/t/t6018-rev-list-glob.sh
> index 0bf10d0686..8e2b136356 100755
> --- a/t/t6018-rev-list-glob.sh
> +++ b/t/t6018-rev-list-glob.sh
> @@ -36,7 +36,13 @@ test_expect_success 'setup' '
>  	git tag foo/bar master &&
>  	commit master3 &&
>  	git update-ref refs/remotes/foo/baz master &&
> -	commit master4
> +	commit master4 &&
> +	git update-ref refs/remotes/upstream/one subspace/one &&
> +	git update-ref refs/remotes/upstream/two subspace/two &&
> +	git update-ref refs/remotes/upstream/x subspace-x &&
> +	git tag qux/one subspace/one &&
> +	git tag qux/two subspace/two &&
> +	git tag qux/x subspace-x
>  '

Let me follow along.

We add three remote-tracking looking branches for 'upstream', and
three tags under refs/tags/qux/.


> +test_expect_failure 'rev-parse --exclude=glob with --branches=glob' '
> +	compare rev-parse "--exclude=subspace-* --branches=sub*" "subspace/one subspace/two"
> +'

We want to list all branches that begin with "sub", but we do not
want ones that begin with "subspace-".  subspace/{one,two} should
pass that criteria, while subspace-x, other/three, someref, and
master should not.  Makes sense.

> +
> +test_expect_failure 'rev-parse --exclude=glob with --tags=glob' '
> +	compare rev-parse "--exclude=qux/? --tags=qux/*" "qux/one qux/two"
> +'

We want all tags that begin with "qux/" but we do not want qux/
followed by just a single letter.  qux/{one,two} are in, qux/x is
out.  Makes sense.

> +test_expect_failure 'rev-parse --exclude=glob with --remotes=glob' '
> +	compare rev-parse "--exclude=upstream/? --remotes=upstream/*" "upstream/one upstream/two"
> +'

Similarly for refs/remotes/upstream/ hierarchy.

> +test_expect_failure 'rev-parse --exclude=ref with --branches=glob' '
> +	compare rev-parse "--exclude=subspace-x --branches=sub*" "subspace/one subspace/two
> +'

This is almost a repeat of the first new one.  As subspace-* in
branches only match subspace-x, this should give the same result as
that one.

> +test_expect_failure 'rev-parse --exclude=ref with --tags=glob' '
> +	compare rev-parse "--exclude=qux/x --tags=qux/*" "qux/one qux/two"
> +'

Likewise.

> +test_expect_failure 'rev-parse --exclude=ref with --remotes=glob' '
> +	compare rev-parse "--exclude=upstream/x --remotes=upstream/*" "upstream/one upstream/two"
> +'

Likewise.

> +test_expect_failure 'rev-list --exclude=glob with --branches=glob' '
> +	compare rev-list "--exclude=subspace-* --branches=sub*" "subspace/one subspace/two"
> +'

And then the same pattern continues with rev-list.

> +test_expect_failure 'rev-list --exclude=glob with --tags=glob' '
> +	compare rev-list "--exclude=qux/? --tags=qux/*" "qux/one qux/two"
> +'
> +
> +test_expect_failure 'rev-list --exclude=glob with --remotes=glob' '
> +	compare rev-list "--exclude=upstream/? --remotes=upstream/*" "upstream/one upstream/two"
> +'
> +
> +test_expect_failure 'rev-list --exclude=ref with --branches=glob' '
> +	compare rev-list "--exclude=subspace-x --branches=sub*" "subspace/one subspace/two"
> +'
> +
> +test_expect_failure 'rev-list --exclude=ref with --tags=glob' '
> +	compare rev-list "--exclude=qux/x --tags=qux/*" "qux/one qux/two"
> +'
> +
> +test_expect_failure 'rev-list --exclude=ref with --remotes=glob' '
> +	compare rev-list "--exclude=upstream/x --remotes=upstream/*" "upstream/one upstream/two"
> +'
> +

With the ordering of these tests, it is fairly clear that you are
exhaustively testing all the combinations 
	
for command in rev-parse rev-list:
	for exclude in glob ref:
		for specifc in glob ref:
			for kind in branches tags remotes:
				compare $command exclude=$exclude --$kind=$specific

which is very good.  No, I am not suggesting to write a shell loop
to drive these tests; I am saying that the list of tests are in the
same order as such a nested loop would invoke compare, which makes
it predictable for the readers who pay attention, and it is a good
thing.


>  test_expect_success 'rev-list --glob=refs/heads/subspace/*' '
>  
>  	compare rev-list "subspace/one subspace/two" "--glob=refs/heads/subspace/*"
> @@ -233,7 +287,7 @@ test_expect_success 'rev-list --tags=foo' '
>  
>  test_expect_success 'rev-list --tags' '
>  
> -	compare rev-list "foo/bar" "--tags"
> +	compare rev-list "foo/bar qux/x qux/two qux/one" "--tags"

Of course, you'd need to compensate for new stuff here ...

>  
>  '
>  
> @@ -292,7 +346,7 @@ test_expect_success 'shortlog accepts --glob/--tags/--remotes' '
>  	  "master other/three someref subspace-x subspace/one subspace/two" \
>  	  "--glob=heads/*" &&
>  	compare shortlog foo/bar --tags=foo &&
> -	compare shortlog foo/bar --tags &&
> +	compare shortlog "foo/bar qux/one qux/two qux/x" --tags &&

... and here.

>  	compare shortlog foo/baz --remotes=foo

All makes sense.  Will queue.





[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux