Charles, I've been staring at this for a while... it seems to me that
this deals with move operations at the expense of breaking simple
deletes. It looks like it will keep-local *any* delete, whether or
not there is an add-with-history following it... and the actual file
removal won't happen unless the add-with-history happens.
Also, what if there are two new files add-with-history'd from the same old file?
$ svn cp foo foocopy
$ svn mv foo bar
$ svn diff --patch
seems like 'foo' will be deleted as soon as the first copy is seen.
...
on further reading, it looks like the effect of applying this patch
$ svn cp foo foocopy
$ svn cp foo foocopy2
$ svn diff --patch
will be to delete foo! Yeah, a single hash to denote "we're delaying
removing this file" and "we've already copied from this file" won't
work.
A possible better approach is to keep_local *all* file deletions but
add the file names to a hash, and then after the entire merge is done,
delete all of the pending deletions?
(Or to join forces with dlr/sussman's work.)
Some other random notes follow, though in general it does seem like
this revision should probably be redone and so some of my comments are
moot.
On 10/5/07, cacknin@tigris.org <cacknin@tigris.org> wrote:
> Author: cacknin
> Date: Fri Oct 5 10:09:58 2007
> New Revision: 26953
>
> Log:
> Fix copyfrom-path use for file move operations when applying a
> WC->WC-created svnpatch.
>
> * subversion/libsvn_client/patch.c
> (patch_cmd_baton): add deletions hash member.
> (is_removal_safe): new helper function.
> (merge_file_added): make use of the new deletions hash and
> is_removal_safe function to decide, when this involves a move, whether
> the source can be deleted or not.
> (merge_file_deleted): turn on the keep_local feature of
> svn_client__wc_delete by default and make use of the new deletions
> hash and is_removal_safe function to decide whether the file can be
> deleted from disk.
> (svn_client_patch): allocate/initialize the deletions hash table
> upon patch_cmd_baton initialization.
>
>
> Modified:
> branches/svnpatch-diff/subversion/libsvn_client/patch.c
>
> Modified: branches/svnpatch-diff/subversion/libsvn_client/patch.c
> URL: http://svn.collab.net/viewvc/svn/branches/svnpatch-diff/subversion/libsvn_client/patch.c?pathrev=26953&r1=26952&r2=26953
> ==============================================================================
> --- branches/svnpatch-diff/subversion/libsvn_client/patch.c (original)
> +++ branches/svnpatch-diff/subversion/libsvn_client/patch.c Fri Oct 5 10:09:58 2007
> @@ -77,6 +77,10 @@
> * dry_run mode. */
> apr_hash_t *dry_run_deletions;
>
> + /* The list of paths of schedule-delete entries. See is_removal_safe
It's not a list (lists are ordered).
> + * docstring. */
> + apr_hash_t *deletions;
> +
From your later email it sounds like this hash is doing double-duty.
I think it would be clearer to split it into two hashes, one for
delete-before-add and one for add-before-delete.
> apr_pool_t *pool;
> };
>
> @@ -92,6 +96,19 @@
> APR_HASH_KEY_STRING) != NULL);
> }
>
> +/* Helper function. When PATH is found in the deletions hash, it is
> + * assumed the file can be removed from disk. When patching -- i.e.
> + * reading serialiazed Editor Commands --, a move operation is either
"serialized"
> + * represented with an add-file with copy-path and a delete-entry, or
> + * the other way around. When the latter, we need to defer the file
> + * on-disk removal (use of the keep-local feature) so that the add-file
> + * command succeeds at copying since it needs the source. */
> +static APR_INLINE svn_boolean_t
> +is_removal_safe(struct patch_cmd_baton *patch_b, const char *path)
> +{
> + return apr_hash_get(patch_b->deletions, path,
> + APR_HASH_KEY_STRING) != NULL;
> +}
I would call this something like "is_pending_deletion" or
"already_copied_from" (since it is doing double duty for these two
concepts). (But as above, "already_copied_from" is a bad heuristic
because a file can be copied to multiple places.)
>
>
> /* A svn_wc_diff_callbacks3_t function. Used for both file and directory
> @@ -383,6 +400,12 @@
> patch_b->ctx->cancel_baton,
> NULL, NULL, /* no notification */
> subpool));
> +
> + if (is_removal_safe(patch_b, copyfrom_path))
> + svn_io_remove_file(copyfrom_path, subpool);
> + else
> + apr_hash_set(patch_b->deletions, copyfrom_path,
> + APR_HASH_KEY_STRING, copyfrom_path);
> }
> else /* schedule-add */
> {
> @@ -491,6 +514,7 @@
> svn_wc_adm_access_t *parent_access;
> const char *parent_path;
> svn_error_t *err;
> + svn_boolean_t keep_local = TRUE;
>
> /* Easy out: if we have no adm_access for the parent directory,
> then this portion of the tree-delta "patch" must be inapplicable.
> @@ -511,10 +535,20 @@
> svn_path_split(mine, &parent_path, NULL, subpool);
> SVN_ERR(svn_wc_adm_retrieve(&parent_access, adm_access, parent_path,
> subpool));
> +
> + if (is_removal_safe(patch_b, mine))
> + keep_local = FALSE;
> + else
> + {
> + const char *file_copy = apr_pstrdup(patch_b->pool, mine);
> + apr_hash_set(patch_b->deletions, file_copy,
> + APR_HASH_KEY_STRING, file_copy);
> + }
> +
> /* Passing NULL for the notify_func and notify_baton because
> delete_entry() will do it for us. */
> err = svn_client__wc_delete(mine, parent_access, patch_b->force,
> - patch_b->dry_run, FALSE, NULL, NULL,
> + patch_b->dry_run, keep_local, NULL, NULL,
> patch_b->ctx, subpool);
> if (err && state)
> {
> @@ -1724,6 +1758,7 @@
> patch_cmd_baton.ctx = ctx;
> patch_cmd_baton.dry_run_deletions = (dry_run ? apr_hash_make(pool)
> : NULL);
> + patch_cmd_baton.deletions = apr_hash_make(pool);
> patch_cmd_baton.pool = pool;
>
> SVN_ERR(svn_wc_adm_open3(&adm_access, NULL, wc_path,
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: svn-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: svn-help@subversion.tigris.org
>
>
--
David Glasser | glasser_at_davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Oct 6 00:47:39 2007