threads / patch / 47391

patchdiff: add test showing regression to --relative

Subject: [PATCH] diff: add test showing regression to --relative

## tl;dr

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

replies: 14people: 4as markdown or json

Jacob Keller· Dec 7, 2017, 00:35 UTC · lore
From: Jacob Keller <jacob.keller@gmail.com>
Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 t/t4045-diff-relative.sh | 5 +++++
 1 file changed, 5 insertions(+)
Show changes to t/t4045-diff-relative.sh +5 −0
diff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh
index 3950f5034d31..41e4f59b2ffb 100755
--- a/t/t4045-diff-relative.sh
+++ b/t/t4045-diff-relative.sh
@@ -70,4 +70,9 @@ for type in diff numstat stat raw; do
 	check_$type dir/file2 --relative=sub
 done
 
+cd subdir
+for type in diff numstat stat raw; do
+	check_$type file2 --relative
+done
+
 test_done
-- 
2.15.1.477.g3ed0a2a61da8
Jeff King· Dec 7, 2017, 01:04 UTC · re: Jacob Keller · lore

Re: [PATCH] diff: add test showing regression to --relative

On Wed, Dec 06, 2017 at 04:35:17PM -0800, Jacob Keller wrote:
> Subject: [PATCH] diff: add test showing regression to --relative

Since we'd hopefully not ever merge that regression, I think this patch ought to stand on its own. In which case it probably wants to say something like:

  diff: test --relative without a prefix
  We already test "diff --relative=subdir", but not that
  "--relative" by itself should use the current directory as
  its prefix.
Show 12 quoted lines
> diff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh
> index 3950f5034d31..41e4f59b2ffb 100755
> --- a/t/t4045-diff-relative.sh
> +++ b/t/t4045-diff-relative.sh
> @@ -70,4 +70,9 @@ for type in diff numstat stat raw; do
>  	check_$type dir/file2 --relative=sub
>  done
>  
> +cd subdir
> +for type in diff numstat stat raw; do
> +	check_$type file2 --relative
> +done

We should avoid moving the cwd of the whole test script in case we add tests later. Normally we'd do the cd inside a subshell, but that's complicated by the wrapper (we wouldn't want to increment the test counter just inside the subshell, for instance).

Adding "cd .." is the smallest thing we could do to fix that. But I think the more robust solution is to actually teach the check_* helper about doing the "cd" inside the test_expect block. Or just pushing the helper down into the test block and living with repeating the "test_expect_success" parts for each call.

-Peff
Jacob Keller· Dec 7, 2017, 01:21 UTC · re: Jeff King · lore

Re: [PATCH] diff: add test showing regression to --relative

On Wed, Dec 6, 2017 at 5:04 PM, Jeff King <peff@peff.net> wrote:
Show 14 quoted lines
> On Wed, Dec 06, 2017 at 04:35:17PM -0800, Jacob Keller wrote:
>
>> Subject: [PATCH] diff: add test showing regression to --relative
>
> Since we'd hopefully not ever merge that regression, I think this patch
> ought to stand on its own. In which case it probably wants to say
> something like:
>
>   diff: test --relative without a prefix
>
>   We already test "diff --relative=subdir", but not that
>   "--relative" by itself should use the current directory as
>   its prefix.
>

Yea, I wasn't sure what the actual status of the changes were though, so I thought I'd send the actual fix I did. It's definitely up to Christian in determining what he thinks the best path forward is.

Show 25 quoted lines
>> diff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh
>> index 3950f5034d31..41e4f59b2ffb 100755
>> --- a/t/t4045-diff-relative.sh
>> +++ b/t/t4045-diff-relative.sh
>> @@ -70,4 +70,9 @@ for type in diff numstat stat raw; do
>>       check_$type dir/file2 --relative=sub
>>  done
>>
>> +cd subdir
>> +for type in diff numstat stat raw; do
>> +     check_$type file2 --relative
>> +done
>
> We should avoid moving the cwd of the whole test script in
> case we add tests later. Normally we'd do the cd inside a
> subshell, but that's complicated by the wrapper (we wouldn't
> want to increment the test counter just inside the subshell,
> for instance).
>
> Adding "cd .." is the smallest thing we could do to fix
> that. But I think the more robust solution is to actually
> teach the check_* helper about doing the "cd" inside the
> test_expect block. Or just pushing the helper down into the
> test block and living with repeating the
> "test_expect_success" parts for each call.

Yea, I tried cd inside the subshell and it didn't work, so I did this as the quicker solution.

I think the best method would be to update the check_* helper functions to take a dir parameter, and use that to do git -C "dir" when calling the diff commands.

Thanks, Jake

>
> -Peff
Junio C Hamano· Dec 7, 2017, 17:30 UTC · re: Jacob Keller · lore

[PATCH v2 0/7] reroll of cc/skip-to-optional-val

I'm sending out only the last three parts, as the changes necessary to 03/07 that incorrectly used the variant without _default() to overwrite options->prefix should be trivally obvious.

05/07 uses the _default() variant so that the caller can react differently to "--relative" from "--relative=" correctly.

06/07 is an update to t4045, so that the change made in 07/07 becomes readable.

07/07 updates the test to verify the change in 05/07. I just made sure that these tests catch it if we deliberately reintroduce the broken version from Christian's series on top.

Junio C Hamano (3):
  diff: use skip-to-optional-val in parsing --relative
  t4045: reindent to make helpers readable
  t4045: test 'diff --relative' for real
 builtin/index-pack.c     |  11 ++---
 diff.c                   |  50 +++++++--------------
 git-compat-util.h        |  23 ++++++++++
 strbuf.c                 |  20 +++++++++
 t/t4045-diff-relative.sh | 111 ++++++++++++++++++++++++++++-------------------
 5 files changed, 128 insertions(+), 87 deletions(-)
-- 
2.15.1-480-gbc5668f98a
Junio C Hamano· Dec 7, 2017, 17:30 UTC · re: Junio C Hamano · lore

[PATCH v2 6/7] t4045: reindent to make helpers readable

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t4045-diff-relative.sh | 95 +++++++++++++++++++++++++-----------------------
 1 file changed, 50 insertions(+), 45 deletions(-)
Show changes to t/t4045-diff-relative.sh +50 −44
diff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh
index 3950f5034d..fefd2f3f81 100755
--- a/t/t4045-diff-relative.sh
+++ b/t/t4045-diff-relative.sh
@@ -12,59 +12,64 @@ test_expect_success 'setup' '
 	git commit -m one
 '
 
-check_diff() {
-expect=$1; shift
-cat >expected <<EOF
-diff --git a/$expect b/$expect
-new file mode 100644
-index 0000000..25c05ef
---- /dev/null
-+++ b/$expect
-@@ -0,0 +1 @@
-+other content
-EOF
-test_expect_success "-p $*" "
-	git diff -p $* HEAD^ >actual &&
-	test_cmp expected actual
-"
+check_diff () {
+	expect=$1
+	shift
+	cat >expected <<-EOF
+	diff --git a/$expect b/$expect
+	new file mode 100644
+	index 0000000..25c05ef
+	--- /dev/null
+	+++ b/$expect
+	@@ -0,0 +1 @@
+	+other content
+	EOF
+	test_expect_success "-p $*" "
+		git diff -p $* HEAD^ >actual &&
+		test_cmp expected actual
+	"
 }
 
-check_numstat() {
-expect=$1; shift
-cat >expected <<EOF
-1	0	$expect
-EOF
-test_expect_success "--numstat $*" "
-	echo '1	0	$expect' >expected &&
-	git diff --numstat $* HEAD^ >actual &&
-	test_cmp expected actual
-"
+check_numstat () {
+	expect=$1
+	shift
+	cat >expected <<-EOF
+	1	0	$expect
+	EOF
+	test_expect_success "--numstat $*" "
+		echo '1	0	$expect' >expected &&
+		git diff --numstat $* HEAD^ >actual &&
+		test_cmp expected actual
+	"
 }
 
-check_stat() {
-expect=$1; shift
-cat >expected <<EOF
- $expect | 1 +
- 1 file changed, 1 insertion(+)
-EOF
-test_expect_success "--stat $*" "
-	git diff --stat $* HEAD^ >actual &&
-	test_i18ncmp expected actual
-"
+check_stat () {
+	expect=$1
+	shift
+	cat >expected <<-EOF
+	 $expect | 1 +
+	 1 file changed, 1 insertion(+)
+	EOF
+	test_expect_success "--stat $*" "
+		git diff --stat $* HEAD^ >actual &&
+		test_i18ncmp expected actual
+	"
 }
 
-check_raw() {
-expect=$1; shift
-cat >expected <<EOF
-:000000 100644 0000000000000000000000000000000000000000 25c05ef3639d2d270e7fe765a67668f098092bc5 A	$expect
-EOF
-test_expect_success "--raw $*" "
-	git diff --no-abbrev --raw $* HEAD^ >actual &&
-	test_cmp expected actual
-"
+check_raw () {
+	expect=$1
+	shift
+	cat >expected <<-EOF
+	:000000 100644 0000000000000000000000000000000000000000 25c05ef3639d2d270e7fe765a67668f098092bc5 A	$expect
+	EOF
+	test_expect_success "--raw $*" "
+		git diff --no-abbrev --raw $* HEAD^ >actual &&
+		test_cmp expected actual
+	"
 }
 
-for type in diff numstat stat raw; do
+for type in diff numstat stat raw
+do
 	check_$type file2 --relative=subdir/
 	check_$type file2 --relative=subdir
 	check_$type dir/file2 --relative=sub
-- 
2.15.1-480-gbc5668f98a
Junio C Hamano· Dec 7, 2017, 17:30 UTC · re: Junio C Hamano · lore

[PATCH v2 7/7] t4045: test 'diff --relative' for real

The existing tests only checked how well -relative=<dir> work, without testing --relative (without any value).

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t4045-diff-relative.sh | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)
Show changes to t/t4045-diff-relative.sh +20 −4
diff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh
index fefd2f3f81..815cdd7295 100755
--- a/t/t4045-diff-relative.sh
+++ b/t/t4045-diff-relative.sh
@@ -25,7 +25,10 @@ check_diff () {
 	+other content
 	EOF
 	test_expect_success "-p $*" "
-		git diff -p $* HEAD^ >actual &&
+		(
+			test -z "$in_there" || cd "$in_there"
+			git diff -p $* HEAD^
+		) >actual &&
 		test_cmp expected actual
 	"
 }
@@ -38,7 +41,10 @@ check_numstat () {
 	EOF
 	test_expect_success "--numstat $*" "
 		echo '1	0	$expect' >expected &&
-		git diff --numstat $* HEAD^ >actual &&
+		(
+			test -z "$in_there" || cd "$in_there"
+			git diff --numstat $* HEAD^
+		) >actual &&
 		test_cmp expected actual
 	"
 }
@@ -51,7 +57,10 @@ check_stat () {
 	 1 file changed, 1 insertion(+)
 	EOF
 	test_expect_success "--stat $*" "
-		git diff --stat $* HEAD^ >actual &&
+		(
+			test -z "$in_there" || cd "$in_there"
+			git diff --stat $* HEAD^
+		) >actual &&
 		test_i18ncmp expected actual
 	"
 }
@@ -63,15 +72,22 @@ check_raw () {
 	:000000 100644 0000000000000000000000000000000000000000 25c05ef3639d2d270e7fe765a67668f098092bc5 A	$expect
 	EOF
 	test_expect_success "--raw $*" "
-		git diff --no-abbrev --raw $* HEAD^ >actual &&
+		(
+			test -z "$in_there" || cd "$in_there"
+			git diff --no-abbrev --raw $* HEAD^ >actual
+		) &&
 		test_cmp expected actual
 	"
 }
 
 for type in diff numstat stat raw
 do
+	in_there=
 	check_$type file2 --relative=subdir/
 	check_$type file2 --relative=subdir
+	in_there=subdir
+	check_$type file2 --relative
+	in_there=
 	check_$type dir/file2 --relative=sub
 done
 
-- 
2.15.1-480-gbc5668f98a
Jacob Keller· Dec 7, 2017, 19:06 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 7/7] t4045: test 'diff --relative' for real

On Thu, Dec 7, 2017 at 9:30 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 57 quoted lines
> The existing tests only checked how well -relative=<dir> work,
> without testing --relative (without any value).
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  t/t4045-diff-relative.sh | 24 ++++++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh
> index fefd2f3f81..815cdd7295 100755
> --- a/t/t4045-diff-relative.sh
> +++ b/t/t4045-diff-relative.sh
> @@ -25,7 +25,10 @@ check_diff () {
>         +other content
>         EOF
>         test_expect_success "-p $*" "
> -               git diff -p $* HEAD^ >actual &&
> +               (
> +                       test -z "$in_there" || cd "$in_there"
> +                       git diff -p $* HEAD^
> +               ) >actual &&
>                 test_cmp expected actual
>         "
>  }
> @@ -38,7 +41,10 @@ check_numstat () {
>         EOF
>         test_expect_success "--numstat $*" "
>                 echo '1 0       $expect' >expected &&
> -               git diff --numstat $* HEAD^ >actual &&
> +               (
> +                       test -z "$in_there" || cd "$in_there"
> +                       git diff --numstat $* HEAD^
> +               ) >actual &&
>                 test_cmp expected actual
>         "
>  }
> @@ -51,7 +57,10 @@ check_stat () {
>          1 file changed, 1 insertion(+)
>         EOF
>         test_expect_success "--stat $*" "
> -               git diff --stat $* HEAD^ >actual &&
> +               (
> +                       test -z "$in_there" || cd "$in_there"
> +                       git diff --stat $* HEAD^
> +               ) >actual &&
>                 test_i18ncmp expected actual
>         "
>  }
> @@ -63,15 +72,22 @@ check_raw () {
>         :000000 100644 0000000000000000000000000000000000000000 25c05ef3639d2d270e7fe765a67668f098092bc5 A      $expect
>         EOF
>         test_expect_success "--raw $*" "
> -               git diff --no-abbrev --raw $* HEAD^ >actual &&
> +               (
> +                       test -z "$in_there" || cd "$in_there"
> +                       git diff --no-abbrev --raw $* HEAD^ >actual
> +               ) &&

You could avoid the subshell by just passing $in_there to -C on the git commands. Same for the other tests. If you quote it, -C '$in_there', then it will work regardless of if in_there is set or not, (-C with an empty string doesn't cd anywhere). I think this is generally preferable for tests given it helps avoid unnecessary subshells when testing on Windows..?

Show 15 quoted lines
>                 test_cmp expected actual
>         "
>  }
>
>  for type in diff numstat stat raw
>  do
> +       in_there=
>         check_$type file2 --relative=subdir/
>         check_$type file2 --relative=subdir
> +       in_there=subdir
> +       check_$type file2 --relative
> +       in_there=
>         check_$type dir/file2 --relative=sub
>  done
>

This isn't quite what I had in mind for the directory parameter. I passed it as an extra argument, but I think this is probably more sensible.

> --
> 2.15.1-480-gbc5668f98a
>
Junio C Hamano· Dec 7, 2017, 17:30 UTC · re: Junio C Hamano · lore

[PATCH v2 5/7] diff: use skip-to-optional-val in parsing --relative

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)
Show changes to diff.c +3 −4
diff --git a/diff.c b/diff.c
index cd032c6367..e99ac6ec8a 100644
--- a/diff.c
+++ b/diff.c
@@ -4563,11 +4563,10 @@ int diff_opt_parse(struct diff_options *options,
 		options->flags.rename_empty = 1;
 	else if (!strcmp(arg, "--no-rename-empty"))
 		options->flags.rename_empty = 0;
-	else if (!strcmp(arg, "--relative"))
+	else if (skip_to_optional_val_default(arg, "--relative", &arg, NULL)) {
 		options->flags.relative_name = 1;
-	else if (skip_prefix(arg, "--relative=", &arg)) {
-		options->flags.relative_name = 1;
-		options->prefix = arg;
+		if (arg)
+			options->prefix = arg;
 	}
 
 	/* xdiff options */
-- 
2.15.1-480-gbc5668f98a
Jacob Keller· Dec 7, 2017, 19:03 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 5/7] diff: use skip-to-optional-val in parsing --relative

On Thu, Dec 7, 2017 at 9:30 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 27 quoted lines
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  diff.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/diff.c b/diff.c
> index cd032c6367..e99ac6ec8a 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -4563,11 +4563,10 @@ int diff_opt_parse(struct diff_options *options,
>                 options->flags.rename_empty = 1;
>         else if (!strcmp(arg, "--no-rename-empty"))
>                 options->flags.rename_empty = 0;
> -       else if (!strcmp(arg, "--relative"))
> +       else if (skip_to_optional_val_default(arg, "--relative", &arg, NULL)) {
>                 options->flags.relative_name = 1;
> -       else if (skip_prefix(arg, "--relative=", &arg)) {
> -               options->flags.relative_name = 1;
> -               options->prefix = arg;
> +               if (arg)
> +                       options->prefix = arg;
>         }
>
>         /* xdiff options */
> --
> 2.15.1-480-gbc5668f98a
>
Yea, this is exactly what I had imagined as the fix.

Thanks, Jake

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

Re: [PATCH v2 5/7] diff: use skip-to-optional-val in parsing --relative

On Thu, Dec 07, 2017 at 09:30:32AM -0800, Junio C Hamano wrote:
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---

It might be worth mentioning why this conversion is pulled out from the others (because its "default" case is "do not touch the pointer").

Other than that, it looks good to me.
-Peff
Junio C Hamano· Dec 7, 2017, 21:59 UTC · re: Jeff King · lore

Re: [PATCH v2 5/7] diff: use skip-to-optional-val in parsing --relative

Jeff King <peff@peff.net> writes:
Show 7 quoted lines
> On Thu, Dec 07, 2017 at 09:30:32AM -0800, Junio C Hamano wrote:
>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>
> It might be worth mentioning why this conversion is pulled out from the
> others (because its "default" case is "do not touch the pointer").

I am not sure what you mean by "pulled out from the others". I did not intend to keep these 3 additional patches permanently; rather, I did them to help Christian's rerolling the series, and I do not think this one should be separate from other ones that use the _default() variant when that happens.

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

Re: [PATCH v2 5/7] diff: use skip-to-optional-val in parsing --relative

On Thu, Dec 07, 2017 at 01:59:39PM -0800, Junio C Hamano wrote:
Show 15 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > On Thu, Dec 07, 2017 at 09:30:32AM -0800, Junio C Hamano wrote:
> >
> >> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> >> ---
> >
> > It might be worth mentioning why this conversion is pulled out from the
> > others (because its "default" case is "do not touch the pointer").
> 
> I am not sure what you mean by "pulled out from the others".  I did
> not intend to keep these 3 additional patches permanently; rather, I
> did them to help Christian's rerolling the series, and I do not think
> this one should be separate from other ones that use the _default()
> variant when that happens.
Ah, I see. I had thought you meant these to be applied on top.
-Peff
Junio C Hamano· Dec 7, 2017, 22:31 UTC · re: Jeff King · lore

Re: [PATCH v2 5/7] diff: use skip-to-optional-val in parsing --relative

Jeff King <peff@peff.net> writes:
Show 19 quoted lines
> On Thu, Dec 07, 2017 at 01:59:39PM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > On Thu, Dec 07, 2017 at 09:30:32AM -0800, Junio C Hamano wrote:
>> >
>> >> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> >> ---
>> >
>> > It might be worth mentioning why this conversion is pulled out from the
>> > others (because its "default" case is "do not touch the pointer").
>> 
>> I am not sure what you mean by "pulled out from the others".  I did
>> not intend to keep these 3 additional patches permanently; rather, I
>> did them to help Christian's rerolling the series, and I do not think
>> this one should be separate from other ones that use the _default()
>> variant when that happens.
>
> Ah, I see. I had thought you meant these to be applied on top.
Ah, I do not particularly mind doing things incrementally, either.

If this goes on top as a standalone patch, then the reason why it is separate from the other users of _default() is not because the way it uses the null return is special, but because it was written by a different author, I would think.

The reason why _default() variant exists is because its callers want to react differently to "--foo" from the way they react to "--foo=" (with an empty string as its value), and from that point of view, this caller is not all that different from other ones, like the one that parses --color-words Christian wrote in his initial round.

Jeff King· Dec 8, 2017, 09:05 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 5/7] diff: use skip-to-optional-val in parsing --relative

On Thu, Dec 07, 2017 at 02:31:38PM -0800, Junio C Hamano wrote:
> If this goes on top as a standalone patch, then the reason why it is
> separate from the other users of _default() is not because the way
> it uses the null return is special, but because it was written by a
> different author, I would think.

Mostly I was just concerned that we missed a somewhat subtle bug in the first conversion, and I think it's worth calling out in the commit message why that callsite must be converted in a particular way. Whether that happens in a separate commit or squashed, I don't care too much.

But I dunno. Maybe it is obvious now that the correct code is in there. ;)

-Peff
Junio C Hamano· Dec 8, 2017, 21:09 UTC · re: Jeff King · lore

Re: [PATCH v2 5/7] diff: use skip-to-optional-val in parsing --relative

Jeff King <peff@peff.net> writes:
Show 14 quoted lines
> On Thu, Dec 07, 2017 at 02:31:38PM -0800, Junio C Hamano wrote:
>
>> If this goes on top as a standalone patch, then the reason why it is
>> separate from the other users of _default() is not because the way
>> it uses the null return is special, but because it was written by a
>> different author, I would think.
>
> Mostly I was just concerned that we missed a somewhat subtle bug in the
> first conversion, and I think it's worth calling out in the commit
> message why that callsite must be converted in a particular way. Whether
> that happens in a separate commit or squashed, I don't care too much.
>
> But I dunno. Maybe it is obvious now that the correct code is in there.
> ;)

It probably is too obvious to me only because the use case like this one that wanted to treat --foo and --foo="" differently was the only reason why I pushed against Christian's original one that hardcoded the equivalence without allowing what the _default() variant lets us do, I think.

← back to recent threads