Michał Kępień <michal@xxxxxxx> 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.
