Yes, you hit the nail on the head. I had a similar concern, but wasn't
sure whether to bring it up.
Specifically: in the particular case that *you* created the function
for, the copyfrom-fetching would most likely *never* be invoked.
node_get_url() should return a URL in almost every situation. In fact,
I started looking at get_url to delineate exactly *which* situations
it would not return a URL (that change is pending, but I'll quick
finish that and commit it for demonstration).
But for other cases where you may have entry->copyfrom_*, then yes...
Philip's change is not going to be usable.
I'll leave it for you to steer what kinds of query functions you need,
and I'll get this mod to node_get_url checked in.
Cheers,
-g
On Thu, Apr 22, 2010 at 21:42, C. Michael Pilato <cmpilato_at_collab.net> wrote:
> 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, ©from_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
>
>
Received on 2010-04-23 03:55:18 CEST