Re: [PATCH v4] diff: ensure consistent diff behavior with ignore options

[Lidong Yan <yldhome2d2@gmail.com>]

Junio C Hamano <gitster@pobox.com> writes:
> 
> 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.

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