You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Näslund <da...@longitudo.com> on 2010/07/09 07:52:21 UTC
Non-matching revision arguments in the diff code
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
I was under the impression that a revision is something fixed (after
we've done the peg revision dance). Any suggestion to why the revisions
may differ?
Sidenote: svn diff -r PREV:BASE shows 'working copy' as revision in the
diff label. I'm thinking 'working copy' must involve the working
changes.
Thanks,
Daniel
Re: Non-matching revision arguments in the diff code
Posted by Daniel Näslund <da...@longitudo.com>.
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
Re: Non-matching revision arguments in the diff code
Posted by Daniel Näslund <da...@longitudo.com>.
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
Re: Non-matching revision arguments in the diff code
Posted by Julian Foad <ju...@wandisco.com>.
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
Hi Daniel.
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.
- Julian
> I was under the impression that a revision is something fixed (after
> we've done the peg revision dance). Any suggestion to why the revisions
> may differ?
>
> Sidenote: svn diff -r PREV:BASE shows 'working copy' as revision in the
> diff label. I'm thinking 'working copy' must involve the working
> changes.
>
> Thanks,
> Daniel