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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 7, 2020, 20:08 UTC
Message-ID: <xmqqft6p240j.fsf@gitster.c.googlers.com>
In-Reply-To: <20201007194821.GA20549@larwa.hq.kempniu.pl>

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

Show 10 quoted lines

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

Show 24 quoted lines

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

Show 6 quoted lines

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