Junio C Hamano [off-list ref] writes:

Lidong Yan [off-list ref] writes:

quoted

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.

Following the above, perhaps replace "identical" with "equivalent".

Equivalent seems to be a good definition, will replace.

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.

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.

Will fix grammar errors and update log message.

As the "dry_run" variable is used only once in this block, we
probably do not want to add it.

Sure, will fix.

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 think your comment is good enough so I will just copy it.

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

Will rename.

Perhaps use test_grep helper shell function, i.e.

test_grep ! "file1" actual &&

I guess this is because test_grep has more useful debugging information.
Will replace.

quoted

+ 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

Yes, exactly.

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.

Early on Patrict suggested the same thing, I will read test_when_finish
to see how it works.

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

Good thing to know. Just curious, I also found that running ./txxx -v causes
failed results and successful results to be mixed together. Do you usually
solve this by using awk to extract the error messages?

quoted

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?

Yes, I will remove the optionally set expect_failure part.

Thank you very much for your review,
Lidong