On Mon, Dec 31, 2012 at 7:07 PM, <rhuijben_at_apache.org> wrote:
> Author: rhuijben
> Date: Mon Dec 31 18:07:29 2012
> New Revision: 1427210
>
> URL: http://svn.apache.org/viewvc?rev=1427210&view=rev
> Log:
> Following up on r1427197, apply a similar fix to the suffix scanning. Avoid
> updating variables until they are known to be correct to allow breaking out
> of the loop. And reset state variables after detecting a common suffix.
>
> Just like r1427197, the only behavior change should be in resetting the
> state variables.
Great! Nice fix (seems issue #4270 is fixed with this revision,
together with r1427197). Maybe both of these revisions should be
nominated for backport? The bug has the potential to generate a wrong
diff, so if it happens with diff3 this could also cause an incorrect
result from update or merge.
Just a small nit below...
> * subversion/libsvn_diff/diff_file.c
> (find_identical_suffix): Update loops and loop conditions to allow resetting
> state variables.
>
> 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=1427210&r1=1427209&r2=1427210&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_diff/diff_file.c (original)
> +++ subversion/trunk/subversion/libsvn_diff/diff_file.c Mon Dec 31 18:07:29 2012
> @@ -628,32 +628,41 @@ find_identical_suffix(apr_off_t *suffix_
> can_read_word = can_read_word
> && ( file_for_suffix[i].curp - sizeof(apr_uintptr_t)
> >= min_curp[i]);
> - if (can_read_word)
> + while (can_read_word)
> {
> - do
> + apr_uintptr_t chunk;
> +
> + /* For each file curp is positioned at the next byte to read, but we
> + want to examine the bytes before the current location. */
> +
> + chunk = *(const apr_uintptr_t *)(file_for_suffix[0].curp + 1
> + - sizeof(apr_uintptr_t));
> + if (contains_eol(chunk))
> + break;
> +
> + for (i = 1, is_match = TRUE; i < file_len; i++)
> + is_match = is_match
> + && ( chunk
> + == *(const apr_uintptr_t *)
> + (file_for_suffix[i].curp + 1
> + - sizeof(apr_uintptr_t)));
> +
> + if (! is_match)
> + break;
> +
> + for (i = 0; i < file_len; i++)
> {
> - apr_uintptr_t chunk;
> - for (i = 0; i < file_len; i++)
> - file_for_suffix[i].curp -= sizeof(apr_uintptr_t);
> -
> - chunk = *(const apr_uintptr_t *)(file_for_suffix[0].curp + 1);
> - if (contains_eol(chunk))
> - break;
> -
> - for (i = 0, can_read_word = TRUE; i < file_len; i++)
> - can_read_word = can_read_word
> - && ( file_for_suffix[i].curp - sizeof(apr_uintptr_t)
> - >= min_curp[i]);
> - for (i = 1, is_match = TRUE; i < file_len; i++)
> - is_match = is_match
> - && ( chunk
> - == *(const apr_uintptr_t *)(file_for_suffix[i].curp + 1));
> - } while (can_read_word && is_match);
> + file_for_suffix[i].curp -= sizeof(apr_uintptr_t);
> + can_read_word = can_read_word
> + && ( (file_for_suffix[i].curp
> + - sizeof(apr_uintptr_t))
> + >= min_curp[i]);
> + }
>
> - for (i = 0; i < file_len; i++)
> - file_for_suffix[i].curp += sizeof(apr_uintptr_t);
> + /* We skipped 4 bytes, so there are no closing EOLs */
> + had_nl = FALSE;
> + had_cr = FALSE;
had_cr isn't actually used in this function yet, so it doesn't have to
be (re)set. It's unconditionally set to FALSE a couple of lines later,
when it's used in the do-while loop that has the comment "Slide
forward until we find an eol sequence ...".
(I know ... I should have separated this large function in a couple of
smaller ones so the variables could be better scoped, to avoid this
confusion ... but neglected to do this)
> }
> -
> #endif
>
> reached_prefix = file_for_suffix[0].chunk == suffix_min_chunk0
>
>
--
Johan
Received on 2013-01-15 01:22:32 CET