On Thu, Mar 09, 2006 at 01:58:33AM +0000, Julian Foad wrote:
> Julian Foad wrote:
> >Paul (earlier in the thread) compared the running of diff "BASE:1" with
> >"1:BASE" and found where the latter went wrong:
> >
> If I understand the meaning of "a pegged diff" correctly, it is the
> left-hand side that is wrong here. Both of these should be pegged diffs.
>
Agreed.
> > {
> > /* The 'svn diff [-r N[:M]] [TARGET[@REV]...]' case matches. */
> >
> > /* Here each target is a pegged object. Find out the starting
> > and ending paths for each target. */
> [...]
> > /* Determine if we need to do pegged diffs. */
> > if (opt_state->start_revision.kind != svn_opt_revision_base
> > && opt_state->start_revision.kind != svn_opt_revision_working)
> > pegged_diff = TRUE;
>
> That's wrong. In this section, according to the comment, we should assign
> pegged_diff=TRUE unconditionally.
>
Nope; 'svn diff foo' is handled by that case too, and that's not a pegged
diff (though it looks like later code may may it pegged @WORKING, which
may work anyway). I think it only wants to be pegged if either revision
is 'non-local'.
> I've just checked in a refactoring that removes a fair bit of duplicated
> code from libsvn_client/diff.c.
(Nice work btw! Looks good to me, though I didn't spend too long on it).
> It also fixes another bug in which "svn diff -rBASE:1 WC_PATH" generates
> the wrong output, but my test for that is not right (it passes with and
> without the patch). I have a sandbox in which I can see the effect, but am
> not sure yet exactly how to reproduce it from scratch.
>
I'm quite surprised that the existing diff_tests don't trigger whatever
you're seeing here - they're fairly comprehensive, but I guess we're
missing a particular case. I wouldn't mind knowing what the case is.
> Therefore this is not yet ready for commit, but I thought I ought to share
> it, especially as it might affect Malcolm who seems to be looking at diff
> bugs at the moment.
>
Thanks; I'm mostly working in libsvn_wc, but it's always good to know
what else is going on.
> Index: subversion/svn/diff-cmd.c
> ===================================================================
> --- subversion/svn/diff-cmd.c (revision 18780)
> +++ subversion/svn/diff-cmd.c (working copy)
> @@ -218,11 +218,7 @@ svn_cl__diff(apr_getopt_t *os,
> opt_state->end_revision.kind = working_copy_present
> ? svn_opt_revision_working : svn_opt_revision_head;
>
> - /* Determine if we need to do pegged diffs. */
> - if (opt_state->start_revision.kind != svn_opt_revision_base
> - && opt_state->start_revision.kind != svn_opt_revision_working)
> - pegged_diff = TRUE;
> -
> + pegged_diff = TRUE;
> }
>
> svn_opt_push_implicit_dot_target(targets, pool);
Like I said above, I think that this is wrong for the (implicit)
BASE:WORKING case.
How about something along the lines of:
/* Determine if we need to do pegged diffs. */
if (!((opt_state->start_revision.kind == svn_opt_revision_base
|| opt_state->start_revision.kind == svn_opt_revision_working)
&& (opt_state->end_revision.kind == svn_opt_revision_base
|| opt_state->end_revision.kind == svn_opt_revision_working)))
pegged_diff = TRUE;
> +#----------------------------------------------------------------------
> +# A base->repos diff used to output an all-lines-deleted diff
> +def diff_base_repos(sbox):
I'm not sure how the conditions being tested here are materially different
to that in e.g. diff_mime_type_changes (the BASE:1 test).
> + # Check for correct output of diff BASE:1
> + out, err = svntest.actions.run_and_verify_svn(None, SVNAnyOutput, [],
> + 'diff', '-rBASE:1')
(I prefer checking the actual expected output rather than programmatically
checking parts of it, though it does make the tests a lot longer).
Regards,
Malcolm
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Mar 9 13:14:06 2006