On Fri, 2010-12-10, Danny Trebbien wrote:
> > Thanks, Danny.
> >
> > This looks fine.
> >
> > I added doc strings to the new static functions stream_translated() and
> > translate_cstring(). I clarified in all doc strings whether
> > TRANSLATED_EOL is set to FALSE or untouched if there is no translation.
> > (In some cases you documented these more in the log message than in the
> > code :-)
> >
> > Just about to commit this ... FAIL: autoprop_tests.py 15, blame_tests.py
> > 7, commit_tests.py 40, ...
> >
> > subst.c' line 668: assertion failed (STRING_IS_EOL(eol_str,
> > eol_str_len))
> >
> > Whassup? Attaching my version, as I'm out of time tonight. Hope I
> > didn't mess it up. I was testing trunk_at_1042741 (patched as attached),
> > svnserve and FSFS, in case it matters.
> >
> > Cheers,
> > - Julian
>
> Attached is version 5 of the patch. It incorporates your changes,
> Julian, as well as a fix for the problem that was causing several of
> the tests to fail.
>
> In investigating why one of the autoprop tests failed, I discovered
> that the EOL_STR was an empty string rather than one of the standard
> end-of-line strings. I didn't expect this. However, tracing the source
> of EOL_STR backward, I realized that EOL_STR can be anything because
> some of the public libsvn_subr routines pass a C-string parameter
> directly to the EOL_STR field of the translation baton. I therefore
> changed the code slightly so that translate_newline() does not rely on
> EOL_STR being a standard end-of-line string:
> https://github.com/dtrebbien/subversion/compare/eb3fb5c5b6...75966428dc
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)
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.
Thanks.
- Julian
Received on 2010-12-13 12:39:34 CET