You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by David Glasser <gl...@davidglasser.net> on 2007/10/16 22:05:08 UTC
Re: svn commit: r27222 - branches/svnpatch-diff/subversion/libsvn_client
On 10/16/07, cacknin@tigris.org <ca...@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, ©from_kind, pool));
> +
> + if (copyfrom_kind == svn_node_none)
> + {
> + SVN_ERR(svn_wc_get_pristine_copy_path(copyfrom_path,
> + ©from_textbase_path,
> + pool));
> + SVN_ERR(svn_io_check_path(copyfrom_textbase_path,
> + ©from_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, ©from_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(©from_access, NULL,
> + copyfrom_path, FALSE, -1,
> + patch_b->ctx->cancel_func,
> + patch_b->ctx->cancel_baton, pool));
> + SVN_ERR(svn_wc_get_ancestry(©from_url, ©from_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, ©from_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@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
Re: svn commit: r27222 - branches/svnpatch-diff/subversion/libsvn_client
Posted by Philip Martin <ph...@codematters.co.uk>.
"Charles Acknin" <ch...@gmail.com> writes:
> + SVN_ERR(svn_io_check_path(src_path, &src_kind, pool));
> +
> + if (src_kind == svn_node_none)
> + SVN_ERR(svn_io_copy_file(src_txtb, tmp_wc_text, FALSE, pool));
> + else
> + SVN_ERR(svn_io_copy_file(src_path, tmp_wc_text, TRUE, pool));
As a general rule avoid check-then-operate code. Try the first copy
and if that fails and the error corresponds to a missing file then try
the second copy. That avoids some disk IO and any possibility of a
race.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r27222 - branches/svnpatch-diff/subversion/libsvn_client
Posted by Charles Acknin <ch...@gmail.com>.
Actually it looks like so:
[[[
* subversion/libsvn_wc/copy.c
(copy_file_administratively): when the source file is missing, use its
text-base instead.
(svn_wc_copy2): assume the source file can be missing from disk.
]]]
[[[
Index: subversion/libsvn_wc/copy.c
===================================================================
--- subversion/libsvn_wc/copy.c (revision 27227)
+++ subversion/libsvn_wc/copy.c (working copy)
@@ -385,7 +385,7 @@
void *notify_baton,
apr_pool_t *pool)
{
- svn_node_kind_t dst_kind;
+ svn_node_kind_t dst_kind, src_kind;
const svn_wc_entry_t *src_entry, *dst_entry;
/* The 'dst_path' is simply dst_parent/dst_basename */
@@ -481,7 +481,14 @@
FALSE, special, pool));
}
else
- SVN_ERR(svn_io_copy_file(src_path, tmp_wc_text, TRUE, pool));
+ {
+ SVN_ERR(svn_io_check_path(src_path, &src_kind, pool));
+
+ if (src_kind == svn_node_none)
+ SVN_ERR(svn_io_copy_file(src_txtb, tmp_wc_text, FALSE, pool));
+ else
+ SVN_ERR(svn_io_copy_file(src_path, tmp_wc_text, TRUE, pool));
+ }
}
SVN_ERR(svn_wc_add_repos_file2(dst_path, dst_parent,
@@ -808,7 +815,7 @@
SVN_ERR(svn_io_check_path(src_path, &src_kind, pool));
- if (src_kind == svn_node_file)
+ if (src_kind == svn_node_file || src_kind == svn_node_none)
{
/* Check if we are copying a file scheduled for addition,
these require special handling. */
]]]
svn_wc.h:svn_wc_copy2()'s docstring would need a little fix accordingly.
Does that look fine? (copy_tests.py pass with success)
Charles
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r27222 - branches/svnpatch-diff/subversion/libsvn_client
Posted by Charles Acknin <ch...@gmail.com>.
On 10/17/07, David Glasser <gl...@davidglasser.net> wrote:
> While this code looks correct, it sure looks like you'd rather just be
> calling svn_wc__load_props instead of having svn_get_prop_diffs diff
> them and then undiff them yourself... This is suggesting more and
> more that you want to split the low-level application off into
> libsvn_wc, as we discussed a bit last night on IRC.
True, I've been feeling several times the need to use svn_wc__*
functions but couldn't because I was in libsvn_client. I think I'll
soon have to go the way you suggest if I keep bumping into this kind
of issue. The reason I'm not doing this right away is because
libsvn_client/merge -- which in logic is very similar to
libsvn_client/patch -- seems to sort this out pretty well (from where
it is, i.e. libsvn_client). Thus I suppose there's no reason
libsvn_client/patch can't do the same.
(Basically those scope problems I've met were because of differences
between merging and patching, like patch application takes place
offline, etc.)
> btw, have you looked at copy_file_administratively in
> libsvn_wc/copy.c? That's basically what you're trying to, um, copy
> (no pun intended).
Okay, just looked at it. Indeed, it does remind me those lines in
add_file_with_history :-) But hey, this is a static function, and the
only API that calls it is svn_wc_copy*, which I precisely couldn't use
and is the reason why I had to write such code. That sounds rather
paradoxical. Maybe I could modify svn_wc_copy2 and
copy_file_administratively to DTRT when src_path is svn_node_none? It
looks quite simple in fact. That would save some code redundancy and
minimize the code maintenance as well.
Should I work this out on the branch or send a patch?
Charles
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r27222 - branches/svnpatch-diff/subversion/libsvn_client
Posted by David Glasser <gl...@davidglasser.net>.
On 10/17/07, Charles Acknin <ch...@gmail.com> wrote:
> On 10/17/07, David Glasser <gl...@davidglasser.net> wrote:
> > Oh, by the way: my first thought when reading the "copy from pristine
> > file" section was that you'd forgotten to deal with subst
> > (eol/keywords). In fact your code is correct because
> > svn_wc_add_repos_file2 does that, but maybe adding a comment before
> > that call saying that it does substitution? Or maybe that was just me
> > being confused.
>
> Fixed in r27228.
>
> > > > > + /* 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(©from_access, NULL,
> > > > > + copyfrom_path, FALSE, -1,
> > > > > + patch_b->ctx->cancel_func,
> > > > > + patch_b->ctx->cancel_baton, pool));
> > > > > + SVN_ERR(svn_wc_get_ancestry(©from_url, ©from_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.
> > >
> > > Well not exactly. I'm throwing away the source file's unmodified
> > > properties, which is surely wrong, yes. On the other hand, the patch
> > > does bring modified properties. I'll have to work this a bit more,
> > > the problem is: do we have an API to get only unmodified properties,
> > > so that the file-add would bring unmodified properties and the
> > > svnpatch the modified ones? I wouldn't want the modified property
> > > diff not to show up and be buried in the property file like this is
> > > the case with merge.
> >
> > I can't imagine there isn't a base props API somewhere, as long as the
> > file hasn't been deleted... though I can't find one in a quick glance
> > at svn_wc.h.
>
> I found svn_wc_get_prop_diffs which seems like a useful API.
Yay.
> > Also, huh, now I'm really confused... if merge_file_deleted strikes
> > before merge_file_added then won't the pristine text-base be gone too?
> > How does that bit work at all?
>
> The merge_file_deleted just unversion files. Depending on the
> keep_local flag, the working file is removed from disk, but the
> text-base remains.
Er, um, of course you're right here.
> As for the props, here's a fix that makes use of svn_wc_get_prop_diffs:
>
> [[[
> Index: subversion/libsvn_client/patch.c
> ===================================================================
> --- subversion/libsvn_client/patch.c (revision 27228)
> +++ subversion/libsvn_client/patch.c (working copy)
> @@ -146,6 +146,10 @@
> const char *copyfrom_src; /* copy of copyfrom's text-base */
> svn_revnum_t copyfrom_rev;
> svn_wc_adm_access_t *copyfrom_access;
> + apr_hash_t *copyfrom_pristine_props, *copyfrom_working_props;
> + apr_array_header_t *cpf_propchanges;
> + apr_hash_index_t *hi;
> + int i;
>
> /* Since @c svn_wc_add_repos_file2() *moves* text-base path to
> * target-path, operate on a copy of it instead. */
> @@ -163,9 +167,39 @@
> patch_b->ctx->cancel_baton, pool));
> SVN_ERR(svn_wc_get_ancestry(©from_url, ©from_rev,
> copyfrom_path, copyfrom_access, pool));
> +
> + /* Retrieve the source file's properties. Once the
> + * propchanges array is converted to a hash table, we fill it
> + * up with pristine props to make it a hash of working props
> + * to feed @c svn_wc_add_repos_file2(). */
> + SVN_ERR(svn_wc_get_prop_diffs(&cpf_propchanges,
> + ©from_pristine_props,
> + copyfrom_path, copyfrom_access, pool));
> +
> + copyfrom_working_props = apr_hash_make(pool);
> + for (i = 0; i < cpf_propchanges->nelts; ++i)
> + {
> + const svn_prop_t *prop = &APR_ARRAY_IDX(cpf_propchanges, i,
> + svn_prop_t);
> + apr_hash_set(copyfrom_working_props, prop->name,
> APR_HASH_KEY_STRING,
> + prop->value);
> + }
> + for (hi = apr_hash_first(pool, copyfrom_pristine_props); hi;
> + hi = apr_hash_next(hi))
> + {
> + const void *k;
> + void *v;
> + const char *name, *value;
> + apr_hash_this(hi, &k, NULL, &v);
> + name = k;
> + value = v;
> + apr_hash_set(copyfrom_working_props, name,
> APR_HASH_KEY_STRING, value);
> + }
> +
> SVN_ERR(svn_wc_add_repos_file2(path, adm_access,
> copyfrom_src, NULL,
> - apr_hash_make(pool), NULL,
> + copyfrom_pristine_props,
> + copyfrom_working_props,
> copyfrom_url, copyfrom_rev,
> pool));
> }
> ]]]
>
> Review welcome :-)
While this code looks correct, it sure looks like you'd rather just be
calling svn_wc__load_props instead of having svn_get_prop_diffs diff
them and then undiff them yourself... This is suggesting more and
more that you want to split the low-level application off into
libsvn_wc, as we discussed a bit last night on IRC.
btw, have you looked at copy_file_administratively in
libsvn_wc/copy.c? That's basically what you're trying to, um, copy
(no pun intended).
--dave
--
David Glasser | glasser@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
Re: svn commit: r27222 - branches/svnpatch-diff/subversion/libsvn_client
Posted by Charles Acknin <ch...@gmail.com>.
On 10/17/07, David Glasser <gl...@davidglasser.net> wrote:
> Oh, by the way: my first thought when reading the "copy from pristine
> file" section was that you'd forgotten to deal with subst
> (eol/keywords). In fact your code is correct because
> svn_wc_add_repos_file2 does that, but maybe adding a comment before
> that call saying that it does substitution? Or maybe that was just me
> being confused.
Fixed in r27228.
> > > > + /* 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(©from_access, NULL,
> > > > + copyfrom_path, FALSE, -1,
> > > > + patch_b->ctx->cancel_func,
> > > > + patch_b->ctx->cancel_baton, pool));
> > > > + SVN_ERR(svn_wc_get_ancestry(©from_url, ©from_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.
> >
> > Well not exactly. I'm throwing away the source file's unmodified
> > properties, which is surely wrong, yes. On the other hand, the patch
> > does bring modified properties. I'll have to work this a bit more,
> > the problem is: do we have an API to get only unmodified properties,
> > so that the file-add would bring unmodified properties and the
> > svnpatch the modified ones? I wouldn't want the modified property
> > diff not to show up and be buried in the property file like this is
> > the case with merge.
>
> I can't imagine there isn't a base props API somewhere, as long as the
> file hasn't been deleted... though I can't find one in a quick glance
> at svn_wc.h.
I found svn_wc_get_prop_diffs which seems like a useful API.
> Also, huh, now I'm really confused... if merge_file_deleted strikes
> before merge_file_added then won't the pristine text-base be gone too?
> How does that bit work at all?
The merge_file_deleted just unversion files. Depending on the
keep_local flag, the working file is removed from disk, but the
text-base remains.
As for the props, here's a fix that makes use of svn_wc_get_prop_diffs:
[[[
Index: subversion/libsvn_client/patch.c
===================================================================
--- subversion/libsvn_client/patch.c (revision 27228)
+++ subversion/libsvn_client/patch.c (working copy)
@@ -146,6 +146,10 @@
const char *copyfrom_src; /* copy of copyfrom's text-base */
svn_revnum_t copyfrom_rev;
svn_wc_adm_access_t *copyfrom_access;
+ apr_hash_t *copyfrom_pristine_props, *copyfrom_working_props;
+ apr_array_header_t *cpf_propchanges;
+ apr_hash_index_t *hi;
+ int i;
/* Since @c svn_wc_add_repos_file2() *moves* text-base path to
* target-path, operate on a copy of it instead. */
@@ -163,9 +167,39 @@
patch_b->ctx->cancel_baton, pool));
SVN_ERR(svn_wc_get_ancestry(©from_url, ©from_rev,
copyfrom_path, copyfrom_access, pool));
+
+ /* Retrieve the source file's properties. Once the
+ * propchanges array is converted to a hash table, we fill it
+ * up with pristine props to make it a hash of working props
+ * to feed @c svn_wc_add_repos_file2(). */
+ SVN_ERR(svn_wc_get_prop_diffs(&cpf_propchanges,
+ ©from_pristine_props,
+ copyfrom_path, copyfrom_access, pool));
+
+ copyfrom_working_props = apr_hash_make(pool);
+ for (i = 0; i < cpf_propchanges->nelts; ++i)
+ {
+ const svn_prop_t *prop = &APR_ARRAY_IDX(cpf_propchanges, i,
+ svn_prop_t);
+ apr_hash_set(copyfrom_working_props, prop->name,
APR_HASH_KEY_STRING,
+ prop->value);
+ }
+ for (hi = apr_hash_first(pool, copyfrom_pristine_props); hi;
+ hi = apr_hash_next(hi))
+ {
+ const void *k;
+ void *v;
+ const char *name, *value;
+ apr_hash_this(hi, &k, NULL, &v);
+ name = k;
+ value = v;
+ apr_hash_set(copyfrom_working_props, name,
APR_HASH_KEY_STRING, value);
+ }
+
SVN_ERR(svn_wc_add_repos_file2(path, adm_access,
copyfrom_src, NULL,
- apr_hash_make(pool), NULL,
+ copyfrom_pristine_props,
+ copyfrom_working_props,
copyfrom_url, copyfrom_rev,
pool));
}
]]]
Review welcome :-)
(I'm late I have a lecture, I'll commit this evening if that's fine)
Charles
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r27222 - branches/svnpatch-diff/subversion/libsvn_client
Posted by David Glasser <gl...@davidglasser.net>.
On 10/16/07, Charles Acknin <ch...@gmail.com> wrote:
> On 10/17/07, David Glasser <gl...@davidglasser.net> wrote:
> > 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?)
>
> I just figured it would be more convenient to have related helper
> functions at the same place in the file. get_empty_file() calls
> create_empty_file() for example. I'm trying to keep this file as
> clear as possible since it contains both diff callbacks (first half)
> and editor functions (second half).
>
> Now I'm just realizing the add_file_with_history *is* at the wrong
> place, in the midst of diff callbacks although it is called by
> merge_file_added(). Going to move it up.
Yes, clarity is good! Perhaps helpers at the top, then.
> > 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?)
>
> Why? Local-mods implies file-not-deleted implies call to
> svn_wc_copy2, which does handle that gracefully. Were you talking
> about the docstring or the function body itself?
I guess I read the docstring and expected to see logic about
local-mods, and didn't, and was surprised. But as I guessed (and
didn't investigate enough) and you confirmed, that happens in
svn_wc_copy2.
Oh, by the way: my first thought when reading the "copy from pristine
file" section was that you'd forgotten to deal with subst
(eol/keywords). In fact your code is correct because
svn_wc_add_repos_file2 does that, but maybe adding a comment before
that call saying that it does substitution? Or maybe that was just me
being confused.
> > > + /* 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(©from_access, NULL,
> > > + copyfrom_path, FALSE, -1,
> > > + patch_b->ctx->cancel_func,
> > > + patch_b->ctx->cancel_baton, pool));
> > > + SVN_ERR(svn_wc_get_ancestry(©from_url, ©from_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.
>
> Well not exactly. I'm throwing away the source file's unmodified
> properties, which is surely wrong, yes. On the other hand, the patch
> does bring modified properties. I'll have to work this a bit more,
> the problem is: do we have an API to get only unmodified properties,
> so that the file-add would bring unmodified properties and the
> svnpatch the modified ones? I wouldn't want the modified property
> diff not to show up and be buried in the property file like this is
> the case with merge.
I can't imagine there isn't a base props API somewhere, as long as the
file hasn't been deleted... though I can't find one in a quick glance
at svn_wc.h.
Also, huh, now I'm really confused... if merge_file_deleted strikes
before merge_file_added then won't the pristine text-base be gone too?
How does that bit work at all?
--dave
--
David Glasser | glasser@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
Re: svn commit: r27222 - branches/svnpatch-diff/subversion/libsvn_client
Posted by Charles Acknin <ch...@gmail.com>.
On 10/17/07, David Glasser <gl...@davidglasser.net> wrote:
> 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?)
I just figured it would be more convenient to have related helper
functions at the same place in the file. get_empty_file() calls
create_empty_file() for example. I'm trying to keep this file as
clear as possible since it contains both diff callbacks (first half)
and editor functions (second half).
Now I'm just realizing the add_file_with_history *is* at the wrong
place, in the midst of diff callbacks although it is called by
merge_file_added(). Going to move it up.
> 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?)
Why? Local-mods implies file-not-deleted implies call to
svn_wc_copy2, which does handle that gracefully. Were you talking
about the docstring or the function body itself?
> > + /* 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(©from_access, NULL,
> > + copyfrom_path, FALSE, -1,
> > + patch_b->ctx->cancel_func,
> > + patch_b->ctx->cancel_baton, pool));
> > + SVN_ERR(svn_wc_get_ancestry(©from_url, ©from_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.
Well not exactly. I'm throwing away the source file's unmodified
properties, which is surely wrong, yes. On the other hand, the patch
does bring modified properties. I'll have to work this a bit more,
the problem is: do we have an API to get only unmodified properties,
so that the file-add would bring unmodified properties and the
svnpatch the modified ones? I wouldn't want the modified property
diff not to show up and be buried in the property file like this is
the case with merge.
> > 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?
This is it. dlr and I discussed this issue on IRC a few days ago. We
ended up thinking that would be reasonable to leave the file
unversioned. After all, the file brings user modifications which we
wouldn't want to erase. I think that goes pretty much in the same
direction as sussman's recent work with copyfrom'ed update. dlr said
something that sounds relevant to me: "the user is trying to apply a
patch in a modified working copy, he can't expect it to be in the
exact same state as that of the patch creation".
Charles
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r27222 - branches/svnpatch-diff/subversion/libsvn_client
Posted by Charles Acknin <ch...@gmail.com>.
(Forgot one part)
As for the tests, patch_tests.py only has 2 tests which don't address
those particular points. I'm about to start writing some python :-)
Charles
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org