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

On 10/16/07, Charles Acknin <charlesackningmail.com> wrote:
> On 10/17/07, David Glasser <glasserdavidglasser.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(&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.

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 | glasserdavidglasser.net | http://www.davidglasser.
net/

------------------------------------------------------------
---------
To unsubscribe, e-mail: dev-unsubscribesubversion.tigris.org
For additional commands, e-mail: dev-helpsubversion.tigris.org

On 10/17/07, David Glasser <glasserdavidglasser.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.

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

(I'm late I have a lecture, I'll commit this evening if
that's fine)

Charles

------------------------------------------------------------
---------
To unsubscribe, e-mail: dev-unsubscribesubversion.tigris.org
For additional commands, e-mail: dev-helpsubversion.tigris.org