Re: [PATCH v4] diff: ensure consistent diff behavior with ignore options
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 7, 2025, 21:27 UTC
- Message-ID
- <xmqqldnult4w.fsf@gitster.g>
- In-Reply-To
- <20250807020643.40213-1-yldhome2d2@gmail.com>
Lidong Yan <yldhome2d2@gmail.com> writes:
> In git-diff, options like `-w` and `-I<regex>` require comparing > file contents to determine whether two files are the same, even when > their SHA values differ.
Let's see if we can do something to clarify "the same" here. Perhaps
... two files are considered equivalent under the specified "ignore" rules, even when they are not bit-for-bit identical.
Show 7 quoted lines
> For options like `--raw`, `--name-status`, > and `--name-only`, git-diff deliberately compares only the SHA values > to determine whether two files are the same, for performance reasons. > As a result, a file shown in `git diff --name-status` may not appear > in `git diff --patch`. > > To quickly determine whether two files are identical, Add helper
Following the above, perhaps replace "identical" with "equivalent".
Also, ", Add helper" should be ", add a helper", as that comma is not finishing a sentence, hence the word that follows it is not at the beginning of the next sentence.
Show 5 quoted lines
> function diff_flush_patch_quiet() in diff.c. Add `.diff_optimize` > field in `struct diff_options`. When `.diff_optimize` is set to > `DIFF_OPT_DRY_RUN`, builtin_diff() will return immediately upon > detecting any change. Call diff_flush_patch_quiet() to determine > if we should flush `--raw`, `--name-only` or `--name-status` output.
Also the implementation details like the name of the .diff_options member and the name of the helper function have changed, and the proposed log message should be updated to match.
Show 26 quoted lines
> Signed-off-by: Jeff King <peff@peff.net>
> Signed-off-by: Lidong Yan <yldhome2d2@gmail.com>
> ---
> diff.c | 55 ++++++++++++++++++++++++++++----------
> diff.h | 2 ++
> t/t4013-diff-various.sh | 14 ++++++++++
> t/t4015-diff-whitespace.sh | 2 +-
> xdiff-interface.h | 6 ++---
> 5 files changed, 61 insertions(+), 18 deletions(-)
>
> diff --git a/diff.c b/diff.c
> index dca87e164f..3bd432db32 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -2444,6 +2444,15 @@ static int fn_out_consume(void *priv, char *line, unsigned long len)
> return 0;
> }
>
> +static int quick_consume(void *priv, char *line UNUSED, unsigned long len UNUSED)
> +{
> + struct emit_callback *ecbdata = priv;
> + struct diff_options *o = ecbdata->opt;
> +
> + o->found_changes = 1;
> + return 1;
> +}OK.
Show 5 quoted lines
> @@ -3709,6 +3718,7 @@ static void builtin_diff(const char *name_a, > xdemitconf_t xecfg; > struct emit_callback ecbdata; > const struct userdiff_funcname *pe; > + int dry_run = o->dry_run;
As the "dry_run" variable is used only once in this block, we probably do not want to add it.
Show 8 quoted lines
> if (must_show_header) {
> emit_diff_symbol(o, DIFF_SYMBOL_HEADER,
> @@ -3759,8 +3769,11 @@ static void builtin_diff(const char *name_a,
>
> if (o->word_diff)
> init_diff_words_data(&ecbdata, o, one, two);
> - if (xdi_diff_outf(&mf1, &mf2, NULL, fn_out_consume,
> - &ecbdata, &xpp, &xecfg))Instead we can check o->dry_run here.
> + if (dry_run) > + xdi_diff_outf(&mf1, &mf2, NULL, quick_consume, > + &ecbdata, &xpp, &xecfg);
We may want to leave a comment to explain why we ignore the error return from xdi_diff_outf()? Perhaps like below?
if (o->dry_run)
/*
* Unlike the !dry_run case, we need to ignore the
* return value from xdi_diff_outf() here, because
* xdi_diff_outf() takes non-zero return from its
* callback function as a sign of error and returns
* early (which is why we return non-zer from our
* callback, quick_consume()). Unfortunately,
* xdi_diff_outf() signals an error by returning
* non-zero.
*/
xdi_diff_outf(&mf1, &mf2, NULL, quick_consume,
&ecbdata, &xpp, &xecfg);I am undecided.
> + else if (xdi_diff_outf(&mf1, &mf2, NULL, fn_out_consume, > + &ecbdata, &xpp, &xecfg))
Show 5 quoted lines
> +/* return 1 if any change is found; otherwise, return 0 */
> +static int diff_flush_patch_quietly(struct diff_filepair *p, struct diff_options *o)
> +{
> + int dry_run = o->dry_run;
> + int found_changes = o->found_changes;In this codebase, these "original value of the variable X was this, we tentatively save that original value away, tweak the variable X to do something, and restore the saved value to variable X" variables are often called "saved_X".
Show 10 quoted lines
> + int ret; > + > + o->dry_run = 1; > + o->found_changes = 0; > + diff_flush_patch(p, o); > + ret = o->found_changes; > + o->dry_run = dry_run; > + o->found_changes |= found_changes; > + return ret; > +}
In the previous iteration, .dry_run/.diff_optimize was set and reset in different places; doing it in a single function here makes it easier to understand what is going on. Nice improvement.
Show 18 quoted lines
> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh > index 8ebd170451..b56a79d979 100755 > --- a/t/t4013-diff-various.sh > +++ b/t/t4013-diff-various.sh > @@ -648,6 +648,20 @@ test_expect_success 'diff -I<regex>: detect malformed regex' ' > test_grep "invalid regex given to -I: " error > ' > > +test_expect_success 'diff -I<regex>: ignore matching file' ' > + test_seq 50 >file1 && > + git add file1 && > + test_seq 50 | sed -e "s/13/ten and three/" -e "s/^[124-9].*/& /" >file1 && > + > + : >actual && > + git diff --raw --ignore-blank-lines -I"ten.*e" -I"^[124-9]" >>actual && > + git diff --name-only --ignore-blank-lines -I"ten.*e" -I"^[124-9]" >>actual && > + git diff --name-status --ignore-blank-lines -I"ten.*e" -I"^[124-9]" >>actual && > + ! grep "file1" actual &&
Perhaps use test_grep helper shell function, i.e.
test_grep ! "file1" actual &&
> + git rm -f file1
Is this because later tests will break if you leave "file1" in the working tree and/or in the index? If so, we should use test_when_finished to make such a clean-up. If you insert
test_when_finished "git rm file1; rm -f file1" &&
at the very beginning, before you create file1 with 1..50, when this test piece finishes executing (whether it completed successfully, or failed in the middle of the &&-chain), the specified command will run.
On the other hand, if the later tests won't mind whether "file1" does or does not exist in the working tree and/or in the index, it is common to leave it behind without cleaning it. When running the test script with the "-i" option, i.e.
$ sh t4013-diff-various.sh -i -v
leaving the files that were used in the test, without cleaning up, sometimes helps your debugging of the test script.
Show 11 quoted lines
> diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh > index 52e3e476ff..e7be8c5a8f 100755 > --- a/t/t4015-diff-whitespace.sh > +++ b/t/t4015-diff-whitespace.sh > @@ -11,7 +11,7 @@ test_description='Test special whitespace in diff engine. > . "$TEST_DIRECTORY"/lib-diff.sh > > for opt_res in --patch --quiet -s --stat --shortstat --dirstat=lines \ > - --raw! --name-only! --name-status! > + --raw --name-only --name-status > do
Wouldn't this make the "if the option is marked with !, tweak the test that notices these two equivalent paths are not-identical" extra code, whose beginning part we see below, unnecessary? The $expect_failure variable would always be an empty string, so "different but equivalent" test should see "git diff --exit-code" exit with status 0, right?
Other than that, looking quite good.
Thanks.