Daniel Näslund wrote:
> On Fri, Apr 16, 2010 at 11:09:06PM -0000, neels_at_apache.org wrote:
>
> [...]
>
>> ==============================================================================
>> --- subversion/trunk/subversion/libsvn_wc/node.c (original)
>> +++ subversion/trunk/subversion/libsvn_wc/node.c Fri Apr 16 23:09:06 2010
>> @@ -626,6 +626,75 @@ svn_wc__node_get_base_rev(svn_revnum_t *
>> }
>>
>> svn_error_t *
>> +svn_wc__node_get_commit_base_rev(svn_revnum_t *commit_base_revision,
>> + svn_wc_context_t *wc_ctx,
>> + const char *local_abspath,
>> + apr_pool_t *scratch_pool)
>> +{
>> + svn_wc__db_status_t status;
>> +
>> + SVN_ERR(svn_wc__db_read_info(&status, NULL,
>> + commit_base_revision,
>> + NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
>> + NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
>> + NULL, NULL, NULL, NULL, NULL,
>> + wc_ctx->db, local_abspath, scratch_pool,
>> + scratch_pool));
>
> What about if a path has been replaced? svn_wc__node_get_commit_base_rev
> is used when diffing wc to wc. Consider this:
>
> [[[
> $ svn rm A/D/G/pi
> $ svn di
> Index: A/D/G/pi
> ===================================================================
> --- A/D/G/pi (revision 1)
> +++ A/D/G/pi (working copy)
> @@ -1 +0,0 @@
> -This is the file 'pi'.
> ]]]
>
> [[[
> $ svn rm A/D/G/pi
> $ touch A/D/G/pi
> $ svn add A/D/G/pi
> Index: A/D/G/pi
> ===================================================================
> --- A/D/G/pi (working copy)
> +++ A/D/G/pi (working copy)
> @@ -1 +0,0 @@
> -This is the file 'pi'.
> ]]]
>
> When we have a replaced node we get an incorrect base revision!
Thanks!
Looking at this, I first found that I apparently broke diff of
simply-deleted nodes. I fixed that in r936464.
Then I looked into replace -- but apparently that behaviour was not changed
by my patch. However, looking into 1.6.x, it still says a revision number
instead of the first "(working copy)" line. So I found some entirely
unrelated code being responsible for that, got a fix for it in the pipeline
as we speak.
It is not plain straightforward to define how svn should act by default. But
you're right that this behaviour is bogus: the line "This is the file 'pi'."
obviously is of revision 1, and diff is showing revision 1's content.
>> + /* If this returned a valid revnum, there is no WORKING node. The node is
>> + cleanly checked out, no modifications, copies or replaces. */
>> + if (SVN_IS_VALID_REVNUM(*commit_base_revision))
>> + return SVN_NO_ERROR;
>> +
>> + if (status == svn_wc__db_status_added)
>> + {
>> + /* If the node was copied/moved-here, return the copy/move source
>> + revision (not this node's base revision). If it's just added,
>> + return SVN_INVALID_REVNUM. */
>> + SVN_ERR(svn_wc__db_scan_addition(NULL, NULL, NULL, NULL, NULL, NULL,
>> + NULL, NULL, commit_base_revision,
>> + wc_ctx->db, local_abspath,
>> + scratch_pool, scratch_pool));
>> + }
>
> I assume you're doing this to not change the behaviour of the callers.
> But that we in the future, always should set the revision to -1 for
> add, copy-here and move-here.
No, actually not entirely. -1 would be fine if we were to redesign the use
of svn_opt_revision_kind_t. Currently, a return value of -1 corresponds to
svn_opt_revision_unspecified, and callers assume HEAD or stuff. So it's Not
Good when the return value for _revision_base of an added node looks like we
were asking for _revision_unspecified. I've made the code return an error
instead of SVN_INVALID_REVNUM, and I had to adjust two callers to do the
ugly 0 magic themselves.
There should be more solid API around this. I believe it would be best to
not even have a function to get only a revision number, but rather the full
toss of number, URL and content, in one function. We also need something
like an svn_opt_revision_revert_base and we need to properly define
svn_opt_revision_base and _working. In particular, _revision_working should
not even have any revision number, it's the working state that is
uncommitted. So that should throw an error. That kind of stuff. Callers
desperately need to clear up on those things. But I'm not taking that on at
this juncture... I'm a burnt child with these things, I keep biting off more
than I can chew :)
>
>> + else if (status == svn_wc__db_status_deleted)
>> + {
>> + /* This node is deleted, but we didn't get a revnum above. So this must
>> + be a delete inside a locally copied tree. Return the revision number
>> + of the parent of the delete, presumably the copy-from revision.
>> + ### This is legacy behaviour, and it sucks. */
>
> I thought that we would only get a rev from read_info() when we have a
> delete if the op_root was the path itself, e.g. that we would need to
> scan upwards if we had deleted A/D and was checking the rev of A/D/G/pi.
My code was wrong. I fixed that in above-mentioned revision. Thanks!
Not sure if read_info() even returns the revision for the op_root. I think
it did otherwise during my recent testing.
>
>> + const char *work_del_abspath;
>> + const char *parent_abspath;
>> + svn_wc__db_status_t parent_status;
>> +
>> + SVN_ERR(svn_wc__db_scan_deletion(NULL, NULL, NULL,
>> + &work_del_abspath,
>> + wc_ctx->db, local_abspath,
>> + scratch_pool, scratch_pool));
>> + SVN_ERR_ASSERT(work_del_abspath != NULL);
>> + parent_abspath = svn_dirent_dirname(work_del_abspath, scratch_pool);
>> +
>> + SVN_ERR(svn_wc__db_read_info(&parent_status,
>> + NULL, NULL, NULL, NULL, NULL, NULL, NULL,
>> + NULL, NULL, NULL, NULL, NULL, NULL, NULL,
>> + NULL, NULL, NULL, NULL, NULL, NULL, NULL,
>> + NULL, NULL,
>> + wc_ctx->db, parent_abspath,
>> + scratch_pool, scratch_pool));
>> +
>> + SVN_ERR_ASSERT(parent_status == svn_wc__db_status_added
>> + || parent_status == svn_wc__db_status_obstructed_add);
>> +
>> + SVN_ERR(svn_wc__db_scan_addition(NULL, NULL, NULL, NULL, NULL, NULL,
>> + NULL, NULL,
>> + commit_base_revision,
>> + wc_ctx->db, parent_abspath,
>> + scratch_pool, scratch_pool));
>> + }
>> +
>> + return SVN_NO_ERROR;
>> +}
>
> In a couple of places we're going to have similar behaviour as this one.
> Consider libsvn_wc/adm_crawler.c:find_base_rev() and my WIP for
> libsvn_wc/status.c:assemble_status(). Maybe some parts could be put
> together to avoid code duplication.
I actually haven't thought to the bottom of which revision numbers we should
be getting when. It's rather complex. My feeling is that it should be
simpler than this, because callers should be asking more precise things.
But feel free to "outsource" some code. Maybe your code can even call
svn_client__get_commit_base_rev()? I've also found other code duplicating
this kind of behaviour (or failing to) -- subversion/libsvn_wc/diff.c
(file_diff) determines the revisions displayed by diff_wc_wc. I guess mature
code would also be sensitive to whether the revision is a copy_from or
revert_base revision, and for comparisons against copied/moved-here bases it
would make the client display something like "(URL_at_REV)" instead of
"(revision REV)" or "(working copy)".
So there's a lot to do. Maybe later. I'm still concentrating on keeping
things as similar as possible.
~Neels
Received on 2010-04-22 01:28:46 CEST