Junio C Hamano <gitster@xxxxxxxxx> writes: > > Lidong Yan <yldhome2d2@xxxxxxxxx> 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. > > 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. > >> + 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? > >> 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
