Re: svn commit: r27222 - branches/svnpatch-diff/subversion/libsvn_client

On 10/16/07, cacknin@tigris.org <cacknin@tigris.org> wrote:
> Author: cacknin
> Date: Tue Oct 16 13:02:16 2007
> New Revision: 27222
>
> Log:
> Attempt to fix 'svn patch' copy/move behavior using copyfrom argument
> from WC-WC serialiazed diff.
>
> * subversion/libsvn_client/patch.c:
> (create_empty_file): put a forward declaration at the beginning.
> (add_file_with_history): new function to help merge_file_added in its
> quest for both correctness and clarity.
> (merge_file_added): make use of add_file_with_history and a large
> amount of indent adjustments.
> (merge_file_deleted): prevent svn_client__wc_delete from deleting the
> file when it has local modifications.
>
>
> 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=27222&r1=27221&r2=27222
> ==============================================================================
> --- branches/svnpatch-diff/subversion/libsvn_client/patch.c (original)
> +++ branches/svnpatch-diff/subversion/libsvn_client/patch.c Tue Oct 16 13:02:16 2007
> @@ -59,6 +59,14 @@
> /*-----------------------------------------------------------------------*/
>
>
> +/* Forward declaration to keep things organised. */
> +static svn_error_t *
> +create_empty_file(apr_file_t **file,
> + const char **empty_file_path,
> + svn_wc_adm_access_t *adm_access,
> + svn_io_file_del_t delete_when,
> + apr_pool_t *pool);
> +

create_empty_file doesn't call anything else in this file... maybe
just move it up itself? (Or does that throw off the organization of
the whole file?)

> struct patch_cmd_baton {
> svn_boolean_t force;
> svn_boolean_t dry_run;
> @@ -283,6 +291,85 @@
> return SVN_NO_ERROR;
> }
>
> +/* This is to keep merge_file_added() clean. We're dealing with a
> + * schedule-add-with-history operation where @a copyfrom_path is the
> + * source and @a path the destination. @a adm_access is @a path's
> + * parent access baton. This is either a file move or a file copy.
> + * When the former, what we do depend on whether (a) the source path has
> + * local modifications and (b) the delete-entry call is past or
> + * forthcoming. If the copyfrom file has no local modification and the
> + * delete-entry is past, then it was removed from disk thus we use it's
> + * text-base instead as we're offline. */

It doesn't actually look like the local-mods issues are dealt with
here... or does svn_wc_copy2 handle that? (Is this situation tested?
In general, are any of the corner cases tested?)

> +static svn_error_t *
> +add_file_with_history(const char *path,
> + svn_wc_adm_access_t *adm_access,
> + const char *copyfrom_path,
> + void *patch_baton,
> + apr_pool_t *pool)
> +{
> + svn_node_kind_t copyfrom_kind, copyfrom_textbase_kind;
> + const char *copyfrom_textbase_path;
> + struct patch_cmd_baton *patch_b = patch_baton;
> + const char *path_basename = svn_path_basename(path, pool);
> +
> + SVN_ERR(svn_io_check_path(copyfrom_path, &copyfrom_kind, pool));
> +
> + if (copyfrom_kind == svn_node_none)
> + {
> + SVN_ERR(svn_wc_get_pristine_copy_path(copyfrom_path,
> + &copyfrom_textbase_path,
> + pool));
> + SVN_ERR(svn_io_check_path(copyfrom_textbase_path,
> + &copyfrom_textbase_kind, pool));
> +
> + /* When the text-base is missing we really can't help. */
> + if (copyfrom_textbase_kind == svn_node_none)
> + return svn_error_create(SVN_ERR_WC_COPYFROM_PATH_NOT_FOUND,
> + NULL, NULL);
> + }
> +
> + if (! patch_b->dry_run)
> + {
> + if (copyfrom_kind == svn_node_none)
> + {
> + char *copyfrom_url;
> + const char *copyfrom_src; /* copy of copyfrom's text-base */
> + svn_revnum_t copyfrom_rev;
> + svn_wc_adm_access_t *copyfrom_access;
> +
> + /* Since @c svn_wc_add_repos_file2() *moves* text-base path to
> + * target-path, operate on a copy of it instead. */
> + SVN_ERR(create_empty_file(NULL, &copyfrom_src, adm_access,
> + svn_io_file_del_none, pool));
> + SVN_ERR(svn_io_copy_file(copyfrom_textbase_path, copyfrom_src,
> + FALSE, pool));
> +
> + /* The file we're about to add needs a copyfrom_url along with
> + * a copyfrom_rev, which we both retrieve through the working
> + * copy administrative area of the source file. */
> + SVN_ERR(svn_wc_adm_probe_open3(&copyfrom_access, NULL,
> + copyfrom_path, FALSE, -1,
> + patch_b->ctx->cancel_func,
> + patch_b->ctx->cancel_baton, pool));
> + SVN_ERR(svn_wc_get_ancestry(&copyfrom_url, &copyfrom_rev,
> + copyfrom_path, copyfrom_access, pool));
> + SVN_ERR(svn_wc_add_repos_file2(path, adm_access,
> + copyfrom_src, NULL,
> + apr_hash_make(pool), NULL,

Hmm, looks like you're throwing away all the props there... that can't
be right.

> + copyfrom_url, copyfrom_rev,
> + pool));
> + }
> + else
> + SVN_ERR(svn_wc_copy2(copyfrom_path, adm_access, path_basename,
> + patch_b->ctx->cancel_func,
> + patch_b->ctx->cancel_baton,
> + NULL, NULL, /* no notification */
> + pool));
> + }
> +
> + return SVN_NO_ERROR;
> +}
> +
> /* A svn_wc_diff_callbacks3_t function. */
> static svn_error_t *
> merge_file_added(svn_wc_adm_access_t *adm_access,
> @@ -303,7 +390,6 @@
> {
> struct patch_cmd_baton *patch_b = baton;
> apr_pool_t *subpool = svn_pool_create(patch_b->pool);
> - const char *mine_basename = svn_path_basename(mine, subpool);
> svn_node_kind_t kind;
> int i;
> apr_hash_t *new_props;
> @@ -361,52 +447,45 @@
> return SVN_NO_ERROR;
> }
>
> - if (! patch_b->dry_run)
> + if (copyfrom_path) /* schedule-add-with-history */
> {
> - if (copyfrom_path) /* schedule-add-with-history */
> - {
> - /* Check whether the source we want to copy from exists. */
> - svn_node_kind_t copyfrom_kind;
> - SVN_ERR(svn_io_check_path(copyfrom_path, &copyfrom_kind,
> - subpool));
> -
> - if (copyfrom_kind == svn_node_none)
> - {
> - if (content_state)
> - *content_state = svn_wc_notify_state_source_missing;
> - svn_pool_destroy(subpool);
> - return SVN_NO_ERROR;
> - }
> -
> - SVN_ERR(svn_wc_copy2(copyfrom_path, adm_access, mine_basename,
> - patch_b->ctx->cancel_func,
> - patch_b->ctx->cancel_baton,
> - NULL, NULL, /* no notification */
> - subpool));
> - }
> - else /* schedule-add */
> + svn_error_t *err;
> + err = add_file_with_history(mine, adm_access, copyfrom_path,
> + patch_b, subpool);
> +
> + if (err && err->apr_err == SVN_ERR_WC_COPYFROM_PATH_NOT_FOUND)
> {
> - /* Copy the cached empty file and schedule-add it. The
> - * contents will come in either via apply-textdelta
> - * following calls if this is a binary file or with
> - * unidiff for text files. */
> - SVN_ERR(svn_io_copy_file(yours, mine, TRUE, subpool));
> - SVN_ERR(svn_wc_add2(mine, adm_access, NULL, SVN_IGNORED_REVNUM,
> - patch_b->ctx->cancel_func,
> - patch_b->ctx->cancel_baton,
> - NULL, NULL, /* no notification */
> - subpool));
> + if (content_state)
> + *content_state = svn_wc_notify_state_source_missing;
> + svn_error_clear(err);
> + svn_pool_destroy(subpool);
> + return SVN_NO_ERROR;
> }
> -
> - /* Now regardless of the schedule-add nature, merge properties. */
> - if (prop_changes->nelts > 0)
> - SVN_ERR(merge_props_changed(adm_access, prop_state,
> - mine, prop_changes,
> - original_props, baton));
> - else
> - if (prop_state)
> - *prop_state = svn_wc_notify_state_unchanged;
> + else if (err)
> + return err;
> }
> + else if (! patch_b->dry_run) /* schedule-add */
> + {
> + /* Copy the cached empty file and schedule-add it. The
> + * contents will come in either via apply-textdelta
> + * following calls if this is a binary file or with
> + * unidiff for text files. */
> + SVN_ERR(svn_io_copy_file(yours, mine, TRUE, subpool));
> + SVN_ERR(svn_wc_add2(mine, adm_access, NULL, SVN_IGNORED_REVNUM,
> + patch_b->ctx->cancel_func,
> + patch_b->ctx->cancel_baton,
> + NULL, NULL, /* no notification */
> + subpool));
> + }
> +
> + /* Now regardless of the schedule-add nature, merge properties. */
> + if (prop_changes->nelts > 0)
> + SVN_ERR(merge_props_changed(adm_access, prop_state,
> + mine, prop_changes,
> + original_props, baton));
> + else
> + if (prop_state)
> + *prop_state = svn_wc_notify_state_unchanged;
>
> if (content_state)
> *content_state = svn_wc_notify_state_changed;
> @@ -491,6 +570,7 @@
> svn_wc_adm_access_t *parent_access;
> const char *parent_path;
> svn_error_t *err;
> + svn_boolean_t has_local_mods;
>
> /* 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 +591,15 @@
> svn_path_split(mine, &parent_path, NULL, subpool);
> SVN_ERR(svn_wc_adm_retrieve(&parent_access, adm_access, parent_path,
> subpool));
> +
> + SVN_ERR(svn_wc_text_modified_p(&has_local_mods, mine, TRUE,
> + adm_access, subpool));
> /* 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,
> + has_local_mods ? TRUE : FALSE,
> + NULL, NULL,

Hmm, so a delete (whether or not in a move) coming in against a file
with local-mods won't be any sort of conflict, it'll just leave it
unversioned? I guess that's reasonable?

> patch_b->ctx, subpool);
> if (err && state)
> {
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: svn-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: svn-help@subversion.tigris.org
>
>

--dave

-- 
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