On Mon, 2010-12-20 at 10:11 +0000, Julian Foad wrote:
> On Sun, 2010-12-19 at 16:22 -0800, Danny Trebbien wrote:
> > Branko Čibej <brane <at> xbc.nu> writes:
> > > > Hi Daniel.
> > > >
> > > > I haven't looked at your patch v5 yet, because this Git diff doesn't
> > > > look quite right. How will this memcmp expression work properly if the
> > > > two strings being compared are different lengths?
> > > >> - if (translated_eol != NULL && DIFFERENT_EOL_STRINGS(eol_str, eol_str_len,
> > > >> - newline_buf, newline_len))
> > > >> + if (translated_eol != NULL &&
> > > >> + memcmp(eol_str,
> > > >> + newline_buf,
> > > >> + eol_str_len < newline_len ? eol_str_len : newline_len) != 0)
> > >
> > > Not very well, because it's prefix matching that doesn't actually take
> > > different string lengths into account and you can get false matches.
> > > e.g., if eol_str == '\r' (ancient mac mode) and newline_bug == '\r\n'
> > > ... what you want is more like:
> > >
> > > if (translated_eol != NULL &&
> > > (eol_str_len != newline_len ||
> > > memcmp(eol_str, newline_buff, eol_str_len) != 0))
> > >
> >
> > Ack! I didn't see your message until now. Yes, you are right.
>
> Hi Danny. Please could you enhance your tests so that they can detect
> this error (so that they will fail if this error is present).
Sometimes I should use more words.
What I meant to say is: I'm trying to devote as much of my time as
possible to other things (WC-NG specifically) so if you could help me
out as much as possible with reducing the review effort needed, I'd
really appreciate it. I assume your test didn't pick up this error
already, and I can't see at a glance whether you have extended the test
so that it does. If you do that, then that'll save me (and anyone now
and forever) from having to scrutinize the code to see whether it
handles this properly. That precisely what a regression test is for.
So if you already extended it, apologies as I haven't tried it or
examined it, as I wanted to ask you first.
Also please include the log message each time you attach a patch.
Please could you re-post the patch and log message together even if no
further changes are needed.
Thanks,
- Julian
Received on 2010-12-20 15:00:18 CET