On Fri, Jul 09, 2010 at 12:36:35PM +0100, Julian Foad wrote:
> On Fri, 2010-07-09 at 09:52 +0200, Daniel Näslund wrote:
> > Hi!
> >
> > The diff callbacks for handling text have revision arguments. The prop
> > callbacks don't.
> >
> > I'm creating diff headers for the cases when we only have property
> > changes. When creating those diff headers I need revisions. Since the
> > prop diff callback doesn't have revision arguments I'm using the
> > revision fields in struct diff_cmd_baton. Here's what the doc string says
> > about those fields.
> >
> > [[[
> > /* These are the numeric representations of the revisions passed to
> > svn_client_diff5, either may be SVN_INVALID_REVNUM. We need these
> > because some of the svn_wc_diff_callbacks4_t don't get revision
> > arguments.
> >
> > ### Perhaps we should change the callback signatures and eliminate
> > ### these?
> > */
> > svn_revnum_t revnum1;
> > svn_revnum_t revnum2;
> > ]]]
> >
> > But they don't match the revisions returned by the diff callbacks.
> > Here's the output from diff_content_changed() comparing what the baton
> > fields says and what the revision argument says (from diff_tests 32):
> >
> > $ svn di -r 1 | grep DBG
> >
> > DBG: diff.c: 692: btn->rev1 1, rev1 0, btn->revn2 -1, rev2 3
> > DBG: diff.c: 692: btn->rev1 1, rev1 0, btn->revn2 -1, rev2 3
What this output actually refers to is two added files, thus the start
revision is set to 0. That kinda makes sense. But for some reason the
diff callback gets called with rev2 set to the actual revision of the
path instead of SVN_INVALID_REVNUM (meaning we're referring to the
working copy revision).
An excerpt from libsvn_wc/diff.c::file_diff():
[[[
/* A wc-wc diff of replaced files actually shows a diff against the
* revert-base, showing all previous lines as removed and adding all new
* lines. This does not happen for copied/moved-here files, not even
* with show_copies_as_adds == TRUE (in which case copy/move is really
* shown as an add, diffing against the empty file). So show the
* revert-base revision for plain replaces. */
if (replaced
&& ! (status == svn_wc__db_status_copied
|| status == svn_wc__db_status_moved_here))
revision = revert_base_revnum;
]]]
Appearantly we're adjusting the revision for the case of replaced files.
Still we could return SVN_INVALID_REVNUM for all the rest since this
part of the code only deals with diffs against the WC.
> For rev2, it looks like the caller is providing SVN_INVALID_REVNUM to
> mean HEAD, and then something is discovering the current value of HEAD
> is 3, but is not updating "btn->revn2" with that information. I think,
> if possible, the code that discovers the actual HEAD revision number
> should store it in the baton so that everything else can refer to it.
> If this discovery happens on the client side, that should be possible.
I thought SVN_INVALID_REVNUM was used for marking WORKING, see
libsvn_client/diff.c::diff_label():
[[[
static const char *
diff_label(const char *path,
svn_revnum_t revnum,
apr_pool_t *pool)
{
const char *label;
if (revnum != SVN_INVALID_REVNUM)
label = apr_psprintf(pool, _("%s\t(revision %ld)"), path, revnum);
else
label = apr_psprintf(pool, _("%s\t(working copy)"), path);
return label;
}
]]]
Removed the XFail markers around diff_tests 32 in r962788.
Thanks,
Daniel
Received on 2010-07-10 11:17:42 CEST