The attached patch is an attempt to close the gap between "transmit text
deltas" and commit post-processing, by passing the temporary new text
base file paths around. It succeeds in that, at least it looks right
and passes the test suite.
The part of this patch that I haven't finished is with back-compat of
svn_wc__process_committed_internal(), and what is the difference between
its 'queue' parameter and its checksum/recurse/no_unlock/etc.
parameters, being values which are alternatively available in the queue.
svn_wc__process_committed_internal() is called by
svn_wc_process_committed_queue2() which passes the 'queue' param, and
also by the deprecated svn_wc_process_committed4() with QUEUE=NULL. I
had been assuming that if QUEUE==NULL then all the parameters that are
available in the queue (checksum for one) are not available, but that's
not how the back-compat wrapper wants to work. I'll need to fix that.
I think the right thing to do is to re-write the wrapper
(svn_wc_process_committed4()) to construct a new queue with one item and
pass that, and stop having the other parameters (checksum etc.) passed
as separate parameters. I'll look at that tomorrow. I may already have
committed changes that break that back-compat; I'll check.
The internal recursion of svn_wc__process_committed_internal() is
confusing me: it passes NO_UNLOCK=TRUE to itself when recursing,
regardless of what no_unlock value was passed in or what is in the queue
item. Yet it passes the received value of KEEP_CHANGELIST. For the
NEW_DAV_CACHE and CHECKSUM arguments it passes NULL, which is half
explained by them only having meaning in connection with a single node:
the same ones cannot be applicable to a child node.
Comments?
- Julian
### This version of the patch carries the text base path through the client
### 'tempfiles' hash and adds it into the WC commit-items struct.
In a commit, pass the temporary paths of the new text base files through
from the pre-commit to the post-commit stage, instead of re-deriving them.
This completes the data flow of these paths so that they no longer need to
be deterministically derivable.
* subversion/include/svn_wc.h
(svn_wc_queue_committed3): Add a new parameter 'new_text_base_abspath'.
### Also doc that CHECKSUM must be given when applicable, but that should be a separate change.
(svn_wc_queue_committed2): Adjust doc string accordingly.
* subversion/libsvn_client/commit.c
(post_commit_baton, post_process_commit_item, svn_client_commit4): Pass the
new text base abspaths down, through the post_commit_baton, to
svn_wc_queue_committed3().
* subversion/libsvn_wc/adm_crawler.c
(svn_wc_transmit_text_deltas3):
### Brute-force verification of these changes. Not to be committed.
* subversion/libsvn_wc/adm_ops.c
(committed_queue_item_t): Add a new member 'new_text_base_abspath'.
(process_committed_leaf): Take 'new_text_base_abspath' as a parameter
instead of deriving it.
(svn_wc__process_committed_internal): Find 'new_text_base_abspath' and
'checksum' in the Committed Queue Item and pass them to
process_committed_leaf(). Don't take 'checksum' as a param.
(svn_wc_queue_committed3): Take 'new_text_base_abspath' as a parameter and
store it in the queue.
(svn_wc_process_committed_queue2): Adjust the call to
svn_wc__process_committed_internal().
* subversion/libsvn_wc/deprecated.c
(svn_wc_process_committed4): Adjust the call to
svn_wc__process_committed_internal().
(svn_wc_queue_committed2): Implement the old behaviour, looking for a file
at a deterministic temporary text base path, and passing that to
svn_wc_queue_committed3().
* subversion/libsvn_wc/wc.h
(svn_wc__process_committed_internal): Remove the 'checksum' parameter, as it
is now passed via the 'queue' parameter.
* notes/wc-ng/use-of-tmp-text-base-path
Update accordingly.
--This line, and those below, will be ignored--
Index: notes/wc-ng/use-of-tmp-text-base-path
===================================================================
--- notes/wc-ng/use-of-tmp-text-base-path (revision 928789)
+++ notes/wc-ng/use-of-tmp-text-base-path (working copy)
@@ -1,5 +1,5 @@
-Call graphs of the use of the WC-1 temporary text base path, as of r927056.
+Call graphs of the use of the WC-1 temporary text base path.
This is to help us eliminate the use of this path and replace it with a more
encapsulated way of referring to the new text base, as part of migration to a
@@ -14,46 +14,46 @@ path is obtained, and the extent to whic
svn_client_commit4()
- |^[T] | [T] Terminates here
- wc_to_repos_copy() |^[M] |
- | |^ | [M] Multiple files
- svn_client__do_commit() |
- [N] |^ | [N] Not when caller is
- |^ | wc_to_repos_copy()
- |^ |
- LIBSVN_CLIENT |^ |
+ |^ |v [T] Terminates here
+ wc_to_repos_copy() |^[M] |v[M]
+ | |^ |v [M] Multiple files
+ svn_client__do_commit() |v
+ [N] |^ |v [N] Not when caller is
+ |^ |v wc_to_repos_copy()
+ |^ |v
+ LIBSVN_CLIENT |^ |v
..........................................................................
- LIBSVN_WC |^ |
- |^ |
+ LIBSVN_WC |^ |v
+ |^ |v
|^ +---+
- |^ |
- svn_wc_transmit_text_deltas3() |
- [N] |^ |
- svn_wc__internal_transmit_text_deltas() |
- [N] |^ |
- |^ |
+ |^ |v
+ svn_wc_transmit_text_deltas3() |v
+ [N] |^ |v
+ svn_wc__internal_transmit_text_deltas() |v
+ [N] |^ |v
+ |^ |v
|^ { svn_wc_process_committed_queue2() }
|^ { svn_wc_process_committed4() }
- |^ |
+ |^ |v
|^ svn_wc__process_committed_internal()
- |^ |
+ |^ |v
|^ process_committed_leaf()
- |^ |^ |v
- |^ |^ svn_wc__wq_add_postcommit()
- |^ |^ *v
- |^ |^ *v
- |^ |^ *v
- |^ |^ WQ:OP_POSTCOMMIT
- |^ |^ *v
- |^ |^ *v
- |^ |^ *v
- |^ |^ run_postcommit()
- |^ |^ |v
- |^ |^ log_do_committed()
- |^ |^ |v
- |^ |^ install_committed_file()
- |^ |^ |v
- |^ |^ |v
+ |^ | |v
+ |^ | svn_wc__wq_add_postcommit()
+ |^ | *v
+ |^ | *v
+ |^ | *v
+ |^ | WQ:OP_POSTCOMMIT
+ |^ | *v
+ |^ | *v
+ |^ | *v
+ |^ | run_postcommit()
+ |^ | |v
+ |^ | log_do_committed()
+ |^ | |v
+ |^ | install_committed_file()
+ |^ | |v
+ |^ | |v
svn_wc__text_base_path(tmp=TRUE) svn_wc__sync_text_base()
|^ |v
|^ [initialization] svn_io_rename()
Index: subversion/include/svn_wc.h
===================================================================
--- subversion/include/svn_wc.h (revision 928789)
+++ subversion/include/svn_wc.h (working copy)
@@ -4746,8 +4746,9 @@ svn_wc_committed_queue_create(apr_pool_t
* svn_wc_process_committed_queue2().
*
* All pointer data passed to this function (@a local_abspath,
- * @a wcprop_changes * and @a checksum) should remain valid until the
- * queue has been processed by svn_wc_process_committed_queue2().
+ * @a wcprop_changes, @a checksum and @a new_text_base_abspath) should
+ * remain valid until the queue has been processed by
+ * svn_wc_process_committed_queue2().
*
* Record in @a queue that @a local_abspath will need to be bumped
* after a commit succeeds.
@@ -4765,11 +4766,9 @@ svn_wc_committed_queue_create(apr_pool_t
* If @a remove_changelist is @c TRUE, any association with a
* changelist will be removed.
*
- * If @a local_abspath is a file and @a checksum is non-NULL, use @a checksum
- * as the checksum for the new text base. Otherwise, calculate the checksum
- * if needed.
- * ### [JAF] No, it doesn't calculate the checksum, it stores null in wc.db:
- * ### see svn_wc__process_committed_internal().
+ * If there is a new text base file to be installed, @a new_text_base_abspath
+ * must be its path and @a checksum must be its MD5 checksum. Otherwise,
+ * @a new_text_base_abspath and @a checksum must be NULL.
*
* If @a recurse is TRUE and @a local_abspath is a directory, then bump every
* versioned object at or under @a path. This is usually done for
@@ -4791,11 +4790,15 @@ svn_wc_queue_committed3(svn_wc_committed
const apr_array_header_t *wcprop_changes,
svn_boolean_t remove_lock,
svn_boolean_t remove_changelist,
+ const char *new_text_base_abspath,
const svn_checksum_t *checksum,
apr_pool_t *scratch_pool);
/** Same as svn_wc_queue_committed3() except @a path doesn't have to be an
- * abspath and @a adm_access is unused.
+ * abspath, @a adm_access is unused and the new text base file (if there is
+ * one) is found automatically.
+ * ### and the docs previously implied @a checksum was optional, but it
+ * ### doesn't seem so: see svn_wc__process_committed_internal().
*
* @since New in 1.6.
*
Index: subversion/libsvn_client/commit.c
===================================================================
--- subversion/libsvn_client/commit.c (revision 928789)
+++ subversion/libsvn_client/commit.c (working copy)
@@ -941,6 +941,7 @@ struct post_commit_baton
svn_wc_context_t *wc_ctx;
svn_boolean_t keep_changelists;
svn_boolean_t keep_locks;
+ apr_hash_t *new_text_base_abspaths;
apr_hash_t *checksums;
};
@@ -984,6 +985,9 @@ post_process_commit_item(void *baton, vo
return svn_wc_queue_committed3(btn->queue, item->path,
loop_recurse, item->incoming_prop_changes,
remove_lock, !btn->keep_changelists,
+ apr_hash_get(btn->new_text_base_abspaths,
+ item->path,
+ APR_HASH_KEY_STRING),
apr_hash_get(btn->checksums,
item->path,
APR_HASH_KEY_STRING),
@@ -1098,7 +1102,7 @@ svn_client_commit4(svn_commit_info_t **c
apr_array_header_t *rel_targets;
apr_hash_t *committables;
apr_hash_t *lock_tokens;
- apr_hash_t *tempfiles = NULL;
+ apr_hash_t *new_text_base_abspaths = NULL;
apr_hash_t *checksums;
apr_array_header_t *commit_items;
svn_error_t *cmt_err = SVN_NO_ERROR, *unlock_err = SVN_NO_ERROR;
@@ -1256,8 +1260,8 @@ svn_client_commit4(svn_commit_info_t **c
/* Perform the commit. */
cmt_err = svn_client__do_commit(base_url, commit_items, editor, edit_baton,
- notify_prefix, &tempfiles, &checksums, ctx,
- pool);
+ notify_prefix, &new_text_base_abspaths,
+ &checksums, ctx, pool);
/* Handle a successful commit. */
if ((! cmt_err)
@@ -1271,6 +1275,7 @@ svn_client_commit4(svn_commit_info_t **c
btn.wc_ctx = ctx->wc_ctx;
btn.keep_changelists = keep_changelists;
btn.keep_locks = keep_locks;
+ btn.new_text_base_abspaths = new_text_base_abspaths;
btn.checksums = checksums;
/* Make a note that our commit is finished. */
@@ -1309,7 +1314,7 @@ svn_client_commit4(svn_commit_info_t **c
unlock_err = svn_wc__release_write_lock(ctx->wc_ctx, base_dir, pool);
if (! unlock_err)
- cleanup_err = remove_tmpfiles(tempfiles, pool);
+ cleanup_err = remove_tmpfiles(new_text_base_abspaths, pool);
}
/* As per our promise, if *commit_info_p isn't set, provide a default where
Index: subversion/libsvn_wc/adm_crawler.c
===================================================================
--- subversion/libsvn_wc/adm_crawler.c (revision 928789)
+++ subversion/libsvn_wc/adm_crawler.c (working copy)
@@ -1330,10 +1330,36 @@ svn_wc_transmit_text_deltas3(const char
apr_pool_t *result_pool,
apr_pool_t *scratch_pool)
{
- return svn_wc__internal_transmit_text_deltas(tempfile, digest, wc_ctx->db,
- local_abspath, fulltext, editor,
- file_baton, result_pool,
- scratch_pool);
+ const char *tempfile_1;
+ const char *new_text_base_abspath;
+ svn_node_kind_t new_text_base_kind;
+
+ SVN_ERR(svn_wc__internal_transmit_text_deltas(&tempfile_1, digest, wc_ctx->db,
+ local_abspath, fulltext, editor,
+ file_baton, result_pool,
+ scratch_pool));
+ if (tempfile)
+ *tempfile = tempfile_1;
+
+ /* For transition, verify that TEMPFILE_1 is non-null iff a file exists
+ * at the WC-1 deterministic tmp text base path. */
+
+ SVN_ERR(svn_wc__text_base_path(&new_text_base_abspath, wc_ctx->db,
+ local_abspath, TRUE, scratch_pool));
+ SVN_ERR(svn_io_check_path(new_text_base_abspath, &new_text_base_kind,
+ scratch_pool));
+ if (new_text_base_kind != svn_node_file)
+ new_text_base_abspath = NULL;
+
+ if (!tempfile_1 != !new_text_base_abspath
+ || (tempfile_1 && strcmp(tempfile_1, new_text_base_abspath) != 0))
+ {
+ printf("## tempfile_1='%s',\n## but new_t='%s' and kind=%d\n",
+ tempfile_1, new_text_base_abspath, new_text_base_kind);
+ abort();
+ }
+
+ return SVN_NO_ERROR;
}
svn_error_t *
Index: subversion/libsvn_wc/adm_ops.c
===================================================================
--- subversion/libsvn_wc/adm_ops.c (revision 928789)
+++ subversion/libsvn_wc/adm_ops.c (working copy)
@@ -83,6 +83,7 @@ typedef struct
svn_boolean_t recurse;
svn_boolean_t no_unlock;
svn_boolean_t keep_changelist;
+ const char *new_text_base_abspath;
const svn_checksum_t *checksum;
apr_hash_t *new_dav_cache;
} committed_queue_item_t;
@@ -345,8 +346,8 @@ process_deletion_postcommit(svn_wc__db_t
}
-/* CHECKSUM is the checksum of the new text base for LOCAL_ABSPATH, and must
- * be provided if there is one, else NULL. */
+/* If there is a new text base file for LOCAL_ABSPATH, CHECKSUM must be its
+ * checksum and NEW_TEXT_BASE_ABSPATH must be its path. */
static svn_error_t *
process_committed_leaf(svn_wc__db_t *db,
const char *local_abspath,
@@ -356,6 +357,7 @@ process_committed_leaf(svn_wc__db_t *db,
apr_hash_t *new_dav_cache,
svn_boolean_t no_unlock,
svn_boolean_t keep_changelist,
+ const char *new_text_base_abspath,
const svn_checksum_t *checksum,
apr_pool_t *scratch_pool)
{
@@ -363,7 +365,6 @@ process_committed_leaf(svn_wc__db_t *db,
svn_wc__db_kind_t kind;
const svn_checksum_t *copied_checksum;
const char *adm_abspath;
- const char *tmp_text_base_abspath;
SVN_ERR_ASSERT(svn_dirent_is_absolute(local_abspath));
@@ -431,19 +432,7 @@ process_committed_leaf(svn_wc__db_t *db,
SVN_ERR(svn_wc__loggy_delete_lock(db, adm_abspath,
local_abspath, scratch_pool));
- /* Set TMP_TEXT_BASE_ABSPATH to the new text base to be installed, if any. */
- {
- svn_node_kind_t new_base_kind;
-
- SVN_ERR(svn_wc__text_base_path(&tmp_text_base_abspath, db, local_abspath,
- TRUE, scratch_pool));
- SVN_ERR(svn_io_check_path(tmp_text_base_abspath, &new_base_kind,
- scratch_pool));
- if (new_base_kind != svn_node_file)
- tmp_text_base_abspath = NULL;
- }
-
- SVN_ERR(svn_wc__wq_add_postcommit(db, local_abspath, tmp_text_base_abspath,
+ SVN_ERR(svn_wc__wq_add_postcommit(db, local_abspath, new_text_base_abspath,
new_revnum,
new_date, rev_author, checksum,
new_dav_cache, keep_changelist,
@@ -463,19 +452,24 @@ svn_wc__process_committed_internal(svn_w
apr_hash_t *new_dav_cache,
svn_boolean_t no_unlock,
svn_boolean_t keep_changelist,
- const svn_checksum_t *checksum,
const svn_wc_committed_queue_t *queue,
apr_pool_t *scratch_pool)
{
+ const committed_queue_item_t *cqi;
svn_wc__db_kind_t kind;
+ cqi = queue ? apr_hash_get(queue->queue, local_abspath, APR_HASH_KEY_STRING)
+ : NULL;
+
SVN_ERR(svn_wc__db_read_kind(&kind, db, local_abspath, TRUE, scratch_pool));
SVN_ERR(process_committed_leaf(db, local_abspath,
new_revnum, new_date, rev_author,
new_dav_cache,
no_unlock, keep_changelist,
- checksum, scratch_pool));
+ cqi ? cqi->new_text_base_abspath : NULL,
+ cqi ? cqi->checksum : NULL,
+ scratch_pool));
if (recurse && kind == svn_wc__db_kind_dir)
{
@@ -533,12 +527,15 @@ svn_wc__process_committed_internal(svn_w
rev_author,
NULL,
TRUE /* no_unlock */,
- keep_changelist, NULL,
+ keep_changelist,
queue, iterpool));
SVN_ERR(svn_wc__wq_run(db, this_abspath, NULL, NULL, iterpool));
}
else
{
+ cqi = queue ? apr_hash_get(queue->queue, this_abspath,
+ APR_HASH_KEY_STRING) : NULL;
+
/* Suppress log creation for deleted entries in a replaced
directory. By the time any log we create here is run,
those entries will already have been removed (as a result
@@ -556,23 +553,15 @@ svn_wc__process_committed_internal(svn_w
continue;
}
- checksum = NULL;
- if (queue != NULL)
- {
- const committed_queue_item_t *cqi
- = apr_hash_get(queue->queue, this_abspath,
- APR_HASH_KEY_STRING);
-
- if (cqi != NULL)
- checksum = cqi->checksum;
- }
-
SVN_ERR(process_committed_leaf(db, this_abspath,
new_revnum,
new_date, rev_author, NULL,
TRUE /* no_unlock */,
keep_changelist,
- checksum, iterpool));
+ cqi ? cqi->new_text_base_abspath
+ : NULL,
+ cqi ? cqi->checksum : NULL,
+ iterpool));
}
}
@@ -626,6 +615,7 @@ svn_wc_queue_committed3(svn_wc_committed
const apr_array_header_t *wcprop_changes,
svn_boolean_t remove_lock,
svn_boolean_t remove_changelist,
+ const char *new_text_base_abspath,
const svn_checksum_t *checksum,
apr_pool_t *scratch_pool)
{
@@ -645,6 +635,7 @@ svn_wc_queue_committed3(svn_wc_committed
cqi->recurse = recurse;
cqi->no_unlock = !remove_lock;
cqi->keep_changelist = !remove_changelist;
+ cqi->new_text_base_abspath = new_text_base_abspath;
cqi->checksum = checksum;
cqi->new_dav_cache = svn_wc__prop_array_to_hash(wcprop_changes, queue->pool);
@@ -725,8 +716,7 @@ svn_wc_process_committed_queue2(svn_wc_c
cqi->new_dav_cache,
cqi->no_unlock,
cqi->keep_changelist,
- cqi->checksum, queue,
- iterpool));
+ queue, iterpool));
SVN_ERR(svn_wc__wq_run(wc_ctx->db, cqi->local_abspath, NULL, NULL,
iterpool));
Index: subversion/libsvn_wc/deprecated.c
===================================================================
--- subversion/libsvn_wc/deprecated.c (revision 928789)
+++ subversion/libsvn_wc/deprecated.c (working copy)
@@ -39,6 +39,7 @@
#include "lock.h"
#include "props.h"
#include "workqueue.h"
+#include "adm_files.h"
#include "svn_private_config.h"
@@ -584,7 +585,7 @@ svn_wc_process_committed4(const char *pa
new_revnum, new_date, rev_author,
wcprop_changes_hash,
!remove_lock, !remove_changelist,
- checksum, NULL, pool));
+ NULL, pool));
/* Run the log file(s) we just created. */
return svn_error_return(svn_wc__wq_run(db, local_abspath, NULL, NULL, pool));
@@ -3714,10 +3715,26 @@ svn_wc_queue_committed2(svn_wc_committed
apr_pool_t *scratch_pool)
{
const char *local_abspath;
+ const char *new_text_base_abspath;
SVN_ERR(svn_dirent_get_absolute(&local_abspath, path, scratch_pool));
+
+ /* Set TMP_TEXT_BASE_ABSPATH to the new text base to be installed, if any. */
+ {
+ svn_wc__db_t *db = svn_wc__adm_get_db(adm_access);
+ svn_node_kind_t new_base_kind;
+
+ SVN_ERR(svn_wc__text_base_path(&new_text_base_abspath, db, local_abspath,
+ TRUE, scratch_pool));
+ SVN_ERR(svn_io_check_path(new_text_base_abspath, &new_base_kind,
+ scratch_pool));
+ if (new_base_kind != svn_node_file)
+ new_text_base_abspath = NULL;
+ }
+
return svn_wc_queue_committed3(queue, local_abspath, recurse, wcprop_changes,
- remove_lock, remove_changelist, checksum,
+ remove_lock, remove_changelist,
+ new_text_base_abspath, checksum,
scratch_pool);
}
Index: subversion/libsvn_wc/wc.h
===================================================================
--- subversion/libsvn_wc/wc.h (revision 928789)
+++ subversion/libsvn_wc/wc.h (working copy)
@@ -212,11 +212,6 @@ svn_wc__get_committed_queue_pool(const s
*
* If @keep_changelist is set, don't remove any changeset assignments
* from @a local_abspath; otherwise, clear it of such assignments.
- *
- * If @a local_abspath is a file and @a checksum is non-NULL, use @a checksum
- * as the checksum for the new text base. Otherwise, calculate the checksum
- * if needed.
- * ### [JAF] No, it doesn't calculate the checksum, it stores null in wc.db.
*/
svn_error_t *
svn_wc__process_committed_internal(svn_wc__db_t *db,
@@ -228,7 +223,6 @@ svn_wc__process_committed_internal(svn_w
apr_hash_t *new_dav_cache,
svn_boolean_t no_unlock,
svn_boolean_t keep_changelist,
- const svn_checksum_t *checksum,
const svn_wc_committed_queue_t *queue,
apr_pool_t *scratch_pool);
