You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by ph...@apache.org on 2010/04/22 20:59:02 UTC
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
Author: philip
Date: Thu Apr 22 18:59:01 2010
New Revision: 937010
URL: http://svn.apache.org/viewvc?rev=937010&view=rev
Log:
Move some code.
* subversion/libsvn_wc/copy.c
(determine_copyfrom_info): Delete.
(copy_file_administratively, copy_dir_administratively): Use
svn_wc__node_get_copyfrom_info instead.
* subversion/include/private/svn_wc_private.h
(svn_wc__node_get_copyfrom_info): Tweak docstring.
* subversion/libsvn_wc/node.c
(svn_wc__node_get_copyfrom_info): Extend to handle all the cases
covered by determine_copyfrom_info.
* subversion/libsvn_client/diff.c
(convert_to_url): Return an error if no URL available.
Modified:
subversion/trunk/subversion/include/private/svn_wc_private.h
subversion/trunk/subversion/libsvn_client/diff.c
subversion/trunk/subversion/libsvn_wc/copy.c
subversion/trunk/subversion/libsvn_wc/node.c
Modified: subversion/trunk/subversion/include/private/svn_wc_private.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/private/svn_wc_private.h?rev=937010&r1=937009&r2=937010&view=diff
==============================================================================
--- subversion/trunk/subversion/include/private/svn_wc_private.h (original)
+++ subversion/trunk/subversion/include/private/svn_wc_private.h Thu Apr 22 18:59:01 2010
@@ -363,9 +363,9 @@ svn_wc__node_get_url(const char **url,
/**
* Set @a *copyfrom_url to the corresponding copy-from URL, and @a
* copyfrom_rev to the corresponding copy-from revision, of @a
- * local_abspath, using @a wc_ctx. If @a local_abspath does not
- * represent the root of a copied subtree, set @a *copyfrom_rev to
- * NULL and @a copyfrom_rev to @c SVN_INVALID_REVNUM.
+ * local_abspath, using @a wc_ctx. If @a local_abspath is not copied,
+ * set @a *copyfrom_rev to NULL and @a *copyfrom_rev to @c
+ * SVN_INVALID_REVNUM.
*/
svn_error_t *
svn_wc__node_get_copyfrom_info(const char **copyfrom_url,
Modified: subversion/trunk/subversion/libsvn_client/diff.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/diff.c?rev=937010&r1=937009&r2=937010&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_client/diff.c (original)
+++ 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));
}
return SVN_NO_ERROR;
}
Modified: subversion/trunk/subversion/libsvn_wc/copy.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/copy.c?rev=937010&r1=937009&r2=937010&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_wc/copy.c (original)
+++ subversion/trunk/subversion/libsvn_wc/copy.c Thu Apr 22 18:59:01 2010
@@ -258,78 +258,6 @@ copy_added_dir_administratively(svn_wc_c
return SVN_NO_ERROR;
}
-/* A helper for copy_file_administratively() which sets *COPYFROM_URL
- and *COPYFROM_REV appropriately (possibly to NULL/SVN_INVALID_REVNUM). */
-static svn_error_t *
-determine_copyfrom_info(const char **copyfrom_url,
- svn_revnum_t *copyfrom_rev,
- svn_wc__db_t *db,
- const char *src_abspath,
- apr_pool_t *result_pool,
- apr_pool_t *scratch_pool)
-{
- const char *url;
- const char *original_root_url;
- const char *original_repos_relpath;
- svn_revnum_t original_revision;
- svn_wc__db_status_t status;
-
- url = NULL;
- original_revision = SVN_INVALID_REVNUM;
-
- SVN_ERR(svn_wc__db_read_info(&status, NULL, NULL, NULL, NULL, NULL, NULL,
- NULL, NULL, NULL, NULL, NULL, NULL, NULL,
- NULL, &original_repos_relpath,
- &original_root_url, NULL, &original_revision,
- NULL, NULL, NULL, NULL, NULL, db, src_abspath,
- scratch_pool, scratch_pool));
- if (original_root_url && original_repos_relpath)
- {
- /* When copying/moving a file that was already explicitly
- copied/moved then we know the URL it was copied from... */
- url = svn_path_url_add_component2(original_root_url,
- original_repos_relpath, scratch_pool);
- }
- else if (status == svn_wc__db_status_added
- || status == svn_wc__db_status_obstructed_add)
- {
- /* ...But if this file 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, src_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, src_abspath,
- scratch_pool);
- if (src_relpath)
- url = svn_path_url_add_component2(src_parent_url, src_relpath,
- scratch_pool);
- }
- }
-
- if (url)
- *copyfrom_url = apr_pstrdup(result_pool, url);
- else
- *copyfrom_url = NULL;
- *copyfrom_rev = original_revision;
-
- return SVN_NO_ERROR;
-}
-
-
/* This function effectively creates and schedules a file for
addition, but does extra administrative things to allow it to
function as a 'copy'.
@@ -420,9 +348,9 @@ copy_file_administratively(svn_wc_contex
but not committed? */
if (src_entry->copied)
{
- SVN_ERR(determine_copyfrom_info(©from_url, ©from_rev, db,
- src_abspath,
- scratch_pool, scratch_pool));
+ SVN_ERR(svn_wc__node_get_copyfrom_info(©from_url, ©from_rev,
+ wc_ctx, src_abspath,
+ scratch_pool, scratch_pool));
/* If the COPYFROM information is the SAME as the destination
URL/REVISION, then omit the copyfrom info. */
@@ -762,9 +690,9 @@ copy_dir_administratively(svn_wc_context
SVN_ERR(svn_wc__get_entry(&dst_entry, db, dst_abspath, TRUE,
svn_node_dir, TRUE,
scratch_pool, scratch_pool));
- SVN_ERR(determine_copyfrom_info(©from_url, ©from_rev, db,
- src_abspath,
- scratch_pool, scratch_pool));
+ SVN_ERR(svn_wc__node_get_copyfrom_info(©from_url, ©from_rev,
+ wc_ctx, src_abspath,
+ scratch_pool, scratch_pool));
/* If the COPYFROM information is the SAME as the destination
URL/REVISION, then omit the copyfrom info. */
Modified: subversion/trunk/subversion/libsvn_wc/node.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/node.c?rev=937010&r1=937009&r2=937010&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_wc/node.c (original)
+++ subversion/trunk/subversion/libsvn_wc/node.c Thu Apr 22 18:59:01 2010
@@ -350,11 +350,12 @@ svn_wc__node_get_copyfrom_info(const cha
const char *original_root_url;
const char *original_repos_relpath;
svn_revnum_t original_revision;
+ svn_wc__db_status_t status;
*copyfrom_url = NULL;
*copyfrom_rev = SVN_INVALID_REVNUM;
- SVN_ERR(svn_wc__db_read_info(NULL, NULL, NULL, NULL, NULL, NULL, NULL,
+ SVN_ERR(svn_wc__db_read_info(&status, NULL, NULL, NULL, NULL, NULL, NULL,
NULL, NULL, NULL, NULL, NULL, NULL, NULL,
NULL, &original_repos_relpath,
&original_root_url, NULL, &original_revision,
@@ -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)
+ {
+ /* ...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)
+ {
+ *copyfrom_url = svn_path_url_add_component2(src_parent_url,
+ src_relpath,
+ result_pool);
+ *copyfrom_rev = original_revision;
+ }
+ }
+ }
return SVN_NO_ERROR;
}
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
Posted by "C. Michael Pilato" <cm...@collab.net>.
philip@apache.org wrote:
> Author: philip
> Date: Thu Apr 22 18:59:01 2010
> New Revision: 937010
>
> URL: http://svn.apache.org/viewvc?rev=937010&view=rev
> Log:
> Move some code.
>
> * subversion/libsvn_wc/copy.c
> (determine_copyfrom_info): Delete.
> (copy_file_administratively, copy_dir_administratively): Use
> svn_wc__node_get_copyfrom_info instead.
>
> * subversion/include/private/svn_wc_private.h
> (svn_wc__node_get_copyfrom_info): Tweak docstring.
>
> * subversion/libsvn_wc/node.c
> (svn_wc__node_get_copyfrom_info): Extend to handle all the cases
> covered by determine_copyfrom_info.
>
> * subversion/libsvn_client/diff.c
> (convert_to_url): Return an error if no URL available.
. o O ( He stole my next commit! )
$ svn revert -R .
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
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
Posted by Philip Martin <ph...@wandisco.com>.
Greg Stein <gs...@gmail.com> writes:
>> I understand all that. My question is in the (few) cases where we
>> don't have an URL will we have a copyfrom URL. It seems unlikely to
>> me.
>
> I will state, "in any potential case where a URL is not available...
> NO, we will not have a copyfrom."
OK. The other places that retrieve the copyfrom URL in the client
library use it ahead of the URL -- the opposite to diff. I'll take it
out of diff.
--
Philip
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
Posted by Greg Stein <gs...@gmail.com>.
On Fri, Apr 23, 2010 at 06:56, Philip Martin <ph...@wandisco.com> wrote:
> Greg Stein <gs...@gmail.com> writes:
>
>>> Are there any circumstances today when a node will not have an URL but
>>> will have a copyfrom URL? Everything seems to work if I remove the
>>> copyfrom stuf from convert_to_url.
>>
>> entry->url "does not exist"... today, we call a function to provide a
>> URL. That means we can return a URL in every possible situation, for
>> some semantic of "what does that URL represent?"
>>
>> In general(*), entry->url means "the repository location that the node
>> came from, or where it will end up after a commit". And with that
>> semantic, we can *almost* always provide an answer.
>>
>> The only situation that I can think of is where a switched subdir has
>> been rm'd so we get back svn_wc__db_status_obstructed from the wc_db
>> functions. If we use the parent's information, we can guess at a URL,
>> but (due to the switch) it is wrong. Conceivably, we could *ensure*
>> that enough information is left in the parent stub to properly compute
>> the URL.
>>
>> We can always compute "where will this end up?" regardless of rm'd
>> subdirs. Excluded/absent/etc nodes can be derived from the parent, as
>> they are never switched.
>>
>> In single-db, the above-noted obstruction is no longer possible, which
>> means we'll always have a URL according to the above definition.
>
> I understand all that. My question is in the (few) cases where we
> don't have an URL will we have a copyfrom URL. It seems unlikely to
> me.
I will state, "in any potential case where a URL is not available...
NO, we will not have a copyfrom."
So: anything varying from that statement is a bug, so it can be taken
as axiomatic, and we work from there.
Cheers,
-g
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
Posted by Philip Martin <ph...@wandisco.com>.
Greg Stein <gs...@gmail.com> writes:
>> Are there any circumstances today when a node will not have an URL but
>> will have a copyfrom URL? Everything seems to work if I remove the
>> copyfrom stuf from convert_to_url.
>
> entry->url "does not exist"... today, we call a function to provide a
> URL. That means we can return a URL in every possible situation, for
> some semantic of "what does that URL represent?"
>
> In general(*), entry->url means "the repository location that the node
> came from, or where it will end up after a commit". And with that
> semantic, we can *almost* always provide an answer.
>
> The only situation that I can think of is where a switched subdir has
> been rm'd so we get back svn_wc__db_status_obstructed from the wc_db
> functions. If we use the parent's information, we can guess at a URL,
> but (due to the switch) it is wrong. Conceivably, we could *ensure*
> that enough information is left in the parent stub to properly compute
> the URL.
>
> We can always compute "where will this end up?" regardless of rm'd
> subdirs. Excluded/absent/etc nodes can be derived from the parent, as
> they are never switched.
>
> In single-db, the above-noted obstruction is no longer possible, which
> means we'll always have a URL according to the above definition.
I understand all that. My question is in the (few) cases where we
don't have an URL will we have a copyfrom URL. It seems unlikely to
me.
--
Philip
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
Posted by Greg Stein <gs...@gmail.com>.
On Fri, Apr 23, 2010 at 06:37, Philip Martin <ph...@wandisco.com> wrote:
> Greg Stein <gs...@gmail.com> writes:
>
>> 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.
>
> The copyfrom stuff in libsvn_client/diff.c:convert_to_url dates to
> pre-1.0 and was to fix issue 1229. In those days it appears that
> copied items had copyfrom URLs but not URLs.
>
> Are there any circumstances today when a node will not have an URL but
> will have a copyfrom URL? Everything seems to work if I remove the
> copyfrom stuf from convert_to_url.
entry->url "does not exist"... today, we call a function to provide a
URL. That means we can return a URL in every possible situation, for
some semantic of "what does that URL represent?"
In general(*), entry->url means "the repository location that the node
came from, or where it will end up after a commit". And with that
semantic, we can *almost* always provide an answer.
The only situation that I can think of is where a switched subdir has
been rm'd so we get back svn_wc__db_status_obstructed from the wc_db
functions. If we use the parent's information, we can guess at a URL,
but (due to the switch) it is wrong. Conceivably, we could *ensure*
that enough information is left in the parent stub to properly compute
the URL.
We can always compute "where will this end up?" regardless of rm'd
subdirs. Excluded/absent/etc nodes can be derived from the parent, as
they are never switched.
In single-db, the above-noted obstruction is no longer possible, which
means we'll always have a URL according to the above definition.
Cheers,
-g
(*) in general... in some cases, the url is "stale" and points
elsewhere. see notes/api-errata/wc001.txt
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
Posted by Philip Martin <ph...@wandisco.com>.
Greg Stein <gs...@gmail.com> writes:
> 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.
The copyfrom stuff in libsvn_client/diff.c:convert_to_url dates to
pre-1.0 and was to fix issue 1229. In those days it appears that
copied items had copyfrom URLs but not URLs.
Are there any circumstances today when a node will not have an URL but
will have a copyfrom URL? Everything seems to work if I remove the
copyfrom stuf from convert_to_url.
--
Philip
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
Posted by "C. Michael Pilato" <cm...@collab.net>.
Scratch that -- Philip already made that change.
C. Michael Pilato wrote:
> Greg Stein wrote:
>> 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.
>
> I agree, it's probably okay in this case. But I think the docstring for
> svn_wc__node_get_copyfrom_info() needs to be updated to make its behavior clear.
>
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
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
Posted by "C. Michael Pilato" <cm...@collab.net>.
Greg Stein wrote:
> 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.
I agree, it's probably okay in this case. But I think the docstring for
svn_wc__node_get_copyfrom_info() needs to be updated to make its behavior clear.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
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
Posted by Greg Stein <gs...@gmail.com>.
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 <cm...@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, <ph...@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 <cm...@collab.net>
> CollabNet <> www.collab.net <> Distributed Development On Demand
>
>
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
Posted by "C. Michael Pilato" <cm...@collab.net>.
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, <ph...@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 <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
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
Posted by Greg Stein <gs...@gmail.com>.
On Thu, Apr 22, 2010 at 14:59, <ph...@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
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
Posted by "C. Michael Pilato" <cm...@collab.net>.
philip@apache.org wrote:
> Author: philip
> Date: Thu Apr 22 18:59:01 2010
> New Revision: 937010
>
> URL: http://svn.apache.org/viewvc?rev=937010&view=rev
> Log:
> Move some code.
>
> * subversion/libsvn_wc/copy.c
> (determine_copyfrom_info): Delete.
> (copy_file_administratively, copy_dir_administratively): Use
> svn_wc__node_get_copyfrom_info instead.
>
> * subversion/include/private/svn_wc_private.h
> (svn_wc__node_get_copyfrom_info): Tweak docstring.
>
> * subversion/libsvn_wc/node.c
> (svn_wc__node_get_copyfrom_info): Extend to handle all the cases
> covered by determine_copyfrom_info.
>
> * subversion/libsvn_client/diff.c
> (convert_to_url): Return an error if no URL available.
. o O ( He stole my next commit! )
$ svn revert -R .
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand