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

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

[Jacob Keller]

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(+)

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

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

[Jeff King]

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.

> 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

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

[Jacob Keller]

On Wed, Dec 6, 2017 at 5:04 PM, Jeff King <peff@peff.net> wrote:
> 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.

>> 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

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

[Junio C Hamano]

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

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

[Junio C Hamano]

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

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

[Jacob Keller]

On Thu, Dec 7, 2017 at 9:30 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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

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

[Jeff King]

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

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

[Junio C Hamano]

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.

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

[Jeff King]

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.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

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

[Jeff King]

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

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

[Junio C Hamano]

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t4045-diff-relative.sh | 95 +++++++++++++++++++++++++-----------------------
 1 file changed, 50 insertions(+), 45 deletions(-)

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

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

[Junio C Hamano]

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
+		) &&
 		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

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

[Jacob Keller]

On Thu, Dec 7, 2017 at 9:30 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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..?

>                 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
>