Re: [PATCH] change behaviour of svn_client__get_revision_number()?

For those who have difficulties seeing the attachment, let me repost it
inline. You can also get it at
http://kleinekatze.de/get_revision_number.20100331-1154

~Neels

[[[
Remove entry_t usage from svn_client__get_revision_number().

* subversion/include/private/svn_wc_private.h
  (svn_wc__node_get_base_rev): Properly document this function.
  (svn_wc__node_get_commit_base_rev): New function, declaration.

* subversion/libsvn_client/revisions.c
  (svn_client__get_revision_number): Remove entry_t use. Behaviour changes:
    Added nodes return SVN_INVALID_REVNUM instead of 0. Otherwise return
    the commit-base revision (the copy-from revision if applicable, otherwise
    the NG-BASE node's revision).

* subversion/libsvn_wc/node.c
  (svn_wc__node_get_commit_base_rev): New function, implementation.

* subversion/tests/cmdline/diff_tests.py
  (diff_non_version_controlled_file): Adapt expected error message string.

--This line, and those below, will be ignored--
Index: subversion/include/private/svn_wc_private.h
===================================================================
--- subversion/include/private/svn_wc_private.h (revision 929082)
+++ subversion/include/private/svn_wc_private.h (working copy)
@@ -477,6 +477,16 @@ svn_wc__node_is_status_added(svn_boolean
  * Get the base revision of @a local_abspath using @a wc_ctx. If
  * @a local_abspath is not in the working copy, return
  * @c SVN_ERR_WC_PATH_NOT_FOUND.
+ *
+ * Return the revision of the revert-base, i.e. the revision that this node
+ * was checked out at or last updated/switched to regardless of any
+ * uncommitted changes (delete, replace and/or copy-here/move-here). The
+ * returned revision does not reflect any uncommitted changes on the node.
+ *
+ * For a locally added node that is not part of a replace, this
+ * returns @c SVN_INVALID_REVNUM.
+ *
+ * See also svn_wc__node_get_commit_base_rev().
  */
 svn_error_t *
 svn_wc__node_get_base_rev(svn_revnum_t *base_revision,
@@ -485,6 +495,31 @@ svn_wc__node_get_base_rev(svn_revnum_t *
                           apr_pool_t *scratch_pool);

 /**
+ * Get the base revision of @a local_abspath using @a wc_ctx. If
+ * @a local_abspath is not in the working copy, return
+ * @c SVN_ERR_WC_PATH_NOT_FOUND.
+ *
+ * Return the revision number of the base for this node's next commit, which
+ * reflects the uncommitted changes on this node.
+ *
+ * If this node has no uncommitted changes, return the same as
+ * svn_wc__node_get_base_rev().
+ *
+ * If this node is moved-here or copied-here (possibly as part of a replace),
+ * return the revision of the copy/move source.
+ *
+ * If this node is locally added or replaced and is not moved-here or
+ * copied-here, return the (revert-base) revision of the parent node under
+ * which this node was added (more precisely, the revision of the parent of
+ * this local add's op-root).
+ */
+svn_error_t *
+svn_wc__node_get_commit_base_rev(svn_revnum_t *base_revision,
+ svn_wc_context_t *wc_ctx,
+ const char *local_abspath,
+ apr_pool_t *scratch_pool);
+
+/**
  * Get the lock token of @a local_abspath using @a wc_ctx or NULL
  * if there is no lock. If @a local_abspath is not in the working
 * copy, return @c SVN_ERR_WC_PATH_NOT_FOUND.
Index: subversion/libsvn_client/revisions.c
===================================================================
--- subversion/libsvn_client/revisions.c (revision 929082)
+++ subversion/libsvn_client/revisions.c (working copy)
@@ -79,8 +79,6 @@ svn_client__get_revision_number(svn_revn
     case svn_opt_revision_working:
     case svn_opt_revision_base:
       {
- const svn_wc_entry_t *ent;
-
         /* Sanity check. */
         if (local_abspath == NULL)
           return svn_error_create(SVN_ERR_CLIENT_VERSIONED_PATH_REQUIRED,
@@ -91,19 +89,15 @@ svn_client__get_revision_number(svn_revn
         if (svn_path_is_url(local_abspath))
           goto invalid_rev_arg;

- SVN_ERR(svn_wc__get_entry_versioned(&ent, wc_ctx, local_abspath,
- svn_node_unknown, FALSE, FALSE,
- scratch_pool, scratch_pool));
-
- *revnum = ent->revision;
+ SVN_ERR(svn_wc__node_get_commit_base_rev(revnum, wc_ctx,
+ local_abspath,
+ scratch_pool));
       }
       break;

     case svn_opt_revision_committed:
     case svn_opt_revision_previous:
       {
- const svn_wc_entry_t *ent;
-
         /* Sanity check. */
         if (local_abspath == NULL)
           return svn_error_create(SVN_ERR_CLIENT_VERSIONED_PATH_REQUIRED,
@@ -114,15 +108,14 @@ svn_client__get_revision_number(svn_revn
         if (svn_path_is_url(local_abspath))
           goto invalid_rev_arg;

- SVN_ERR(svn_wc__get_entry_versioned(&ent, wc_ctx, local_abspath,
- svn_node_unknown, FALSE, FALSE,
- scratch_pool, scratch_pool));
-
- if (! SVN_IS_VALID_REVNUM(ent->cmt_rev))
+ SVN_ERR(svn_wc__node_get_changed_info(revnum, NULL, NULL,
+ wc_ctx, local_abspath,
+ scratch_pool, scratch_pool));
+ if (! SVN_IS_VALID_REVNUM(*revnum))
           return svn_error_createf(SVN_ERR_CLIENT_BAD_REVISION, NULL,
                                    _("Path '%s' has no committed "
                                      "revision"), local_abspath);
- *revnum = ent->cmt_rev;
+
         if (revision->kind == svn_opt_revision_previous)
           (*revnum)--;
       }
Index: subversion/libsvn_wc/node.c
===================================================================
--- subversion/libsvn_wc/node.c (revision 929082)
+++ subversion/libsvn_wc/node.c (working copy)
@@ -626,6 +626,41 @@ 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));
+
+ /* 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));
+ }
+
+ return SVN_NO_ERROR;
+}
+
+svn_error_t *
 svn_wc__node_get_lock_token(const char **lock_token,
                             svn_wc_context_t *wc_ctx,
                             const char *local_abspath,
Index: subversion/tests/cmdline/diff_tests.py
===================================================================
--- subversion/tests/cmdline/diff_tests.py (revision 929082)
+++ subversion/tests/cmdline/diff_tests.py (working copy)
@@ -568,7 +568,7 @@ def diff_non_version_controlled_file(sbo
   # there was a way to figure out if svn crashed, but all run_svn gives us is
   # the output, so here we are...
   for line in err_output:
- if re.search("foo' is not under version control$", line):
+ if re.search("foo' was not found.$", line):
       break
   else:
     raise svntest.Failure
]]]

neels wrote:
> Hi all,
>
> I have a patch that passes 'make check', but changes the behaviour of
> svn_client__get_revision_number() in ways that surpass my current
> understanding. Previously, this function would follow the
> overloadedness of entry->revision, in ways not entirely clear to me.
>
> read_entries_new() does these things WRT entry->revision:
> - get the revision from __read_info()
> - if status_added, pick up the parent node's revision instead
> - but if base_shadowed, use the NG-BASE node's revision.
> - if not_present, use NG-BASE node's revision
> - if plain add, set revision = 0
> - if obstructed_add, set revision = SVN_INVALID_REVNUM
> - if copied, set revision to scan_addition's ORIGINAL_REVISION
>
> When svn_client__get_revision_number() was called with
> svn_opt_revision_base or svn_opt_revision_working, it just returned
> the entry->revision. Frankly, it's a bloody nightmare to figure out
> what happened when, exactly.
>
> Instead, I intend to implement svn_client__get_revision_number() with
> svn_opt_revision_base | svn_opt_revision_working like this:
> return the commit-base (not the revert-base) revision. i.e., return
> the NG-BASE node's revision, but:
> if the node is copied here, return the copy_from revision;
> if the node is plainly added, even if the node is replaced by a
> plain add, return SVN_INVALID_REVNUM.
>
> Attached patch implements that AFAICT, but I note some behaviour
> changes (I did a dumb printf() of every revision returned and compared
> trunk's make check output to my patch)
> - Added nodes now return revision number -1 instead of 0 (expected)
> - Copied-here nodes seem to have sometimes returned an earlier
> revision than the copy_from revision that this patch returns
> (unexpected to me)
>
> Note once more that the patch passes 'make check'. But I am unsure
> whether I really understand what I'm doing. Comments welcome.
>
> Less importantly, another change is in the returned error message. If
> the function was called on a node not under version control, it would
> issue an SVN_ERR_WC_PATH_NOT_FOUND with error string "'foo' is not
> under version control". With this patch, svn still returns the same
> error code, but an error message of "'foo' was not found." (see the
> change of diff_tests.py in attached patch) -- I'm wondering if I
> should mangle the error message to be identical to previous behaviour.
> The old message makes slightly more sense to humans in this case,
> right?
>
> Thanks,
> ~Neels