On Wed, 2008-11-26 at 03:59 +0100, Neels J Hofmeyr wrote:
> sbutler_at_tigris.org wrote:
> > Author: sbutler
> > Date: Sat Nov 22 15:46:26 2008
> > New Revision: 34345
> >
> > Log:
> > Properly loggify the recording of tree conflict data during update and
> > switch. The desired end result is the same as before; only the timing
> > of the entry-writing has changed. It's now consistent with the other
> > loggy commands.
> >
> > * subversion/libsvn_wc/tree_conflicts.c
> > (svn_wc__loggy_add_tree_conflict): Move to log.c, where the other
> > svn_wc__loggy_* functions are.
> >
> > * subversion/libsvn_wc/log.c
> > (#include tree_conflicts.h): We need tree conflict helper functions.
> > (svn_wc__loggy_add_tree_conflict): Move here from tree_conflicts.
> > Change this function to simply write an <add-tree-conflict> log
> > element in the victim's parent's log.
> > (SVN_WC__LOG_ADD_TREE_CONFLICT,
> > SVN_WC__LOG_ATTR_DATA): New XML element and attribute names.
> > (log_runner): Add fields tree_conflicts_added (boolean) and
> > tree_conflicts (array). Add field result_pool to hold the tree
> > conflicts.
> > (log_do_add_tree_conflict): Sanity check a new tree conflict and
> > add it to loggy->tree_conflicts.
> > (start_handler): Support <add-tree-conflict> log elements.
> > (run_log_from_memory): Note: no tree conflict support needed because
> > this function doesn't update the entry.
> > (run_log): If the log_runner has tree conflicts, encode them as text
> > data and include them in the entry rewrite.
> >
> > * subversion/tests/cmdline/depth_tests.py
> > (make_depth_tree_conflicts): Correct errors in expected status.
> > (verify_lines): New function borrowed from merge tests.
[...]
> > (tree_conflicts_resolved_depth_immediates,
> > tree_conflicts_resolved_depth_infinity): Allow expected output in
> > any order.
> >
> > Modified:
> > trunk/subversion/libsvn_wc/log.c
> > trunk/subversion/libsvn_wc/tree_conflicts.c
> > trunk/subversion/tests/cmdline/depth_tests.py
> >
> > Modified: trunk/subversion/libsvn_wc/log.c
> > URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_wc/log.c?pathrev=34345&r1=34344&r2=34345
> > ==============================================================================
> > --- trunk/subversion/libsvn_wc/log.c Sat Nov 22 15:34:51 2008 (r34344)
> > +++ trunk/subversion/libsvn_wc/log.c Sat Nov 22 15:46:26 2008 (r34345)
> > @@ -41,6 +41,7 @@
> > #include "lock.h"
> > #include "translate.h"
> > #include "questions.h"
> > +#include "tree_conflicts.h"
> >
> > #include "private/svn_wc_private.h"
> > #include "svn_private_config.h"
> > @@ -103,6 +104,9 @@
> > /* Set SVN_WC__LOG_ATTR_NAME to have timestamp SVN_WC__LOG_ATTR_TIMESTAMP. */
> > #define SVN_WC__LOG_SET_TIMESTAMP "set-timestamp"
> >
> > +/* Add a new tree conflict to the parent entry's tree-conflict-data. */
> > +#define SVN_WC__LOG_ADD_TREE_CONFLICT "add-tree-conflict"
> > +
> >
> > /* Handle closure after a commit completes successfully:
> > *
> > @@ -135,6 +139,7 @@
> > #define SVN_WC__LOG_ATTR_PROPVAL "propval"
> > #define SVN_WC__LOG_ATTR_FORMAT "format"
> > #define SVN_WC__LOG_ATTR_FORCE "force"
> > +#define SVN_WC__LOG_ATTR_DATA "data"
> >
> > /* This one is for SVN_WC__LOG_MERGE. */
> > #define SVN_WC__LOG_ATTR_ARG_1 "arg1"
> > @@ -185,14 +190,17 @@
> > /*** Userdata for the callbacks. ***/
> > struct log_runner
> > {
> > - apr_pool_t *pool;
> > + apr_pool_t *pool; /* cleared before processing each log element */
> > + apr_pool_t *result_pool;
> > svn_xml_parser_t *parser;
> > svn_boolean_t entries_modified;
> > svn_boolean_t wcprops_modified;
> > svn_boolean_t rerun;
> > svn_wc_adm_access_t *adm_access; /* the dir in which all this happens */
> > const char *diff3_cmd; /* external diff3 cmd, or null if none */
> > -
> > + svn_boolean_t tree_conflicts_added;
> > + apr_array_header_t *tree_conflicts; /* array of pointers to
> > + svn_wc_conflict_description_t. */
> > /* Which top-level log element we're on for this logfile. Some
> > callers care whether a failure happened on the first element or
> > on some later element (e.g., 'svn cleanup').
> > @@ -1485,6 +1493,45 @@ log_do_upgrade_format(struct log_runner
> > }
> >
> >
> > +static svn_error_t *
> > +log_do_add_tree_conflict(struct log_runner *loggy,
> > + const char **atts)
> > +{
> > + apr_array_header_t *new_conflicts;
> > + const svn_wc_conflict_description_t *new_conflict;
> > + const char* dir_path =svn_wc_adm_access_path(loggy->adm_access);
[...]
> > +
> > + /* Convert the text data to a conflict. */
> > + new_conflict = apr_pcalloc(loggy->pool,
> > + sizeof(svn_wc_conflict_description_t *));
>
> You are setting new_conflict, ...
>
> > + new_conflicts = apr_array_make(loggy->pool, 1,
> > + sizeof(svn_wc_conflict_description_t *));
> > + SVN_ERR(svn_wc__read_tree_conflicts(&new_conflicts,
> > + svn_xml_get_attr_value(SVN_WC__LOG_ATTR_DATA, atts),
> > + dir_path, loggy->pool));
> > + new_conflict = APR_ARRAY_IDX(new_conflicts, 0,
> > + svn_wc_conflict_description_t *);
>
> ...and you are setting it again? Am I not getting something?
It looks like the first one was redundant. I removed it, and also
tweaked where you pointed out typos, in r34498.
Now, the interesting and difficult stuff ...
> > +
> > + /* Re-adding an existing tree conflict victim is an error. */
> > + if (svn_wc__tree_conflict_exists(loggy->tree_conflicts,
> > + svn_path_basename(new_conflict->path,
> > + loggy->pool),
> > + loggy->pool))
> > + return svn_error_create(SVN_ERR_WC_CORRUPT, NULL,
> > + _("Attempt to add tree conflict that already exists"));
This is wrong. Loggy operations must be idempotent, so the meaning of
the operation should not be strictly "add" but instead "set" the tree
conflict description for a particular victim. It should silently do
nothing if the conflict already exists.
If a conflict description already exists for the child, but differs from
the new conflict description for that same child, that is an error. We
could detect that. In theory we will never try to raise such a conflict,
but a check here would be helpful for bug-catching.
There should be a check at a higher level for trying to add an identical
conflict that already exists, as that would be useful for debugging too.
> > + /* Copy the new conflict to to the result pool. Add its pointer to
[...]
> > + the array of existing conflicts. */
> > + APR_ARRAY_PUSH(loggy->tree_conflicts,
> > + const svn_wc_conflict_description_t *) =
> > + svn_wc__conflict_description_dup(new_conflict,
> > + loggy->result_pool);
> > +
> > + loggy->tree_conflicts_added = TRUE;
> > +
> > + return SVN_NO_ERROR;
> > +}
> > +
> > static void
> > start_handler(void *userData, const char *eltname, const char **atts)
> > {
> > @@ -1570,6 +1617,9 @@ start_handler(void *userData, const char
> > else if (strcmp(eltname, SVN_WC__LOG_UPGRADE_FORMAT) == 0) {
> > err = log_do_upgrade_format(loggy, atts);
> > }
> > + else if (strcmp(eltname, SVN_WC__LOG_ADD_TREE_CONFLICT) == 0) {
> > + err = log_do_add_tree_conflict(loggy, atts);
> > + }
> > else
> > {
> > SIGNAL_ERROR
> > @@ -1686,6 +1736,7 @@ run_log_from_memory(svn_wc_adm_access_t
> > loggy = apr_pcalloc(pool, sizeof(*loggy));
> > loggy->adm_access = adm_access;
> > loggy->pool = svn_pool_create(pool);
> > + loggy->result_pool = svn_pool_create(pool);
> > loggy->parser = svn_xml_make_parser(loggy, start_handler,
> > NULL, NULL, pool);
> > loggy->entries_modified = FALSE;
> > @@ -1693,6 +1744,8 @@ run_log_from_memory(svn_wc_adm_access_t
> > loggy->rerun = rerun;
> > loggy->diff3_cmd = diff3_cmd;
> > loggy->count = 0;
> > + loggy->tree_conflicts_added = FALSE;
Why is the array not initialized from the 'entry' here in
run_log_from_memory() like it is in run_log()?
> > + loggy->tree_conflicts = NULL;
> >
> > parser = loggy->parser;
> > /* Expat wants everything wrapped in a top-level form, so start with
> > @@ -1720,6 +1773,7 @@ run_log(svn_wc_adm_access_t *adm_access,
> > int log_number;
> > apr_pool_t *iterpool = svn_pool_create(pool);
> > svn_boolean_t killme, kill_adm_only;
> > + const svn_wc_entry_t *entry;
> >
> > /* kff todo: use the tag-making functions here, now. */
> > const char *log_start
> > @@ -1736,12 +1790,24 @@ run_log(svn_wc_adm_access_t *adm_access,
> > parser = svn_xml_make_parser(loggy, start_handler, NULL, NULL, pool);
> > loggy->adm_access = adm_access;
> > loggy->pool = svn_pool_create(pool);
> > + loggy->result_pool = svn_pool_create(pool);
> > loggy->parser = parser;
> > loggy->entries_modified = FALSE;
> > loggy->wcprops_modified = FALSE;
> > loggy->rerun = rerun;
> > loggy->diff3_cmd = diff3_cmd;
> > loggy->count = 0;
> > + loggy->tree_conflicts_added = FALSE;
> > + loggy->tree_conflicts = apr_array_make(pool, 0,
> > + sizeof(svn_wc_conflict_description_t *));
> > +
> > + /* Populate the tree conflict array with the existing tree conflicts. */
> > + SVN_ERR(svn_wc_entry(&entry, svn_wc_adm_access_path(adm_access), adm_access,
> > + TRUE, pool));
> > + SVN_ERR(svn_wc__read_tree_conflicts(&(loggy->tree_conflicts),
> > + entry->tree_conflict_data,
> > + svn_wc_adm_access_path(adm_access),
> > + pool));
>
> Nice, only one read of existing conflicts per parent dir.
> But, wait a minute, that means that the conflicts are all written out only
> *after* the rest of the loggy commands have been executed?
Yes. This code runs a log file with tree conflict changes as follows:
1. Read the existing tree conflicts from the 'entry' in the adm_access
cache, into a list in memory.
2. Execute all of the log commands; for the "add tree conflict"
command, add the description to the list in memory.
3. Write the new list of tree conflicts to the 'entry' with
svn_wc__entry_modify().
4. Some sort of re-synchronisation:
> {
> apr_hash_t *entries;
> SVN_ERR(svn_wc_entries_read(&entries, adm_access, TRUE, pool));
> SVN_ERR(svn_wc__entries_write(entries, adm_access, pool));
> }
If the "add tree conflict" command is idempotent, will this whole
algorithm should be fine?
> Could that be a problem? I don't know enough about it, so I'm paranoid about
> changing the order in any way... :P
I don't understand the implications either, so I cannot say whether it
is OK.
> Does it make sense to write out the new state of tree_conflicts after each
> consecutive series of tree-conflicts-add log commands?
>
> >
> > /* Expat wants everything wrapped in a top-level form, so start with
> > a ghost open tag. */
> > @@ -1794,7 +1860,29 @@ run_log(svn_wc_adm_access_t *adm_access,
> > goto rerun;
> > #endif
That "#endif" is the end of "#ifdef RERUN_LOG_FILES" testing loop, so
that we can test for the ability to re-run the loggy operations without
ill effect. I think the new block added below should be within that
loop.
However, I tried enabling "RERUN_LOG_FILES" in trunk_at_34498 without
making any other change, and many of the regression tests fail, so it
looks like the loggy code has bit-rotted.
> > - if (loggy->entries_modified == TRUE)
> > + /* If the logs included tree conflicts, write them to the entry. */
> > + if (loggy->tree_conflicts_added)
> > + {
> > + char *conflict_data;
> > + svn_wc_entry_t tmp_entry;
> > + svn_error_t *err;
> > +
> > + SVN_ERR(svn_wc__write_tree_conflicts(&conflict_data,
> > + loggy->tree_conflicts, pool));
> > +
> > + tmp_entry.tree_conflict_data = apr_pstrdup(pool, conflict_data);
> > + err = svn_wc__entry_modify(adm_access, SVN_WC_ENTRY_THIS_DIR,
> > + &tmp_entry,
> > + SVN_WC__ENTRY_MODIFY_TREE_CONFLICT_DATA,
> > + FALSE, pool);
> > + if (err)
> > + return svn_error_createf(pick_error_code(loggy), err,
> > + _("Error recording tree conflicts in '%s'"),
> > + svn_wc_adm_access_path(adm_access));
> > +
> > + loggy->entries_modified = TRUE;
> > + }
> > + if (loggy->entries_modified)
> > {
> > apr_hash_t *entries;
> > SVN_ERR(svn_wc_entries_read(&entries, adm_access, TRUE, pool));
> > @@ -2367,6 +2455,33 @@ svn_wc__loggy_upgrade_format(svn_stringb
> > return SVN_NO_ERROR;
> > }
> >
> > +svn_error_t *
> > +svn_wc__loggy_add_tree_conflict(svn_stringbuf_t **log_accum,
> > + const svn_wc_conflict_description_t *conflict,
> > + svn_wc_adm_access_t *adm_access,
> > + apr_pool_t *pool)
> > +{
> > + char *conflict_data;
> > + apr_array_header_t *conflicts;
> > +
> > + /* ### TODO: implement write_one_tree_conflict(). */
> > + conflicts = apr_array_make(pool, 1,
> > + sizeof(svn_wc_conflict_description_t *));
> > + APR_ARRAY_PUSH(conflicts, const svn_wc_conflict_description_t *) = conflict;
> > +
> > + SVN_ERR(svn_wc__write_tree_conflicts(&conflict_data, conflicts, pool));
> > +
> > + svn_xml_make_open_tag(log_accum, pool, svn_xml_self_closing,
> > + SVN_WC__LOG_ADD_TREE_CONFLICT,
> > + SVN_WC__LOG_ATTR_NAME,
> > + SVN_WC_ENTRY_THIS_DIR,
> > + SVN_WC__LOG_ATTR_DATA,
> > + conflict_data,
> > + NULL);
> > +
> > + return SVN_NO_ERROR;
> > +}
> > +
> >
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-12-01 16:14:08 CET