Re: [PATCH 1/2] diff: add -I<regex> that ignores matching changes

[Junio C Hamano <gitster@pobox.com>]

Michał Kępień <michal@isc.org> writes:

>> cannot be memset(0) before use?  It seems like a much better
>> approach to ensure that we clear the structure.
>
> I do not know of any reason for xpparam_t structures to not be
> zero-initialized.  In fact, they _are_ zero-initialized most of the
> time; AFAICT, there are just three places in the tree in which they are
> not.
>
> Would you like me to address that in a separate patch in this patch
> series?

Yes, as a preliminary clean-up patch before the real/fun stuff ;-)

>> > +-I<regex>::
>> > +	Ignore changes whose all lines match <regex>.
>> > +
>> 
>> The implementation seems to allow only one regex, but I suspect we'd
>> want to mimic
>> 
>>     $ printf "%s\n" a a a >test_a
>>     $ printf "%s\n" b b b >test_b
>>     $ diff -Ia     test_a test_b
>>     $ diff     -Ib test_a test_b
>>     $ diff -Ia -Ib test_a test_b
>
> Ah, sure.  After skimming through various man pages available online, I
> was not sure whether all standalone versions of diff which support -I
> allow that switch to be used multiple times and I thought it would be
> better to start with the simplest thing possible.  If you would rather
> have the above implemented immediately, I can sure try that in v2.
>
>> and until that happens, the explanation needs to say something about
>> earlier <regex> being discarded when this option is given more than
>> once.
>
> Sorry, where do I look for that explanation? 

You wrote "Ignore changes whose all lines match <regex>"; I was
suggesting that we also need "when given more than once, all but the
last <regex> are ignored" after that sentence, because that is not
what people who know how -I works in versions of "diff" that support
it expect.

But I think it should be trivial to make it a list of patterns and
try to match against them in a loop.  So let's support multiple
patterns from the get-go.

> I have not thought about this approach before, but it sounds elegant to
> me, at least at first glance - option parsing code sounds like the right
> place for sanitizing user-provided parameters.  Doubly so if we take the
> multiple -I options approach.  I will try this in v2.

Sounds good.

>> I agree with what you said in the cover letter that it would be a
>> good idea to name the existing xdl_mark_ignorable() and the new one
>> in such a way that they look similar and parallel, by renaming the
>> former to xdl_mark_ignorable_lines or something.
>
> Then I will do that in v2.  Separate patch?

Given that the function has only one caller, I think it is better
done within the same patch.  xdl_mark_ignorable() as the name of the
function is not wrong per-se, so it does not deserve to be renamed
standalone in a preliminary clean-up patch---there is nothing to be
cleaned up.  The fact that we introduce a similarly tasked function
makes its current name less desirable, so adjusting to the new world
order is better done as part of the same patch.

> My pleasure ;-)  And thanks for taking a look.  Sorry about the long
> turnaround time, but unfortunately this is the best I can do at the
> moment.

Pleasure is mutual ;-)

We've survived without -I for 15 years.  Even a few more months
would not hurt anybody.  Take time, aim for a quality job, and most
importantly, have fun.

Thanks.