Re: [PATCH v4 7/7] tests: order arguments to git-rev-list properly

[Junio C Hamano]

Matthew DeVore  writes:

> I screwed up by putting the positional argument *after* the
> redirection. Sorry for the mix-up. This is interestingly syntactically
> valid, though bad stylistically. Here is an inter-diff:

Thanks for being careful.  Except for a rather idiomatic 

echo >&2 message ...

which has redirection at the beginning to emphasize that the output
goes to the standard error stream, I do agree with your "stylistic"
choice of keeping the redirection at the end.

> diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
> index eeedd1623..6ff614692 100755
> --- a/t/t5616-partial-clone.sh
> +++ b/t/t5616-partial-clone.sh
> @@ -35,7 +35,7 @@ test_expect_success 'setup bare clone for server' '
>  test_expect_success 'do partial clone 1' '
>  git clone --no-checkout --filter=blob:none
> "file://$(pwd)/srv.bare" pc1 &&
>
> -git -C pc1 rev-list --quiet --objects --missing=print >revs HEAD &&
> +git -C pc1 rev-list --quiet --objects --missing=print HEAD >revs &&
>  awk -f print_1.awk revs |
>  sed "s/?//" |
>  sort >observed.oids &&
> @@ -93,8 +93,8 @@ test_expect_success 'verify diff causes dynamic
> object fetch' '
>  test_expect_success 'verify blame causes dynamic object fetch' '
>  git -C pc1 blame origin/master -- file.1.txt >observed.blame &&
>  test_cmp expect.blame observed.blame &&
> -git -C pc1 rev-list --quiet --objects --missing=print >observed \
> -master..origin/master &&
> +git -C pc1 rev-list --quiet --objects --missing=print \
> +master..origin/master >observed &&
>  test_line_count = 0 observed
>  '

Re: [PATCH v4 7/7] tests: order arguments to git-rev-list properly

[Matthew DeVore]

On Wed, Oct 3, 2018 at 9:26 AM Matthew DeVore  wrote:
>
> -   git -C pc1 rev-list HEAD --quiet --objects --missing=print >revs &&
> +   git -C pc1 rev-list --quiet --objects --missing=print >revs HEAD &&
> awk -f print_1.awk revs |
...
> git -C pc1 blame origin/master -- file.1.txt >observed.blame &&
> test_cmp expect.blame observed.blame &&
> -   git -C pc1 rev-list master..origin/master --quiet --objects 
> --missing=print >observed &&
> +   git -C pc1 rev-list --quiet --objects --missing=print >observed \
> +   master..origin/master &&
> test_line_count = 0 observed

I screwed up by putting the positional argument *after* the
redirection. Sorry for the mix-up. This is interestingly syntactically
valid, though bad stylistically. Here is an inter-diff:

diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
index eeedd1623..6ff614692 100755
--- a/t/t5616-partial-clone.sh
+++ b/t/t5616-partial-clone.sh
@@ -35,7 +35,7 @@ test_expect_success 'setup bare clone for server' '
 test_expect_success 'do partial clone 1' '
 git clone --no-checkout --filter=blob:none
"file://$(pwd)/srv.bare" pc1 &&

-git -C pc1 rev-list --quiet --objects --missing=print >revs HEAD &&
+git -C pc1 rev-list --quiet --objects --missing=print HEAD >revs &&
 awk -f print_1.awk revs |
 sed "s/?//" |
 sort >observed.oids &&
@@ -93,8 +93,8 @@ test_expect_success 'verify diff causes dynamic
object fetch' '
 test_expect_success 'verify blame causes dynamic object fetch' '
 git -C pc1 blame origin/master -- file.1.txt >observed.blame &&
 test_cmp expect.blame observed.blame &&
-git -C pc1 rev-list --quiet --objects --missing=print >observed \
-master..origin/master &&
+git -C pc1 rev-list --quiet --objects --missing=print \
+master..origin/master >observed &&
 test_line_count = 0 observed
 '

[PATCH v4 7/7] tests: order arguments to git-rev-list properly

[Matthew DeVore]

It is a common mistake to put positional arguments before flags when
invoking git-rev-list. Order the positional arguments last.

This patch skips git-rev-list invocations which include the --not flag,
since the ordering of flags and positional arguments affects the
behavior. This patch also skips invocations of git-rev-list that occur
in command substitution in which the exit code is discarded, since
fixing those properly will require a more involved cleanup.

Signed-off-by: Matthew DeVore 
---
 t/t5616-partial-clone.sh| 26 +
 t/t5702-protocol-v2.sh  |  4 +--
 t/t6112-rev-list-filters-objects.sh | 43 ++---
 3 files changed, 44 insertions(+), 29 deletions(-)

diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
index fc7aeb1ab..eeedd1623 100755
--- a/t/t5616-partial-clone.sh
+++ b/t/t5616-partial-clone.sh
@@ -35,7 +35,7 @@ test_expect_success 'setup bare clone for server' '
 test_expect_success 'do partial clone 1' '
git clone --no-checkout --filter=blob:none "file://$(pwd)/srv.bare" pc1 
&&
 
-   git -C pc1 rev-list HEAD --quiet --objects --missing=print >revs &&
+   git -C pc1 rev-list --quiet --objects --missing=print >revs HEAD &&
awk -f print_1.awk revs |
sed "s/?//" |
sort >observed.oids &&
@@ -48,10 +48,10 @@ test_expect_success 'do partial clone 1' '
 
 # checkout master to force dynamic object fetch of blobs at HEAD.
 test_expect_success 'verify checkout with dynamic object fetch' '
-   git -C pc1 rev-list HEAD --quiet --objects --missing=print >observed &&
+   git -C pc1 rev-list --quiet --objects --missing=print HEAD >observed &&
test_line_count = 4 observed &&
git -C pc1 checkout master &&
-   git -C pc1 rev-list HEAD --quiet --objects --missing=print >observed &&
+   git -C pc1 rev-list --quiet --objects --missing=print HEAD >observed &&
test_line_count = 0 observed
 '
 
@@ -74,7 +74,8 @@ test_expect_success 'push new commits to server' '
 # have the new blobs.
 test_expect_success 'partial fetch inherits filter settings' '
git -C pc1 fetch origin &&
-   git -C pc1 rev-list master..origin/master --quiet --objects 
--missing=print >observed &&
+   git -C pc1 rev-list --quiet --objects --missing=print \
+   master..origin/master >observed &&
test_line_count = 5 observed
 '
 
@@ -82,7 +83,8 @@ test_expect_success 'partial fetch inherits filter settings' '
 # we should only get 1 new blob (for the file in origin/master).
 test_expect_success 'verify diff causes dynamic object fetch' '
git -C pc1 diff master..origin/master -- file.1.txt &&
-   git -C pc1 rev-list master..origin/master --quiet --objects 
--missing=print >observed &&
+   git -C pc1 rev-list --quiet --objects --missing=print \
+master..origin/master >observed &&
test_line_count = 4 observed
 '
 
@@ -91,7 +93,8 @@ test_expect_success 'verify diff causes dynamic object fetch' 
'
 test_expect_success 'verify blame causes dynamic object fetch' '
git -C pc1 blame origin/master -- file.1.txt >observed.blame &&
test_cmp expect.blame observed.blame &&
-   git -C pc1 rev-list master..origin/master --quiet --objects 
--missing=print >observed &&
+   git -C pc1 rev-list --quiet --objects --missing=print >observed \
+   master..origin/master &&
test_line_count = 0 observed
 '
 
@@ -111,7 +114,8 @@ test_expect_success 'push new commits to server for 
file.2.txt' '
 # Verify we have all the new blobs.
 test_expect_success 'override inherited filter-spec using --no-filter' '
git -C pc1 fetch --no-filter origin &&
-   git -C pc1 rev-list master..origin/master --quiet --objects 
--missing=print >observed &&
+   git -C pc1 rev-list --quiet --objects --missing=print \
+   master..origin/master >observed &&
test_line_count = 0 observed
 '
 
@@ -133,8 +137,8 @@ test_expect_success 'push new commits to server for 
file.3.txt' '
 test_expect_success 'manual prefetch of missing objects' '
git -C pc1 fetch --filter=blob:none origin &&
 
-   git -C pc1 rev-list master..origin/master --quiet --objects 
--missing=print \
-   >revs &&
+   git -C pc1 rev-list --quiet --objects --missing=print \
+master..origin/master >revs &&
awk -f print_1.awk revs |
sed "s/?//" |
sort >observed.oids &&
@@ -142,8 +146,8 @@ test_expect_success 'manual prefetch of missing objects' '
test_line_count = 6 observed.oids &&
git -C pc1 fetch-pack --stdin "file://$(pwd)/srv.bare" revs &&
+   git -C pc1 rev-list --quiet --objects --missing=print \
+   master..origin/master >revs &&
awk -f print_1.awk revs |
sed "s/?//" |
sort >observed.oids &&
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 54727450b..11a84efff 100755
---