Daniel Shahaf wrote on Fri, Feb 25, 2022 at 15:18:44 +0000:
> Nathan Hartman wrote on Thu, 24 Feb 2022 23:44 +00:00:
> > On Thu, Feb 24, 2022 at 2:33 PM Mark Phippard <markp...@gmail.com> wrote:
> >> Maybe merge should just refuse to run at all if it detects any
> >> svn:needs-lock properties in the WC?
> >
> > Then it would never be able to run; I think you meant: refuse to run
> > if we don't hold the lock on a file that has the svn:needs-lock
> > property. However, it would be rather irritating if the refusal to run
> > were caused by files unaffected by the merge.
>
> Or if the user deliberately did the merge without intending to commit it.
>
> How about making this situation a conflict? "local file not locked,
> incoming edit upon merge". Then the file is not silently changed, but
> the user can use --accept or «svn resolve» to make the changes anyway if
> they know that's what they want.
Is it just this?
[[[
Index: subversion/include/svn_wc.h
===================================================================
--- subversion/include/svn_wc.h (revision 1898427)
+++ subversion/include/svn_wc.h (working copy)
@@ -1657,7 +1657,10 @@ typedef enum svn_wc_conflict_reason_t
/** Object is moved away. @since New in 1.8. */
svn_wc_conflict_reason_moved_away,
/** Object is moved here. @since New in 1.8. */
- svn_wc_conflict_reason_moved_here
+ svn_wc_conflict_reason_moved_here,
+ /** #SVN_PROP_NEEDS_LOCK was set and the file wasn't locked.
+ * @since New in 1.15. */
+ svn_wc_conflict_reason_not_locked
} svn_wc_conflict_reason_t;
Index: subversion/libsvn_client/conflicts.c
===================================================================
--- subversion/libsvn_client/conflicts.c (revision 1898427)
+++ subversion/libsvn_client/conflicts.c (working copy)
@@ -244,6 +244,7 @@ static const svn_token_map_t map_conflict_reason[]
{ "unversioned", svn_wc_conflict_reason_unversioned },
{ "moved-away", svn_wc_conflict_reason_moved_away },
{ "moved-here", svn_wc_conflict_reason_moved_here },
+ { "not-locked", svn_wc_conflict_reason_not_locked },
{ NULL, 0 }
};
@@ -1327,6 +1328,11 @@ describe_local_file_node_change(const char **descr
}
break;
}
+ case svn_wc_conflict_reason_not_locked:
+ {
+ *description = _("hello world");
+ break;
+ }
}
return SVN_NO_ERROR;
@@ -1477,8 +1483,8 @@ describe_local_dir_node_change(const char **descri
scratch_pool));
}
}
- }
- break;
+ }
+ break;
case svn_wc_conflict_reason_moved_here:
{
const char *moved_from_abspath;
@@ -1552,6 +1558,10 @@ describe_local_dir_node_change(const char **descri
}
}
}
+ break;
+ case svn_wc_conflict_reason_not_locked:
+ *description = _("hello world");
+ break;
}
return SVN_NO_ERROR;
@@ -3168,6 +3178,9 @@ describe_local_none_node_change(const char **descr
case svn_wc_conflict_reason_moved_here:
*description = _("An item had been moved here in the working copy "
"at the time this conflict was recorded.");
+ break;
+ case svn_wc_conflict_reason_not_locked:
+ *description = _("hello world");
break;
}
Index: subversion/libsvn_wc/conflicts.c
===================================================================
--- subversion/libsvn_wc/conflicts.c (revision 1898427)
+++ subversion/libsvn_wc/conflicts.c (working copy)
@@ -520,6 +520,7 @@ static const svn_token_map_t reason_map[] =
{ "replaced", svn_wc_conflict_reason_replaced },
{ "moved-away", svn_wc_conflict_reason_moved_away },
{ "moved-here", svn_wc_conflict_reason_moved_here },
+ { "not-locked", svn_wc_conflict_reason_not_locked },
{ NULL }
};
Index: subversion/libsvn_wc/merge.c
===================================================================
--- subversion/libsvn_wc/merge.c (revision 1898427)
+++ subversion/libsvn_wc/merge.c (working copy)
@@ -24,6 +24,7 @@
#include "svn_wc.h"
#include "svn_diff.h"
#include "svn_dirent_uri.h"
+#include "svn_hash.h"
#include "svn_path.h"
#include "svn_pools.h"
#include "svn_props.h"
@@ -1141,6 +1142,31 @@ svn_wc__internal_merge(svn_skel_t **work_items,
SVN_ERR(maybe_update_target_eols(&left_abspath, prop_diff, left_abspath,
cancel_func, cancel_baton,
scratch_pool, scratch_pool));
+
+ {
+ svn_wc__db_lock_t *lock;
+ apr_hash_t *props;
+
+ /* TODO: short-circuit one of the two calls */
+ SVN_ERR(svn_wc__db_read_props(&props, db, target_abspath,
+ result_pool, scratch_pool));
+ SVN_ERR(svn_wc__db_lock_get(&lock, db, wri_abspath, target_abspath,
+ result_pool, scratch_pool));
+
+ if (svn_hash_gets(props, SVN_PROP_NEEDS_LOCK) && NULL == lock)
+ {
+ if (!*conflict_skel)
+ *conflict_skel = svn_wc__conflict_skel_create(result_pool);
+ }
+ SVN_ERR(svn_wc__conflict_skel_add_tree_conflict(*conflict_skel,
+ db,
+ wri_abspath,
+
svn_wc_conflict_reason_not_locked,
+
svn_wc_conflict_action_edit,
+ NULL, NULL,
+ result_pool,
scratch_pool));
+ /* ### TODO notification? */
+ }
SVN_ERR(merge_file_trivial(work_items, merge_outcome,
left_abspath, right_abspath,
Index: subversion/libsvn_wc/tree_conflicts.c
===================================================================
--- subversion/libsvn_wc/tree_conflicts.c (revision 1898427)
+++ subversion/libsvn_wc/tree_conflicts.c (working copy)
@@ -82,6 +82,7 @@ const svn_token_map_t svn_wc__conflict_reason_map[
{ "unversioned", svn_wc_conflict_reason_unversioned },
{ "moved-away", svn_wc_conflict_reason_moved_away },
{ "moved-here", svn_wc_conflict_reason_moved_here },
+ { "not-locked", svn_wc_conflict_reason_not_locked },
{ NULL }
};
Index: subversion/svn/cl-conflicts.c
===================================================================
--- subversion/svn/cl-conflicts.c (revision 1898427)
+++ subversion/svn/cl-conflicts.c (working copy)
@@ -55,6 +55,7 @@ static const svn_token_map_t map_conflict_reason_x
{ "unversioned", svn_wc_conflict_reason_unversioned },
{ "moved-away", svn_wc_conflict_reason_moved_away },
{ "moved-here", svn_wc_conflict_reason_moved_here },
+ { "not-locked", svn_wc_conflict_reason_not_locked },
{ NULL, 0 }
};
@@ -91,6 +92,8 @@ local_reason_str(svn_node_kind_t kind, svn_wc_conf
return _("local file moved away");
case svn_wc_conflict_reason_moved_here:
return _("local file moved here");
+ case svn_wc_conflict_reason_not_locked:
+ return _("local file not locked");
}
break;
case svn_node_dir:
@@ -117,6 +120,8 @@ local_reason_str(svn_node_kind_t kind, svn_wc_conf
return _("local dir moved away");
case svn_wc_conflict_reason_moved_here:
return _("local dir moved here");
+ case svn_wc_conflict_reason_not_locked:
+ return _("local dir not locked"); /* Can't happen, surely? */
}
break;
case svn_node_none:
@@ -144,6 +149,8 @@ local_reason_str(svn_node_kind_t kind, svn_wc_conf
return _("local moved away");
case svn_wc_conflict_reason_moved_here:
return _("local moved here");
+ case svn_wc_conflict_reason_not_locked:
+ return _("local not locked");
}
break;
}
Index: subversion/tests/libsvn_wc/op-depth-test.c
===================================================================
--- subversion/tests/libsvn_wc/op-depth-test.c (revision 1898427)
+++ subversion/tests/libsvn_wc/op-depth-test.c (working copy)
@@ -389,6 +389,7 @@ print_conflict(const conflict_info_t *row,
CASE_ENUM_STRVAL(reason, svn_wc_conflict_reason_replaced);
CASE_ENUM_STRVAL(reason, svn_wc_conflict_reason_moved_away);
CASE_ENUM_STRVAL(reason, svn_wc_conflict_reason_moved_here);
+ CASE_ENUM_STRVAL(reason, svn_wc_conflict_reason_not_locked);
default:
SVN_ERR_MALFUNCTION_NO_RETURN();
}
]]]
I'm not sure why the string "not-locked" needs to be repeated 4 times.
I see one of the copies is for «svn info --xml», but I don't know why
the other three aren't a violation of DRY.
Tested manually with this:
[[[
#!/bin/sh
: ${svn:=svn}
rm -rf r wc
svnadmin create r
$svn -q co file://$(pwd)/r wc
cd wc
$svn -q mkdir A
echo "This is the file 'mu'." > A/mu
$svn -q add A/mu
$svn -q ps svn:needs-lock yes A/mu
$svn -q ci -m 'r1: add with property'
$svn -q up
$svn -q cp A A2
$svn -q ci -m 'r2: create branch'
$svn -q up
$svn -q lock A2/mu
echo "modified on branch" >> A2/mu
$svn -q ci -m 'r3: mod on branch'
$svn -q up
$svn merge -c 3 A2 A
$svn status A/mu
$svn info A/mu | tail -4
]]]
[[[
% ./repro.sh
--- Merging r3 into 'A':
U A/mu
--- Recording mergeinfo for merge of r3 into 'A':
U A
M C A/mu
> local file not locked, incoming file edit upon merge
Summary of conflicts:
Tree conflicts: 1
Path: A/mu
Name: mu
Tree conflict: local file not locked, incoming file edit upon merge
Source left: (file) ^/A2/mu@2
Source right: (file) ^/A2/mu@3
%
]]]
The libsvn_wc changes might not be entirely right. For one, The merge
should output a 'C' notification (a TODO comment is included in the
patch for this). Also, «svn resolve» smartness is not included, nor are
unit tests.
This isn't really my itch, though, so anyone interested in seeing this
committed, etc., please jump in and take over from here.
Cheers,
Daniel
