threads / patch / 47397

patchdiff: add tests for --relative without optional prefix value

Subject: [PATCH] diff: add tests for --relative without optional prefix value

## tl;dr

7 messages between Dec 7, 2017 and Dec 7, 2017. Diffs are folded; open one to read it.

replies: 6people: 4as markdown or json

Jacob Keller· Dec 7, 2017, 19:01 UTC · lore
From: Jacob Keller <jacob.keller@gmail.com>

We already have tests for --relative, but they currently only test when a prefix has been provided. This fails to test the case where --relative by itself should use the current directory as the prefix.

Teach the check_$type functions to take a directory argument to indicate which subdirectory to run the git commands in. Add a new test which uses this to test --relative without a prefix value.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 t/t4045-diff-relative.sh | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)
Show changes to t/t4045-diff-relative.sh +12 −7
diff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh
index 3950f5034d31..7d68a6e2a536 100755
--- a/t/t4045-diff-relative.sh
+++ b/t/t4045-diff-relative.sh
@@ -13,6 +13,7 @@ test_expect_success 'setup' '
 '
 
 check_diff() {
+dir=$1; shift
 expect=$1; shift
 cat >expected <<EOF
 diff --git a/$expect b/$expect
@@ -24,50 +25,54 @@ index 0000000..25c05ef
 +other content
 EOF
 test_expect_success "-p $*" "
-	git diff -p $* HEAD^ >actual &&
+	git -C '$dir' diff -p $* HEAD^ >actual &&
 	test_cmp expected actual
 "
 }
 
 check_numstat() {
+dir=$1; shift
 expect=$1; shift
 cat >expected <<EOF
 1	0	$expect
 EOF
 test_expect_success "--numstat $*" "
 	echo '1	0	$expect' >expected &&
-	git diff --numstat $* HEAD^ >actual &&
+	git -C '$dir' diff --numstat $* HEAD^ >actual &&
 	test_cmp expected actual
 "
 }
 
 check_stat() {
+dir=$1; shift
 expect=$1; shift
 cat >expected <<EOF
  $expect | 1 +
  1 file changed, 1 insertion(+)
 EOF
 test_expect_success "--stat $*" "
-	git diff --stat $* HEAD^ >actual &&
+	git -C '$dir' diff --stat $* HEAD^ >actual &&
 	test_i18ncmp expected actual
 "
 }
 
 check_raw() {
+dir=$1; shift
 expect=$1; shift
 cat >expected <<EOF
 :000000 100644 0000000000000000000000000000000000000000 25c05ef3639d2d270e7fe765a67668f098092bc5 A	$expect
 EOF
 test_expect_success "--raw $*" "
-	git diff --no-abbrev --raw $* HEAD^ >actual &&
+	git -C '$dir' diff --no-abbrev --raw $* HEAD^ >actual &&
 	test_cmp expected actual
 "
 }
 
 for type in diff numstat stat raw; do
-	check_$type file2 --relative=subdir/
-	check_$type file2 --relative=subdir
-	check_$type dir/file2 --relative=sub
+	check_$type . file2 --relative=subdir/
+	check_$type . file2 --relative=subdir
+	check_$type . dir/file2 --relative=sub
+	check_$type subdir file2 --relative
 done
 
 test_done
-- 
2.15.1.478.ga1e07cd25f8b
Junio C Hamano· Dec 7, 2017, 19:17 UTC · re: Jacob Keller · lore

Re: [PATCH] diff: add tests for --relative without optional prefix value

Jacob Keller <jacob.e.keller@intel.com> writes:
Show 8 quoted lines
>  for type in diff numstat stat raw; do
> -	check_$type file2 --relative=subdir/
> -	check_$type file2 --relative=subdir
> -	check_$type dir/file2 --relative=sub
> +	check_$type . file2 --relative=subdir/
> +	check_$type . file2 --relative=subdir
> +	check_$type . dir/file2 --relative=sub
> +	check_$type subdir file2 --relative

OK, I didn't think it would be sensible to unconditionally pass the directory and use "-C ." as a no-op. It looks good.

I think the new one should go before the dir/file2 test; all three earlier tests (including this new one) are about taking a patch relative to subdir/ spelled in different ways, and the one about dir/file2 alone is different.

Jacob Keller· Dec 7, 2017, 19:54 UTC · re: Junio C Hamano · lore

Re: [PATCH] diff: add tests for --relative without optional prefix value

On Thu, Dec 7, 2017 at 11:17 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 18 quoted lines
> Jacob Keller <jacob.e.keller@intel.com> writes:
>
>>  for type in diff numstat stat raw; do
>> -     check_$type file2 --relative=subdir/
>> -     check_$type file2 --relative=subdir
>> -     check_$type dir/file2 --relative=sub
>> +     check_$type . file2 --relative=subdir/
>> +     check_$type . file2 --relative=subdir
>> +     check_$type . dir/file2 --relative=sub
>> +     check_$type subdir file2 --relative
>
> OK, I didn't think it would be sensible to unconditionally pass the
> directory and use "-C ." as a no-op.  It looks good.
>
> I think the new one should go before the dir/file2 test; all three
> earlier tests (including this new one) are about taking a patch
> relative to subdir/ spelled in different ways, and the one about
> dir/file2 alone is different.

Yea, your patch looked fine to me, minus the use of subshells where we could avoid it.

I'm fine taking your version.

Thanks, Jake

Jeff King· Dec 7, 2017, 21:12 UTC · re: Jacob Keller · lore

Re: [PATCH] diff: add tests for --relative without optional prefix value

On Thu, Dec 07, 2017 at 11:01:35AM -0800, Jacob Keller wrote:
Show 9 quoted lines
> From: Jacob Keller <jacob.keller@gmail.com>
> 
> We already have tests for --relative, but they currently only test when
> a prefix has been provided. This fails to test the case where --relative
> by itself should use the current directory as the prefix.
> 
> Teach the check_$type functions to take a directory argument to indicate
> which subdirectory to run the git commands in. Add a new test which uses
> this to test --relative without a prefix value.

This looks good to me (and I slightly prefer it over Junio's for the simplicity).

I agree on the ordering suggestion Junio made.
-Peff
Jacob Keller· Dec 7, 2017, 21:20 UTC · re: Jeff King · lore

Re: [PATCH] diff: add tests for --relative without optional prefix value

On Thu, Dec 7, 2017 at 1:12 PM, Jeff King <peff@peff.net> wrote:
Show 18 quoted lines
> On Thu, Dec 07, 2017 at 11:01:35AM -0800, Jacob Keller wrote:
>
>> From: Jacob Keller <jacob.keller@gmail.com>
>>
>> We already have tests for --relative, but they currently only test when
>> a prefix has been provided. This fails to test the case where --relative
>> by itself should use the current directory as the prefix.
>>
>> Teach the check_$type functions to take a directory argument to indicate
>> which subdirectory to run the git commands in. Add a new test which uses
>> this to test --relative without a prefix value.
>
> This looks good to me (and I slightly prefer it over Junio's for the
> simplicity).
>
> I agree on the ordering suggestion Junio made.
>
> -Peff

As do I. Junio, if we go with my version, feel free to squash in the re-order. Or if you prefer, I can send a v2 (though for such a small change I don't see the benefit).

Thanks, Jake

Junio C Hamano· Dec 7, 2017, 21:50 UTC · re: Jeff King · lore

Re: [PATCH] diff: add tests for --relative without optional prefix value

Jeff King <peff@peff.net> writes:
Show 16 quoted lines
> On Thu, Dec 07, 2017 at 11:01:35AM -0800, Jacob Keller wrote:
>
>> From: Jacob Keller <jacob.keller@gmail.com>
>> 
>> We already have tests for --relative, but they currently only test when
>> a prefix has been provided. This fails to test the case where --relative
>> by itself should use the current directory as the prefix.
>> 
>> Teach the check_$type functions to take a directory argument to indicate
>> which subdirectory to run the git commands in. Add a new test which uses
>> this to test --relative without a prefix value.
>
> This looks good to me (and I slightly prefer it over Junio's for the
> simplicity).
>
> I agree on the ordering suggestion Junio made.

I also prefer this one over the other one, provided if it is ported on top of the preliminary clean-up I did in the other one.

Thanks.
Jacob Keller· Dec 7, 2017, 22:05 UTC · re: Junio C Hamano · lore

Re: [PATCH] diff: add tests for --relative without optional prefix value

On Thu, Dec 7, 2017 at 1:50 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 24 quoted lines
> Jeff King <peff@peff.net> writes:
>
>> On Thu, Dec 07, 2017 at 11:01:35AM -0800, Jacob Keller wrote:
>>
>>> From: Jacob Keller <jacob.keller@gmail.com>
>>>
>>> We already have tests for --relative, but they currently only test when
>>> a prefix has been provided. This fails to test the case where --relative
>>> by itself should use the current directory as the prefix.
>>>
>>> Teach the check_$type functions to take a directory argument to indicate
>>> which subdirectory to run the git commands in. Add a new test which uses
>>> this to test --relative without a prefix value.
>>
>> This looks good to me (and I slightly prefer it over Junio's for the
>> simplicity).
>>
>> I agree on the ordering suggestion Junio made.
>
> I also prefer this one over the other one, provided if it is ported
> on top of the preliminary clean-up I did in the other one.
>
> Thanks.
>
Yea. I can do that if you want.

Thanks, Jake

← back to recent threads