Re: svn commit: r937010 - in /subversion/trunk/subversion: include/private/svn_wc_private.h libsvn_client/diff.c libsvn_wc/copy.c libsvn_wc/node.c

I *think* there's a bigger problem with this change.

When I created svn_wc__node_get_copyfrom_info(), I specifically wanted the
behavior that svn_wc_entry_t promised with respect to copyfrom information,
which is that it was set on the targets of a copy operation but *not* on the
children of that copy target (if it was a directory). (This is why I didn't
just migrate determine_copyfrom_info() into node.c as the new function
myself.) Don't Philip's changes cause the function to return copyfrom
information where it previously would not?

I'm okay with that myself, but as far as I can tell the behavior needs to be
optional so all callers can maintain the behavior they had in WC-1 (unless
we know it to be wrong, of course). libsvn_client/diff.c:convert_to_url()
is such a function that previously did *not* recognize inherited copyfrom
information. If Philip's changes do what I think they do, that's been changed.

Greg Stein wrote:
> On Thu, Apr 22, 2010 at 14:59, <philip_at_apache.org> wrote:
>> ...
>> +++ subversion/trunk/subversion/libsvn_client/diff.c Thu Apr 22 18:59:01 2010
>> @@ -894,6 +894,11 @@ convert_to_url(const char **url,
>> SVN_ERR(svn_wc__node_get_copyfrom_info(url, &copyfrom_rev,
>> wc_ctx, abspath_or_url,
>> result_pool, scratch_pool));
>> + if (! url)
>> + return svn_error_createf(SVN_ERR_ENTRY_MISSING_URL, NULL,
>> + _("Path '%s' has no URL"),
>> + svn_dirent_local_style(abspath_or_url,
>> + scratch_pool));
>
> Shouldn't that be: if (*url == NULL) (or ! *url if you prefer)
>
>> ...
>> +++ subversion/trunk/subversion/libsvn_wc/node.c Thu Apr 22 18:59:01 2010
>> ...
>> @@ -362,11 +363,46 @@ svn_wc__node_get_copyfrom_info(const cha
>> local_abspath, scratch_pool, scratch_pool));
>> if (original_root_url && original_repos_relpath)
>> {
>> + /* If this was the root of the copy then the URL is immediately
>> + available... */
>> *copyfrom_url = svn_path_url_add_component2(original_root_url,
>> original_repos_relpath,
>> result_pool);
>> *copyfrom_rev = original_revision;
>> }
>> + else if (status == svn_wc__db_status_added
>> + || status == svn_wc__db_status_obstructed_add)
>
> If the add is obstructed, then you cannot call scan_addition.
>
> The true copyfrom information may have been located in the
> (obstructed) subdir. You got NULL values from read_info because the
> stub does not have them.
>
> It would be nice to put some kind of assertion into scan_addition to
> enforce this. I've seen this come up elsewhere.
>
> I'll take a look now...
>
>> + {
>> + /* ...But if this is merely the descendant of an explicitly
>> + copied/moved directory, we need to do a bit more work to
>> + determine copyfrom_url and copyfrom_rev. */
>> + const char *op_root_abspath;
>> +
>> + SVN_ERR(svn_wc__db_scan_addition(&status, &op_root_abspath, NULL, NULL,
>> + NULL, &original_repos_relpath,
>> + &original_root_url, NULL,
>> + &original_revision, db, local_abspath,
>> + scratch_pool, scratch_pool));
>> + if (status == svn_wc__db_status_copied ||
>> + status == svn_wc__db_status_moved_here)
>> + {
>> + const char *src_parent_url;
>> + const char *src_relpath;
>> +
>> + src_parent_url = svn_path_url_add_component2(original_root_url,
>> + original_repos_relpath,
>> + scratch_pool);
>> + src_relpath = svn_dirent_is_child(op_root_abspath, local_abspath,
>> + scratch_pool);
>> + if (src_relpath)
>
> src_relpath is always not-NULL. You cannot have an op_root_abspath
> that is NOT an ancestor of local_abspath.
>
>> + {
>> + *copyfrom_url = svn_path_url_add_component2(src_parent_url,
>> + src_relpath,
>> + result_pool);
>> + *copyfrom_rev = original_revision;
>> + }
>> ...
>
> Cheers,
> -g

-- 
C. Michael Pilato <cmpilato_at_collab.net>
CollabNet   <>   www.collab.net   <>   Distributed Development On Demand