| Message ID | 8a8bd51e-9ce5-4a68-bfbe-f16dcbb7e89c@web.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | diff: allow --color-moved with --no-ext-diff | expand |
Hello René, using red/green coloration is the current buggy behavior. When setting oldMOved and newMoved I would expect the correct behaviour to the the same as the buggy one. Could this test pass despite the bug?
Am 23.06.24 um 11:17 schrieb lolligerhans@gmx.de: > using red/green coloration is the current buggy behavior. When > setting oldMOved and newMoved I would expect the correct behaviour to > the the same as the buggy one. > > Could this test pass despite the bug? Good question, but it doesn't. The test uses the colors "normal red" and "normal green" for moved lines, i.e. unchanged text color on red and green background, while regular changed lines use red and green text on unchanged background. René
René Scharfe <l.s.r@web.de> writes: > diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh > index b443626afd..a1478680b6 100755 > --- a/t/t4015-diff-whitespace.sh > +++ b/t/t4015-diff-whitespace.sh > @@ -1184,6 +1184,15 @@ test_expect_success 'detect moved code, complete file' ' > test_cmp expected actual > ' > > +test_expect_success '--color-moved with --no-ext-diff' ' > + test_config color.diff.oldMoved "normal red" && > + test_config color.diff.newMoved "normal green" && We are making sure we won't be affected by previous tests. We assume that we did not set color.diff.{old,new} to these two colors, but that would be an OK assumption to make. > + cp actual.raw expect && But then this introduces a dependence to an earlier _specific_ test, the one that created this version (among three) of actual.raw; If we did this instead git diff --color --color-moved=zebra --no-renames HEAD >expect && it would make this a lot more self-contained. > + git -c diff.external=false diff HEAD --no-ext-diff \ > + --color-moved=zebra --color --no-renames >actual && Also, please do stick to the normal CLI ocnvention, dashed options come before the revs, i.e. git -c diff.external=false diff --no-ext-diff --color \ --color-moved=zebra --no-renames HEAD >actual && Our tests shouldn't be setting a wrong example. > + test_cmp expect actual > +' > + > test_expect_success 'detect malicious moved code, inside file' ' > test_config color.diff.oldMoved "normal red" && > test_config color.diff.newMoved "normal green" && Other than that, looking very good. Thanks.
Am 24.06.24 um 18:21 schrieb Junio C Hamano: > René Scharfe <l.s.r@web.de> writes: > >> diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh >> index b443626afd..a1478680b6 100755 >> --- a/t/t4015-diff-whitespace.sh >> +++ b/t/t4015-diff-whitespace.sh >> @@ -1184,6 +1184,15 @@ test_expect_success 'detect moved code, complete file' ' >> test_cmp expected actual >> ' >> >> +test_expect_success '--color-moved with --no-ext-diff' ' >> + test_config color.diff.oldMoved "normal red" && >> + test_config color.diff.newMoved "normal green" && > > We are making sure we won't be affected by previous tests. We > assume that we did not set color.diff.{old,new} to these two colors, > but that would be an OK assumption to make. The previous test also uses test_config to set these values, which means they get cleared at its end. We need to set them again to reuse the actual.raw file. >> + cp actual.raw expect && > > But then this introduces a dependence to an earlier _specific_ test, > the one that created this version (among three) of actual.raw;> > If we did this instead > > git diff --color --color-moved=zebra --no-renames HEAD >expect && > > it would make this a lot more self-contained. Oh, yes, good idea! We'd still rely on there being staged differences that include moved lines, >> + git -c diff.external=false diff HEAD --no-ext-diff \ >> + --color-moved=zebra --color --no-renames >actual && > > Also, please do stick to the normal CLI ocnvention, dashed options > come before the revs, i.e. > > git -c diff.external=false diff --no-ext-diff --color \ > --color-moved=zebra --no-renames HEAD >actual && > > Our tests shouldn't be setting a wrong example. I mimicked the style of the previous test to hint that we do the same here, just with the diff.external/--no-ext-diff noop on top. Not close enough, perhaps, but also hard to spot with 40+ lines between them. René
René Scharfe <l.s.r@web.de> writes: >> If we did this instead >> >> git diff --color --color-moved=zebra --no-renames HEAD >expect && >> >> it would make this a lot more self-contained. > > Oh, yes, good idea! We'd still rely on there being staged differences > that include moved lines, Yeah, the primary point of this fix is that the output with "--no-ext-diff" and configured external diff driver, and without either, should be the same. And running the same command twice, once with and once without these two tweaks, and comparing their results is exactly what we want to do.
diff --git a/diff.c b/diff.c index 6e432cb8fc..aa0fb77761 100644 --- a/diff.c +++ b/diff.c @@ -4965,7 +4965,8 @@ void diff_setup_done(struct diff_options *options) if (options->flags.follow_renames) diff_check_follow_pathspec(&options->pathspec, 1); - if (!options->use_color || external_diff()) + if (!options->use_color || + (options->flags.allow_external && external_diff())) options->color_moved = 0; if (options->filter_not) { diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh index b443626afd..a1478680b6 100755 --- a/t/t4015-diff-whitespace.sh +++ b/t/t4015-diff-whitespace.sh @@ -1184,6 +1184,15 @@ test_expect_success 'detect moved code, complete file' ' test_cmp expected actual ' +test_expect_success '--color-moved with --no-ext-diff' ' + test_config color.diff.oldMoved "normal red" && + test_config color.diff.newMoved "normal green" && + cp actual.raw expect && + git -c diff.external=false diff HEAD --no-ext-diff \ + --color-moved=zebra --color --no-renames >actual && + test_cmp expect actual +' + test_expect_success 'detect malicious moved code, inside file' ' test_config color.diff.oldMoved "normal red" && test_config color.diff.newMoved "normal green" &&
Since the option --color-moved was added by 2e2d5ac184 (diff.c: color moved lines differently, 2017-06-30) we have been ignoring it if an external diff program is configured, presumably because its overhead is unnecessary in that case. Do respect --color-moved if we don't actually use the configured external diff, though. Reported-by: lolligerhans@gmx.de Signed-off-by: René Scharfe <l.s.r@web.de> --- diff.c | 3 ++- t/t4015-diff-whitespace.sh | 9 +++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) -- 2.45.2