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

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