I (Julian Foad) wrote on 2013-04-25:
> I (Julian Foad) wrote:
>> There is a second bug, closely related. If there are multiple prop
>> conflicts on the same node, they all get marked as resolved as soon
>> as you request any one of them to be marked as resolved. There is
>> incomplete API support for marking individual properties as resolved
>> so that may be hard to fix.
>
> How hard can it be to add full support for marking individual properties
> as resolved? I see two distinct levels to "full support".
>
> Firstly, to fix the bug in interactive resolution, support is needed in
> libsvn_wc to ensure that when a conflict resolver callback (called from
> libsvn_wc/conflicts.c) returns any kind of 'resolved' result for a
> property conflict, the WC should mark only that specific property as
> resolved. That would be enough to fix the interactive conflict resolution.
> I'm looking at how feasible this might be.
The answer is, it requires changes within libsvn_wc, but AFAICT it doesn't
require any public or inter-library API changes.
The attached patch shows roughly what is needed, but is not finished and does
not yet work properly.
Because it looks like it can be done within libsvn_wc, I am inclined not to
rush to get this into 1.8.0, but to aim for 1.8.1 instead.
- Julian
> Secondly, we might want to enhance the 'svn resolve' UI to allow
> specifying a particular property. I see that as being a comparatively minor
> enhancement. (We could also add such an option to the svn_client_resolve()
> API,
> but the API can always be given a callback function that only resolves
> conflicts
> on a specific property so there is no need for the API to expose such an
> option
> explicitly.)
>
> The best idea I have *short of* getting "full" support by fixing
> libsvn_wc is to make 'svn' track how many of the propery conflicts you
> resolved, and if you resolved some but not all of them then print a warning
> that
> all of the properties were marked as resolved. That's not great but would
> be simple to implement and would improve the user experience a bit.
>
> It might be possible to leave all of the property conflicts on that node
> *unresolved* if they're not all resolved, which is probably a better user
> experience, but I'm not sure how we'd do that purely in the client.
> Once the callback has returned any kind of "resolved" result for one
> property, libsvn_wc currently marks all prop conflicts on the node as
> resolved
> before calling the callback for the next property, and I don't know of any
> good way to avoid that.
Support marking individual properties as resolved. This fixes inconsistent
behaviour in conflict resolution, where a request (by the resolver callback)
to mark any property conflict as resolved would result in all property
conflicts on that node being marked as resolved.
### This patch is incomplete and not yet working properly.
* subversion/libsvn_wc/conflicts.c
(svn_wc__conflict_invoke_resolver): Mark one property conflict at a time
as resolved, instead of all at once.
(resolve_prop_conflict_on_node): Add a 'resolve_prop' parameter and, if
it specifies a property, only update that property.
(resolve_conflict_on_node): Allow the caller to specify a property name,
and pass it along.
(svn_wc__mark_resolved_text_conflict): Adjust calls.
(svn_wc__mark_resolved_prop_conflicts): Add a 'resolve_prop' parameter
and pass it along.
(conflict_status_walker): Call the resolver callback for only the
specified kinds of conflict. If a property name is specified, resolve
property conflicts only on that property.
* subversion/libsvn_wc/conflicts.h
(svn_wc__conflict_skel_resolve): Doc string...
(svn_wc__mark_resolved_prop_conflicts): Add a 'resolve_prop' parameter.
* subversion/libsvn_wc/questions.c
(internal_conflicted_p): Update calls.
* subversion/libsvn_wc/tree_conflicts.c
(svn_wc__del_tree_conflict): Update calls.
* subversion/libsvn_wc/wc_db.h
(svn_wc__db_op_mark_resolved): Add a 'resolve_prop' parameter.
* subversion/libsvn_wc/wc_db.c
(db_op_mark_resolved,
svn_wc__db_op_mark_resolved): Add a 'resolve_prop' parameter and update calls.
--This line, and those below, will be ignored--
Index: subversion/libsvn_wc/conflicts.c
===================================================================
--- subversion/libsvn_wc/conflicts.c (revision 1478018)
+++ subversion/libsvn_wc/conflicts.c (working copy)
@@ -1884,12 +1884,13 @@ read_tree_conflict_desc(svn_wc_conflict_
(*desc)->action = incoming_change;
return SVN_NO_ERROR;
}
+/* ### THIS ONE IS THE ONE CALLED BY UPDATE... NEEDS WORK? */
svn_error_t *
svn_wc__conflict_invoke_resolver(svn_wc__db_t *db,
const char *local_abspath,
const svn_skel_t *conflict_skel,
const apr_array_header_t *merge_options,
svn_wc_conflict_resolver_func2_t resolver_func,
@@ -1931,13 +1932,12 @@ svn_wc__conflict_invoke_resolver(svn_wc_
apr_hash_t *mine_props;
apr_hash_t *their_props;
apr_hash_t *old_their_props;
apr_hash_t *conflicted;
apr_pool_t *iterpool;
apr_hash_index_t *hi;
- svn_boolean_t mark_resolved = TRUE;
SVN_ERR(svn_wc__conflict_read_prop_conflict(NULL,
&mine_props,
&old_their_props,
&their_props,
&conflicted,
@@ -1983,20 +1983,16 @@ svn_wc__conflict_invoke_resolver(svn_wc_
their_props
? svn_hash_gets(their_props, propname)
: NULL,
resolver_func, resolver_baton,
iterpool));
- if (conflict_remains)
- mark_resolved = FALSE;
- }
-
- if (mark_resolved)
- {
- SVN_ERR(svn_wc__mark_resolved_prop_conflicts(db, local_abspath,
- scratch_pool));
+ if (! conflict_remains)
+ SVN_ERR(svn_wc__mark_resolved_prop_conflicts(db, local_abspath,
+ propname,
+ scratch_pool));
}
svn_pool_destroy(iterpool);
}
if (text_conflicted)
{
@@ -2492,12 +2488,19 @@
}
/*
* Resolve the property conflicts found in DB/LOCAL_ABSPATH/CONFLICTS
* according to CONFLICT_CHOICE. (Don't mark it as resolved.)
*
+ * CONFLICT_CHOICE must be one of:
+ *
+ * base --
+ * mine_conflict/mine_full -- use the value from 'mine'
+ * theirs_conflict/theirs_full -- use the value from 'theirs'
+ * merged -- keep the current value
+ *
* If there was a reject file recorded and present on disk, append to
* *WORK_ITEMS a work item to remove it, and set *REMOVED_REJECT_FILE
* to TRUE. Otherwise, don't change *REMOVED_REJECT_FILE.
*
* It is an error if there is no prop conflict.
*
@@ -2534,12 +2537,13 @@ resolve_prop_conflict_on_node(svn_boolea
svn_skel_t **work_items,
svn_wc__db_t *db,
const char *local_abspath,
svn_wc_operation_t operation,
svn_skel_t *conflicts,
svn_wc_conflict_choice_t conflict_choice,
+ const char *resolve_prop,
apr_pool_t *scratch_pool)
{
const char *prop_reject_file;
apr_hash_t *mine_props;
apr_hash_t *their_old_props;
apr_hash_t *their_props;
@@ -2601,17 +2605,19 @@ resolve_prop_conflict_on_node(svn_boolea
for (hi = apr_hash_first(scratch_pool, conflicted_props);
hi;
hi = apr_hash_next(hi))
{
const char *propname = svn__apr_hash_index_key(hi);
- svn_string_t *new_value = NULL;
- new_value = svn_hash_gets(resolve_from, propname);
+ if (*resolve_prop == '\0' || strcmp(resolve_prop, propname) == 0)
+ {
+ svn_string_t *new_value = svn_hash_gets(resolve_from, propname);
- svn_hash_sets(actual_props, propname, new_value);
+ svn_hash_sets(actual_props, propname, new_value);
+ }
}
SVN_ERR(svn_wc__db_op_set_props(db, local_abspath, actual_props,
FALSE, NULL, NULL,
scratch_pool));
}
@@ -2783,13 +2789,13 @@ resolve_tree_conflict_on_node(svn_skel_t
*/
static svn_error_t *
resolve_conflict_on_node(svn_boolean_t *did_resolve,
svn_wc__db_t *db,
const char *local_abspath,
svn_boolean_t resolve_text,
- svn_boolean_t resolve_props,
+ const char *resolve_prop,
svn_boolean_t resolve_tree,
svn_wc_conflict_choice_t conflict_choice,
svn_wc_notify_func2_t notify_func,
void *notify_baton,
svn_cancel_func_t cancel_func,
void *cancel_baton,
@@ -2819,17 +2825,18 @@ resolve_conflict_on_node(svn_boolean_t *
SVN_ERR(resolve_text_conflict_on_node(did_resolve, &work_items,
db, local_abspath,
operation, conflicts,
conflict_choice,
scratch_pool));
- if (resolve_props && prop_conflicted)
+ if (resolve_prop && prop_conflicted)
SVN_ERR(resolve_prop_conflict_on_node(did_resolve, &work_items,
db, local_abspath,
operation, conflicts,
conflict_choice,
+ resolve_prop,
scratch_pool));
if (resolve_tree)
{
SVN_ERR(resolve_tree_conflict_on_node(&work_items,
db, local_abspath,
@@ -2838,16 +2845,16 @@ resolve_conflict_on_node(svn_boolean_t *
notify_func, notify_baton,
cancel_func, cancel_baton,
scratch_pool));
*did_resolve = TRUE;
}
- if (resolve_text || resolve_props || resolve_tree)
+ if (resolve_text || resolve_prop || resolve_tree)
{
SVN_ERR(svn_wc__db_op_mark_resolved(db, local_abspath,
- resolve_text, resolve_props,
+ resolve_text, resolve_prop,
resolve_tree, work_items,
scratch_pool));
/* Run the work queue to remove conflict marker files. */
SVN_ERR(svn_wc__wq_run(db, local_abspath,
cancel_func, cancel_baton,
@@ -2866,32 +2873,33 @@ svn_wc__mark_resolved_text_conflict(svn_
svn_boolean_t ignored_result;
return svn_error_trace(resolve_conflict_on_node(
&ignored_result,
db, local_abspath,
TRUE /* resolve_text */,
- FALSE /* resolve_props */,
+ NULL /* resolve_prop */,
FALSE /* resolve_tree */,
svn_wc_conflict_choose_merged,
NULL, NULL, /* notify_func */
NULL, NULL, /* cancel_func */
scratch_pool));
}
svn_error_t *
svn_wc__mark_resolved_prop_conflicts(svn_wc__db_t *db,
const char *local_abspath,
+ const char *resolve_prop,
apr_pool_t *scratch_pool)
{
svn_boolean_t ignored_result;
return svn_error_trace(resolve_conflict_on_node(
&ignored_result,
db, local_abspath,
FALSE /* resolve_text */,
- TRUE /* resolve_props */,
+ resolve_prop,
FALSE /* resolve_tree */,
svn_wc_conflict_choose_merged,
NULL, NULL, /* notify_func */
NULL, NULL, /* cancel_func */
scratch_pool));
}
@@ -2944,12 +2952,30 @@ conflict_status_walker(void *baton,
const svn_wc_conflict_description2_t *cd;
svn_boolean_t did_resolve;
svn_wc_conflict_choice_t my_choice = cswb->conflict_choice;
cd = APR_ARRAY_IDX(conflicts, i, const svn_wc_conflict_description2_t *);
+ switch (cd->kind)
+ {
+ case svn_wc_conflict_kind_text:
+ if (! cswb->resolve_text)
+ continue;
+ break;
+ case svn_wc_conflict_kind_property:
+ if (! cswb->resolve_prop
+ || (*cswb->resolve_prop != '\0'
+ && strcmp(cswb->resolve_prop, cd->property_name) != 0))
+ continue;
+ break;
+ case svn_wc_conflict_kind_tree:
+ if (! cswb->resolve_tree)
+ continue;
+ break;
+ }
+
svn_pool_clear(iterpool);
if (my_choice == svn_wc_conflict_choose_unspecified)
{
svn_wc_conflict_result_t *result;
@@ -2969,39 +2995,34 @@ conflict_status_walker(void *baton,
if (my_choice == svn_wc_conflict_choose_postpone)
continue;
switch (cd->kind)
{
case svn_wc_conflict_kind_tree:
- if (!cswb->resolve_tree)
- break;
SVN_ERR(resolve_conflict_on_node(&did_resolve,
db,
local_abspath,
FALSE /* resolve_text */,
- FALSE /* resolve_props */,
+ NULL /* resolve_prop */,
TRUE /* resolve_tree */,
my_choice,
cswb->notify_func,
cswb->notify_baton,
cswb->cancel_func,
cswb->cancel_baton,
iterpool));
resolved = TRUE;
break;
case svn_wc_conflict_kind_text:
- if (!cswb->resolve_text)
- break;
-
SVN_ERR(resolve_conflict_on_node(&did_resolve,
db,
local_abspath,
TRUE /* resolve_text */,
- FALSE /* resolve_props */,
+ NULL /* resolve_prop */,
FALSE /* resolve_tree */,
my_choice,
cswb->notify_func,
cswb->notify_baton,
cswb->cancel_func,
cswb->cancel_baton,
@@ -3009,30 +3030,17 @@ conflict_status_walker(void *baton,
if (did_resolve)
resolved = TRUE;
break;
case svn_wc_conflict_kind_property:
- if (!cswb->resolve_prop)
- break;
-
- /* ### this is bogus. resolve_conflict_on_node() does not handle
- ### individual property resolution. */
- if (*cswb->resolve_prop != '\0' &&
- strcmp(cswb->resolve_prop, cd->property_name) != 0)
- {
- break; /* Skip this property conflict */
- }
-
-
- /* We don't have property name handling here yet :( */
SVN_ERR(resolve_conflict_on_node(&did_resolve,
db,
local_abspath,
FALSE /* resolve_text */,
- TRUE /* resolve_props */,
+ cswb->resolve_prop,
FALSE /* resolve_tree */,
my_choice,
cswb->notify_func,
cswb->notify_baton,
cswb->cancel_func,
cswb->cancel_baton,
Index: subversion/libsvn_wc/conflicts.h
===================================================================
--- subversion/libsvn_wc/conflicts.h (revision 1478018)
+++ subversion/libsvn_wc/conflicts.h (working copy)
@@ -233,13 +233,13 @@
svn_wc_conflict_reason_t local_change,
svn_wc_conflict_action_t incoming_change,
const char *move_src_op_root_abspath,
apr_pool_t *result_pool,
apr_pool_t *scratch_pool);
-/* Allows resolving specific conflicts stored in CONFLICT_SKEL.
+/* Remove specific conflicts from CONFLICT_SKEL.
When RESOLVE_TEXT is TRUE and CONFLICT_SKEL contains a text conflict,
resolve/remove the text conflict in CONFLICT_SKEL.
When RESOLVE_PROP is "" and CONFLICT_SKEL contains a property conflict,
resolve/remove all property conflicts in CONFLICT_SKEL.
@@ -250,13 +250,13 @@ svn_wc__conflict_skel_resolve(
CONFLICT_SKEL.
When RESOLVE_TREE is TRUE and CONFLICT_SKEL contains a tree conflict,
resolve/remove the tree conflict in CONFLICT_SKEL.
If COMPLETELY_RESOLVED is not NULL, then set *COMPLETELY_RESOLVED to TRUE,
- when no conflict registration is left in CONFLICT_SKEL after editting,
+ when no conflict registration is left in CONFLICT_SKEL after editing,
otherwise to FALSE.
Allocate data stored in the skel in RESULT_POOL.
This functions edits CONFLICT_SKEL. New skels might be created in
RESULT_POOL. Temporary allocations will use SCRATCH_POOL.
@@ -427,16 +427,20 @@
/* Mark as resolved any text conflict on the node at DB/LOCAL_ABSPATH. */
svn_error_t *
svn_wc__mark_resolved_text_conflict(svn_wc__db_t *db,
const char *local_abspath,
apr_pool_t *scratch_pool);
-/* Mark as resolved any prop conflicts on the node at DB/LOCAL_ABSPATH. */
+/* Mark as resolved any prop conflicts on the node at DB/LOCAL_ABSPATH.
+ * If RESOLVE_PROP is "", mark resolved all property conflicts on the node;
+ * if RESOLVE_PROP is not NULL and not "", mark resolved just this property.
+ */
svn_error_t *
svn_wc__mark_resolved_prop_conflicts(svn_wc__db_t *db,
const char *local_abspath,
+ const char *resolve_prop,
apr_pool_t *scratch_pool);
#ifdef __cplusplus
}
#endif /* __cplusplus */
Index: subversion/libsvn_wc/questions.c
===================================================================
--- subversion/libsvn_wc/questions.c (revision 1478018)
+++ subversion/libsvn_wc/questions.c (working copy)
@@ -503,13 +503,13 @@ internal_conflicted_p(svn_boolean_t *tex
/* The marker files are missing, so "repair" wc.db if we can */
SVN_ERR(svn_wc__db_wclock_owns_lock(&own_lock, db, local_abspath, FALSE,
scratch_pool));
if (own_lock)
SVN_ERR(svn_wc__db_op_mark_resolved(db, local_abspath,
resolved_text,
- resolved_props,
+ resolved_props ? "" : NULL,
FALSE /* resolved_tree */,
NULL /* work_items */,
scratch_pool));
}
return SVN_NO_ERROR;
Index: subversion/libsvn_wc/tree_conflicts.c
===================================================================
--- subversion/libsvn_wc/tree_conflicts.c (revision 1478018)
+++ subversion/libsvn_wc/tree_conflicts.c (working copy)
@@ -390,13 +390,13 @@ svn_wc__del_tree_conflict(svn_wc_context
const char *victim_abspath,
apr_pool_t *scratch_pool)
{
SVN_ERR_ASSERT(svn_dirent_is_absolute(victim_abspath));
SVN_ERR(svn_wc__db_op_mark_resolved(wc_ctx->db, victim_abspath,
- FALSE, FALSE, TRUE, NULL,
+ FALSE, NULL, TRUE, NULL,
scratch_pool));
return SVN_NO_ERROR;
}
svn_error_t *
Index: subversion/libsvn_wc/wc_db.c
===================================================================
--- subversion/libsvn_wc/wc_db.c (revision 1478018)
+++ subversion/libsvn_wc/wc_db.c (working copy)
@@ -6161,13 +6161,13 @@ svn_wc__db_op_mark_conflict(svn_wc__db_t
*/
static svn_error_t *
db_op_mark_resolved(svn_wc__db_wcroot_t *wcroot,
const char *local_relpath,
svn_wc__db_t *db,
svn_boolean_t resolved_text,
- svn_boolean_t resolved_props,
+ const char *resolved_prop,
svn_boolean_t resolved_tree,
const svn_skel_t *work_items,
apr_pool_t *scratch_pool)
{
svn_sqlite__stmt_t *stmt;
svn_boolean_t have_row;
@@ -6211,13 +6211,13 @@ db_op_mark_resolved(svn_wc__db_wcroot_t
conflicts = svn_skel__parse(conflict_data, conflict_len, scratch_pool);
SVN_ERR(svn_sqlite__reset(stmt));
SVN_ERR(svn_wc__conflict_skel_resolve(&resolved_all, conflicts,
db, wcroot->abspath,
resolved_text,
- resolved_props ? "" : NULL,
+ resolved_prop,
resolved_tree,
scratch_pool, scratch_pool));
SVN_ERR(svn_sqlite__get_statement(&stmt, wcroot->sdb,
STMT_UPDATE_ACTUAL_CONFLICT));
SVN_ERR(svn_sqlite__bindf(stmt, "is", wcroot->wc_id, local_relpath));
@@ -6247,13 +6247,13 @@ db_op_mark_resolved(svn_wc__db_wcroot_t
}
svn_error_t *
svn_wc__db_op_mark_resolved(svn_wc__db_t *db,
const char *local_abspath,
svn_boolean_t resolved_text,
- svn_boolean_t resolved_props,
+ const char *resolved_prop,
svn_boolean_t resolved_tree,
const svn_skel_t *work_items,
apr_pool_t *scratch_pool)
{
svn_wc__db_wcroot_t *wcroot;
const char *local_relpath;
@@ -6263,13 +6263,13 @@ svn_wc__db_op_mark_resolved(svn_wc__db_t
SVN_ERR(svn_wc__db_wcroot_parse_local_abspath(&wcroot, &local_relpath, db,
local_abspath, scratch_pool, scratch_pool));
VERIFY_USABLE_WCROOT(wcroot);
SVN_WC__DB_WITH_TXN(
db_op_mark_resolved(wcroot, local_relpath, db,
- resolved_text, resolved_props, resolved_tree,
+ resolved_text, resolved_prop, resolved_tree,
work_items, scratch_pool),
wcroot);
SVN_ERR(flush_entries(wcroot, local_abspath, svn_depth_empty, scratch_pool));
return SVN_NO_ERROR;
}
Index: subversion/libsvn_wc/wc_db.h
===================================================================
--- subversion/libsvn_wc/wc_db.h (revision 1478018)
+++ subversion/libsvn_wc/wc_db.h (working copy)
@@ -1593,13 +1593,13 @@ svn_wc__db_op_mark_conflict(svn_wc__db_t
### I'm not sure that these three values are the best way to do this,
### but they're handy for now. */
svn_error_t *
svn_wc__db_op_mark_resolved(svn_wc__db_t *db,
const char *local_abspath,
svn_boolean_t resolved_text,
- svn_boolean_t resolved_props,
+ const char *resolved_prop,
svn_boolean_t resolved_tree,
const svn_skel_t *work_items,
apr_pool_t *scratch_pool);
/* Revert all local changes which are being maintained in the database,
