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

Michał Kępień [off-list ref] writes:

quoted

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

quoted

quoted

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

quoted

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.

quoted

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.