Hi Stefan!
A few questions and a WIP patch for making the patch code deal with
property targets being dirs.
* How should we deal with SVN_ERR_ILLEGAL_TARGET? svn_wc_prop_set()
throws one of those if we for instance try to set svn:executable on a
dir or svn:ignore on a file. Should we do some kind of skipped prop
notification?
* Are you ok with the change I made in resolve_target_path()? If we only
have property changes to a file than it's ok to have a dir as a
target.
* What should we do if the target dir for a property is locally_deleted?
I think we should skip it (that's what the code does right now).
I said to you earlier on IRC that I didn't quite understand how we
determined if we had a valid patch target. Do you agree with my doc
change for resolve_target_path() and the two sections below?
* Do you agree on this definition:
! target->skipped => We have an add/del/mod for target
target->node_kind_on_disk => We have a mod for target. (special case:
if we're adding to an existing file we might have an add if the target
already is the expected result after patching).
* For properties I have:
! target->skipped && is_prop_hunk => We have an add/del/mod for
prop_target.
prop_target->content_info->stream => We have a mod for prop_target.
My recent work has been an attempt to make the matching/applying part as
generic as possible, e.g. work for both properties and texts. My limited
testing suggests I've succeeded with that. But I'll have to introduce a
couple of if statements and flags to distinguish between props and text
changes.
[[[
Enable the patch code to use dirs as targets if we only have property
changes.
* subversion/libsvn_client/patch.c
(patch_target_t): Add fields 'has_text_changes' and 'has_prop_changes'
to be able to decide if we should install tmp files for text and/or
props. It's needed since we may have a dir as a target for a
property. If we'd try to copy the tmp file for the text changes onto
a dir we get an error.
(resolve_target_path): Add new parameter 'only_prop_changes' to help
us determine the case when a dir is a valid target.
(init_patch_target): Update caller of resolve_target_path().
(apply_hunk): Record if we've done any changes to props or text
content.
(apply_patches): Only call install_patched_target() if we have changed
the text contents.
]]]
Thanks,
Daniel
Index: subversion/libsvn_client/patch.c
===================================================================
--- subversion/libsvn_client/patch.c (revision 979179)
+++ subversion/libsvn_client/patch.c (arbetskopia)
@@ -186,6 +186,12 @@ typedef struct patch_target_t {
/* True if the target has the executable bit set. */
svn_boolean_t executable;
+ /* True if the patch changes the text of the target */
+ svn_boolean_t has_text_changes;
+
+ /* True if the patch changes any of the properties of the target */
+ svn_boolean_t has_prop_changes;
+
/* All the information that is specifict to the content of the target. */
target_content_info_t *content_info;
@@ -303,6 +309,9 @@ obtain_eol_and_keywords_for_file(apr_hash_t **keyw
* If possible, determine TARGET->WC_PATH, TARGET->ABS_PATH, TARGET->KIND,
* TARGET->ADDED, and TARGET->PARENT_DIR_EXISTS.
* Indicate in TARGET->SKIPPED whether the target should be skipped.
+ * The target should be skipped if the versioned path does not exist or does
+ * not match our expected type, e.g. it should be a file unless we're
+ * dealing with a patch that has ONLY_PROP_CHANGES.
* STRIP_COUNT specifies the number of leading path components
* which should be stripped from target paths in the patch.
* Use RESULT_POOL for allocations of fields in TARGET.
@@ -312,6 +321,7 @@ resolve_target_path(patch_target_t *target,
const char *path_from_patchfile,
const char *local_abspath,
int strip_count,
+ svn_boolean_t only_prop_changes,
svn_wc_context_t *wc_ctx,
apr_pool_t *result_pool,
apr_pool_t *scratch_pool)
@@ -341,6 +351,8 @@ resolve_target_path(patch_target_t *target,
{
target->local_relpath = svn_dirent_is_child(local_abspath, stripped_path,
result_pool);
+
+ /* ### We need to allow setting props on the wc root dir */
if (! target->local_relpath)
{
/* The target path is either outside of the working copy
@@ -403,7 +415,9 @@ resolve_target_path(patch_target_t *target,
}
SVN_ERR(svn_wc_read_kind(&target->db_kind, wc_ctx, target->local_abspath,
FALSE, scratch_pool));
- if (target->db_kind == svn_node_dir)
+
+ /* We allow properties to have dirs as targets */
+ if (target->db_kind == svn_node_dir && ! only_prop_changes)
{
/* ### We cannot yet replace a locally deleted dir with a file,
* ### but some day we might want to allow it. */
@@ -502,6 +516,7 @@ init_patch_target(patch_target_t **patch_target,
{
patch_target_t *target;
target_content_info_t *content_info;
+ svn_boolean_t only_prop_changes = patch->hunks->nelts == 0 ? TRUE : FALSE;
content_info = apr_pcalloc(result_pool, sizeof(*content_info));
@@ -525,8 +540,8 @@ init_patch_target(patch_target_t **patch_target,
target->pool = result_pool;
SVN_ERR(resolve_target_path(target, patch->new_filename,
- base_dir, strip_count, wc_ctx,
- result_pool, scratch_pool));
+ base_dir, strip_count, only_prop_changes,
+ wc_ctx, result_pool, scratch_pool));
if (! target->skipped)
{
const char *diff_header;
@@ -1350,6 +1365,11 @@ apply_hunk(patch_target_t *target, target_content_
while (! eof);
svn_pool_destroy(iterpool);
+ if (is_prop_hunk)
+ target->has_prop_changes = TRUE;
+ else
+ target->has_text_changes = TRUE;
+
return SVN_NO_ERROR;
}
@@ -2328,9 +2348,14 @@ apply_patches(void *baton,
patch_target_info_t *) = target_info;
if (! target->skipped)
- SVN_ERR(install_patched_target(target, btn->abs_wc_path,
- btn->ctx, btn->dry_run,
- iterpool));
+ {
+ if (target->has_text_changes)
+ SVN_ERR(install_patched_target(target, btn->abs_wc_path,
+ btn->ctx, btn->dry_run,
+ iterpool));
+ if (target->has_prop_changes)
+ ; /* ### Here we'll going to call install_patched_prop_target() */
+ }
SVN_ERR(send_patch_notification(target, btn->ctx, iterpool));
}
