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

On 10/17/07, Charles Acknin <charlesacknin@gmail.com> wrote:
> On 10/17/07, David Glasser <glasser@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(&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.
> > >
> > > 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(&copyfrom_url, &copyfrom_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,
> + &copyfrom_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_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