On Tue, Jun 28, 2011 at 3:57 AM, Hyrum K Wright <hy...@hyrumwright.org> wrote: > On Mon, Jun 27, 2011 at 7:25 PM, <jcor...@apache.org> wrote: >> Author: jcorvel >> Date: Tue Jun 28 00:25:57 2011 >> New Revision: 1140388 >> >> URL: http://svn.apache.org/viewvc?rev=1140388&view=rev >> Log: >> Fix a spurious failure of diff-diff3-test 2: '2-way unified diff and trivial >> merge', reported by danielsh. >> >> * subversion/libsvn_diff/diff_file.c >> (find_identical_suffix): Make sure variables had_cr and had_nl are always >> initialized. >> >> Patch by: philip >> Reported by: danielsh >> >> Modified: >> subversion/trunk/subversion/libsvn_diff/diff_file.c >> >> Modified: subversion/trunk/subversion/libsvn_diff/diff_file.c >> URL: >> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_diff/diff_file.c?rev=1140388&r1=1140387&r2=1140388&view=diff >> ============================================================================== >> --- subversion/trunk/subversion/libsvn_diff/diff_file.c (original) >> +++ subversion/trunk/subversion/libsvn_diff/diff_file.c Tue Jun 28 00:25:57 >> 2011 >> @@ -530,7 +530,7 @@ find_identical_suffix(apr_off_t *suffix_ >> int suffix_lines_to_keep = SUFFIX_LINES_TO_KEEP; >> svn_boolean_t is_match, reached_prefix; >> apr_off_t lines = 0; >> - svn_boolean_t had_cr, had_nl; >> + svn_boolean_t had_cr, had_nl = FALSE; > > Contrary to your log message, this only ensures that had_nl is > initialized: the initialization statement does not affect had_cr. For > that you would need a second initialization statement for had_cr. > > (This is one reason why many of us prefer one-variable-per-line > declaration style, rather than multiple-variables-per-line, as above.)
Oops, sorry about that. Initializing only had_nl is sufficient to fix the issue in this case (had_cr is initialized further down in the fuction). I will clean it up tonight (fix log message, and perform another commit to change to one-variable-per-line), but if someone else wants to do this: feel free. I realize that these functions (find_identical_*) are somewhat hard to 'parse' right now, because they are so big and unwieldy. I intended to improve them, reshuffle the code a bit, maybe split it up here and there, but couldn't do this yet because of lack of time. And having arrived this close to the release, I was hesitant to refactor things because I didn't want to introduce any additional risk. Though, now that these bugs are turning up, I kinda regret that ... -- Johan
