diff: add test showing regression to --relative
PATCH 1/1 diff: add test showing regression to --relative
From: Jacob Keller <[email protected]>
--- t/t4045-diff-relative.sh | 5 +++++ 1 file changed, 5 insertions(+)
t/t4045-diff-relative.sh +5 -0
@@ -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
Signature · 1 line
2.15.1.477.g3ed0a2a61da8
Quoted 1 line · Jacob Keller
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.
t/t4045-diff-relative.sh for type in diff numstat stat raw; do · line 76
Quoted 12 lines
t/t4045-diff-relative.sh +4
-0@@ -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
Quoted 8 lines · Jeff King
Quoted 1 line · Jacob Keller
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:
Quoted 6 lines
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.
Quoted 19 lines
t/t4045-diff-relative.sh for type in diff numstat stat raw; do · line 76
Quoted 12 lines
t/t4045-diff-relative.sh +4
-0@@ -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 +doneWe 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).
Quoted 6 lines
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_expectblock. 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
Quoted 2 lines
-Peff
v2 cover reroll of cc/skip-to-optional-val
In reply to Jacob KellerI'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(-)
Signature · 1 line
2.15.1-480-gbc5668f98a
PATCH v2 5/7 diff: use skip-to-optional-val in parsing --relative
--- diff.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
diff.c +3 -4
@@ -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 */
Signature · 1 line
2.15.1-480-gbc5668f98a
Re: [PATCH v2 5/7] diff: use skip-to-optional-val in parsing --relative
Quoted 25 lines · Junio C Hamano
Signed-off-by Junio C Hamano <[email protected]>--- diff.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)diff.c +3
-5@@ -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 */ --
Quoted 2 lines
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
In reply to Junio C HamanoQuoted 2 lines · Junio C Hamano
Signed-off-by Junio C Hamano <[email protected]>---
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
Quoted 7 lines · Jeff King
Quoted 2 lines · Junio C Hamano
Signed-off-by Junio C Hamano <[email protected]>---
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
Quoted 7 lines · Junio C Hamano
Quoted 5 lines · Jeff King
Quoted 2 lines · Junio C Hamano
Signed-off-by Junio C Hamano <[email protected]>---
Quoted 8 lines
Quoted 2 lines
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
Quoted 12 lines · Jeff King
Quoted 10 lines · Junio C Hamano
Quoted 7 lines · Jeff King
Quoted 2 lines · Junio C Hamano
Signed-off-by Junio C Hamano <[email protected]>---
It might be worth mentioning why this conversion is pulled out from the others (because its "default" case is "do not touch the pointer").
Quoted 7 lines
Quoted 5 lines
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
Quoted 4 lines · Junio C Hamano
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
Quoted 7 lines · Jeff King
Quoted 4 lines · Junio C Hamano
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.
Quoted 7 lines
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
In reply to Junio C Hamano--- t/t4045-diff-relative.sh | 95 +++++++++++++++++++++++++----------------------- 1 file changed, 50 insertions(+), 45 deletions(-)
t/t4045-diff-relative.sh +50 -45
@@ -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
Signature · 1 line
2.15.1-480-gbc5668f98a
PATCH v2 7/7 t4045: test 'diff --relative' for real
In reply to Junio C HamanoThe existing tests only checked how well -relative=<dir> work, without testing --relative (without any value).
--- t/t4045-diff-relative.sh | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-)
t/t4045-diff-relative.sh +20 -4
@@ -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
Signature · 1 line
2.15.1-480-gbc5668f98a
Re: [PATCH v2 7/7] t4045: test 'diff --relative' for real
Quoted 49 lines · 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 <[email protected]>--- t/t4045-diff-relative.sh | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-)t/t4045-diff-relative.sh +12
-3@@ -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 " }
t/t4045-diff-relative.sh check_raw () { · line 78
Quoted 8 lines
t/t4045-diff-relative.sh +4
-1@@ -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..?
Quoted 5 lines
t/t4045-diff-relative.sh +0
-0test_cmp expected actual " } for type in diff numstat stat raw
t/t4045-diff-relative.sh check_raw () { · line 93
Quoted 10 lines
t/t4045-diff-relative.sh +4
-0do + 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.
Quoted 3 lines
--
2.15.1-480-gbc5668f98a