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 tr...@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
