Attached is a new version of the patch (v5.1) and corresponding log
message. The only difference between this and version 5 of the patch
is that I added a new test suite,
subversion/tests/libsvn_subr/subst_translate-test, and configured it
in `build.conf`. Notes about these two changes were added to the log
message.
I have also attached the source of
`subversion/tests/libsvn_subr/subst_translate-test.c` for convenience.
> 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 &&
>> + memcmp(eol_str,
>> + newline_buf,
>> + eol_str_len < newline_len ? eol_str_len : newline_len) != 0)
I believe this to be correct because `eol_str_len < newline_len ?
eol_str_len : newline_len` is the minimum of `eol_str_len` and
`newline_len`. I don't use strncmp() here because the difference
between strncmp() and memcmp() is that strncmp() is always on the
lookout for null characters in either string, and memcmp() is not.
Also, strncmp() stops at the first difference.
> I'd like to see this code running in some test scenarios. Please could
> you see if we already have tests for the functions affected by your
> patch, and write one (or more) if not, or extend existing ones if that
> looks appropriate, to test the changes. The test code can be very
> simple - see the existing tests in
> subversion/tests/libsvn_subr/translate-test.c and
> subversion/tests/libsvn_subr/stream-test.c. Sorry to ask for more work
> when you've already done a lot, but I think then I would be able to have
> greater confidence that these changes are not breaking existing
> functionality.
>
> If this is already covered by existing tests, can you tell me which
> one(s) so I can check them.
I couldn't find a test suite that covered the svn_subst_translate_*
functions, so I wrote my own.
Received on 2010-12-17 16:55:00 CET