You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Matthew Bentham <mj...@artvps.com> on 2010/03/02 09:45:02 UTC
[PATCH] wc-ng - Remove use of entry_t from calculate_target_mergeinfo
Trying to remove more uses of svn_wc_entry_t, it seems there is no way
at the moment to get the equivalent of entry->copied using node
routines. Is this the sort of thing that is needed?
[[[
wc-ng: work towards eliminating svn_wc_entry_t
* libsvn_wc/node.c, include/private/svn_wc_private.h
Add svn_wc__node_is_status_copied()
* subversion/libsvn_client/copy.c
(calculate_target_mergeinfo): Remove unused 'no_repos_access'
parameter, replace use of svn_wc__get_entry with node routines.
Patch by: Matthew Bentham <mjb67{_AT_}artvps.com>
]]]
Re: [PATCH] wc-ng - Remove use of entry_t from
calculate_target_mergeinfo
Posted by Julian Foad <ju...@btopenworld.com>.
On Tue, 2010-03-02 at 09:45 +0000, Matthew Bentham wrote:
> Trying to remove more uses of svn_wc_entry_t, it seems there is no way
> at the moment to get the equivalent of entry->copied using node
> routines. Is this the sort of thing that is needed?
>
> [[[
> wc-ng: work towards eliminating svn_wc_entry_t
>
> * libsvn_wc/node.c, include/private/svn_wc_private.h
> Add svn_wc__node_is_status_copied()
>
> * subversion/libsvn_client/copy.c
> (calculate_target_mergeinfo): Remove unused 'no_repos_access'
This change is entirely separate from eliminating svn_wc_entry_t. Can
you submit it as a separate patch?
> parameter, replace use of svn_wc__get_entry with node routines.
>
> Patch by: Matthew Bentham <mjb67{_AT_}artvps.com>
> ]]]
Re: [PATCH] wc-ng - Remove use of entry_t from
calculate_target_mergeinfo
Posted by Julian Foad <ju...@btopenworld.com>.
Matthew Bentham wrote:
> Trying to remove more uses of svn_wc_entry_t, it seems there is no way
> at the moment to get the equivalent of entry->copied using node
> routines. Is this the sort of thing that is needed?
>
> [[[
> wc-ng: work towards eliminating svn_wc_entry_t
>
> * libsvn_wc/node.c, include/private/svn_wc_private.h
> Add svn_wc__node_is_status_copied()
>
> * subversion/libsvn_client/copy.c
> (calculate_target_mergeinfo): Remove unused 'no_repos_access'
This change is logically separate from eliminating svn_wc_entry_t. Can
you submit it as a separate patch, please?
> parameter, replace use of svn_wc__get_entry with node routines.
>
> Patch by: Matthew Bentham <mjb67{_AT_}artvps.com>
> ]]]
[...]
> Index: subversion/libsvn_client/copy.c
> ===================================================================
> --- subversion/libsvn_client/copy.c (revision 916192)
> +++ subversion/libsvn_client/copy.c (working copy)
> @@ -71,20 +71,16 @@
> /* Obtain the implied mergeinfo and the existing mergeinfo of the
> source path, combine them and return the result in
> *TARGET_MERGEINFO. ADM_ACCESS may be NULL, if SRC_PATH_OR_URL is an
> - URL. If NO_REPOS_ACCESS is set, this function is disallowed from
> - consulting the repository about anything. RA_SESSION may be NULL but
> - only if NO_REPOS_ACCESS is true. */
> + URL. RA_SESSION may be NULL. */
Don't you mean, "RA_SESSION must not be NULL" ?
> static svn_error_t *
> calculate_target_mergeinfo(svn_ra_session_t *ra_session,
- Julian
Re: holes in the WC-NG schema?
Posted by Greg Stein <gs...@gmail.com>.
On Fri, Mar 5, 2010 at 11:43, Bert Huijben <be...@qqmail.nl> wrote:
>> -----Original Message-----
>> From: Neels J Hofmeyr [mailto:neels@elego.de]
>> Sent: vrijdag 5 maart 2010 11:45
>> To: dev@subversion.apache.org
>> Subject: Re: holes in the WC-NG schema?
>>
>> Neels J Hofmeyr wrote:
>> > I remember someone talking of a hole. It went something like: If a
> folder is
>> > copied-here, all its children have locally added status, and I
> understand
>> > they refer to the op-root of the add to find out their history, i.e.
> that
>> > they are copied. Now what if I replace such a child node with a fresh,
> new
>> > node -- it will still think that it's part of the copy-here. Just vague
>> > memory, haven't verified. This one should be fixed if it turns out to be
> so.
>>
>> Turns out we are aware of this problem. My noob shot at a solution would
> be
>> to have an indicator whether a WORKING node is the op-root of an add.
>> Then
>> we can have an op-root of an add within another op-root of an add without
>> confusion. We might indicate on the inner op-root that they are not the
> root
>> of all locally added nodes, but just the root of an add operation. (Or
> have
>> scan_addition() find out by also scanning the parent of each add-op-root,
>> which it does anyway when asked to find the repository path of the add,
>> which it derives from the op-root's parent's BASE tree node ('s
> ancestors).)
>
> This would fix local adds, but it is not a complete solution for allowing
> all revert scenarios from nested add with history trees. (especially the
> cases where you replace some subtree of an add with history with a different
> tree)
I've thought on this, and I think the current schema, with the
addition the singular marker we talked about at the root of a (local)
add will be complete.
First, stop saying add-with-history. Those are copies or moves. Very
different from an add. We have three terms, each meaning something
different. Continuing to use the "with history" is simply confusing.
Now. The main thing here is that the local database records enough
information to construct a *commit* to the target repository. We do
not need to remember each and every operation that arrived at that
state. Only what is necessary to replicate the local state up to the
repository.
The WORKING_NODE tables models adds/copies/moves/deletes. The specific
columns in the schema, when not-null, define the roots of copies and
moves. These can be nested since each copy/move specifies a new root.
Deletes do not require this nesting of operations -- once you delete
an ancestor, then any prior deletes of descendants are irrelevant. Our
problem is that we do not model adds beyond "hey! there is a
WORKING_NODE!". Attaching a "fake" copyfrom_* can delimit these adds,
thus allowing complex nesting of what is present in the WORKING tree.
Thus, I state we *can* do a complete modeling, once we define a
singleton to mean "add".
> We need a more advanced schema to fix this category. The simple fix would
> fix this behavior we supported in 1.6, but we might want the better fix.
I disagree.
Cheers,
-g
Re: holes in the WC-NG schema?
Posted by Neels J Hofmeyr <ne...@elego.de>.
Bert Huijben wrote:
>
>> -----Original Message-----
>> From: Neels J Hofmeyr [mailto:neels@elego.de]
>> Sent: vrijdag 5 maart 2010 11:45
>> To: dev@subversion.apache.org
>> Subject: Re: holes in the WC-NG schema?
>>
>> Neels J Hofmeyr wrote:
>>> I remember someone talking of a hole. It went something like: If a
> folder is
>>> copied-here, all its children have locally added status, and I
> understand
>>> they refer to the op-root of the add to find out their history, i.e.
> that
>>> they are copied. Now what if I replace such a child node with a fresh,
> new
>>> node -- it will still think that it's part of the copy-here. Just vague
>>> memory, haven't verified. This one should be fixed if it turns out to be
> so.
>> Turns out we are aware of this problem. My noob shot at a solution would
> be
>> to have an indicator whether a WORKING node is the op-root of an add.
>> Then
>> we can have an op-root of an add within another op-root of an add without
>> confusion. We might indicate on the inner op-root that they are not the
> root
>> of all locally added nodes, but just the root of an add operation. (Or
> have
>> scan_addition() find out by also scanning the parent of each add-op-root,
>> which it does anyway when asked to find the repository path of the add,
>> which it derives from the op-root's parent's BASE tree node ('s
> ancestors).)
>
> This would fix local adds, but it is not a complete solution for allowing
> all revert scenarios from nested add with history trees. (especially the
> cases where you replace some subtree of an add with history with a different
> tree)
...are you talking about being able to revert layer-wise, like, only the
replace within, so that the originally copied tree is brought back? (I won't
get too involved here on top of the other stuff, am just curious to know.)
I'm thinking, if it was allowed to contact the repos, revert could cope
using the current schema + op-root flag thing, I guess?
~Neels
RE: holes in the WC-NG schema?
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Neels J Hofmeyr [mailto:neels@elego.de]
> Sent: vrijdag 5 maart 2010 11:45
> To: dev@subversion.apache.org
> Subject: Re: holes in the WC-NG schema?
>
> Neels J Hofmeyr wrote:
> > I remember someone talking of a hole. It went something like: If a
folder is
> > copied-here, all its children have locally added status, and I
understand
> > they refer to the op-root of the add to find out their history, i.e.
that
> > they are copied. Now what if I replace such a child node with a fresh,
new
> > node -- it will still think that it's part of the copy-here. Just vague
> > memory, haven't verified. This one should be fixed if it turns out to be
so.
>
> Turns out we are aware of this problem. My noob shot at a solution would
be
> to have an indicator whether a WORKING node is the op-root of an add.
> Then
> we can have an op-root of an add within another op-root of an add without
> confusion. We might indicate on the inner op-root that they are not the
root
> of all locally added nodes, but just the root of an add operation. (Or
have
> scan_addition() find out by also scanning the parent of each add-op-root,
> which it does anyway when asked to find the repository path of the add,
> which it derives from the op-root's parent's BASE tree node ('s
ancestors).)
This would fix local adds, but it is not a complete solution for allowing
all revert scenarios from nested add with history trees. (especially the
cases where you replace some subtree of an add with history with a different
tree)
We need a more advanced schema to fix this category. The simple fix would
fix this behavior we supported in 1.6, but we might want the better fix.
Bert
Re: holes in the WC-NG schema?
Posted by Neels J Hofmeyr <ne...@elego.de>.
Neels J Hofmeyr wrote:
> I remember someone talking of a hole. It went something like: If a folder is
> copied-here, all its children have locally added status, and I understand
> they refer to the op-root of the add to find out their history, i.e. that
> they are copied. Now what if I replace such a child node with a fresh, new
> node -- it will still think that it's part of the copy-here. Just vague
> memory, haven't verified. This one should be fixed if it turns out to be so.
Turns out we are aware of this problem. My noob shot at a solution would be
to have an indicator whether a WORKING node is the op-root of an add. Then
we can have an op-root of an add within another op-root of an add without
confusion. We might indicate on the inner op-root that they are not the root
of all locally added nodes, but just the root of an add operation. (Or have
scan_addition() find out by also scanning the parent of each add-op-root,
which it does anyway when asked to find the repository path of the add,
which it derives from the op-root's parent's BASE tree node ('s ancestors).)
Because this indicator is only really needed for non-copy adds within a
copy, it is tempting to abuse some other table row that is only used for
copies. But I think we can do ourselves a favour by having an explicit
indicator instead. Overloading values with special-case meanings is Not Good
(tm). That case with svn_opt_revision_working I described in my original
post is a good example of the snake pit that overloading can become. But ok,
if we can hermetically seal off the special-case-overloading in internal
API, that would be fine with me.
(In a side note, with an explicit indicator we would not need to "fall off
the top of the NG-WORKING tree" to detect the op-root. Not that it matters /
whatever.)
[...]
> - The first place is svn_client__get_revision_number() with a revision of
> svn_opt_revision_working or _base. This function returns a (much overloaded)
> revision number, which is derived from the parent in some added-cases. In
> above case (switched-away parent from a locally added child), deriving from
> the parent is not possible anymore. Regardless, we do not want to look up
> parents at all.
> ... But chances are good that we can eliminate this case by refactoring a
> few callers, properly defining svn_opt_revision_*'s meaning for added nodes
> to be SVN_INVALID_REVNUM, and coining it an API error, or something.
[...]
~Neels
Re: holes in the WC-NG schema?
Posted by Greg Stein <gs...@gmail.com>.
On Thu, Mar 4, 2010 at 19:36, Neels J Hofmeyr <ne...@elego.de> wrote:
> Greg Stein wrote:
>> Oh no no no... the schema is fully set up to record moves properly,
>> without any guesswork. It is just that nobody will invoke the function
>> (in 1.7!) to do the recording.
>>
>> I'm comfortable that the schema will work. Please feel free to analyze
>> it for holes. But you're looking at the 'moved_here' and 'moved_to'
>> columns. You're also going to want to learn about scan_addition() and
>> scan_deletion() in more detail.
>
> I remember someone talking of a hole. It went something like: If a folder is
> copied-here, all its children have locally added status, and I understand
> they refer to the op-root of the add to find out their history, i.e. that
> they are copied. Now what if I replace such a child node with a fresh, new
> node -- it will still think that it's part of the copy-here. Just vague
> memory, haven't verified. This one should be fixed if it turns out to be so.
For an add, yes. The schema doesn't handle this yet. I'm unclear on
how a flag of "added" can't fully handle this. Bert posted something
about it, which I didn't understand. Need to think further on the
scenario.
(and, iirc, props to Philip for discovering this one)
> I also know of another potential "future hole": If I add a node, it
> remembers the URL at which it wants to be added, which is derived from the
> parent during the 'add' operation. If I then even switch the parent away
> --depth=empty, the added child node still remembers its original URL. That
> is pretty cool. But, we have two places where we also may want to know the
> *revision* that the parent had at the time of the 'add' (which should also
> step up when 'svn update's happen):
IMO, that is a bug of wc-1. Today, if you switch the parent, then the
add will go to the new repository location.
Added/copied/moved nodes do not have a repository location (intended
or not) until they are committed. Their target repository location is
identified by the parent.
And all that said, I seem to recall that you are not allowed to switch
a node if there are local changes. Could be wrong, but I do know we
don't "rememeber" the old URL any more.
>...
I'm not sure how your two scenarios apply, since the memory is not present.
>...
> I'm, thirdly, wondering if we can really express all sorts of madly
> cascaded/circular local copies/moves/deletes/replaces from/to subtrees that
> make sense to our users... a proper test suite will tell, right, stsp? ;)
We should be able to. I've thought about a lot of these scenarios...
witness the madhouse docstring of scan_addition and scan_deletion.
Some nasty scenarios.
But that's also why I encourage a review. I certainly missed the
add-within-a-copy/move.
Cheers,
-g
holes in the WC-NG schema?
Posted by Neels J Hofmeyr <ne...@elego.de>.
Greg Stein wrote:
> Oh no no no... the schema is fully set up to record moves properly,
> without any guesswork. It is just that nobody will invoke the function
> (in 1.7!) to do the recording.
>
> I'm comfortable that the schema will work. Please feel free to analyze
> it for holes. But you're looking at the 'moved_here' and 'moved_to'
> columns. You're also going to want to learn about scan_addition() and
> scan_deletion() in more detail.
I remember someone talking of a hole. It went something like: If a folder is
copied-here, all its children have locally added status, and I understand
they refer to the op-root of the add to find out their history, i.e. that
they are copied. Now what if I replace such a child node with a fresh, new
node -- it will still think that it's part of the copy-here. Just vague
memory, haven't verified. This one should be fixed if it turns out to be so.
I also know of another potential "future hole": If I add a node, it
remembers the URL at which it wants to be added, which is derived from the
parent during the 'add' operation. If I then even switch the parent away
--depth=empty, the added child node still remembers its original URL. That
is pretty cool. But, we have two places where we also may want to know the
*revision* that the parent had at the time of the 'add' (which should also
step up when 'svn update's happen):
- The first place is svn_client__get_revision_number() with a revision of
svn_opt_revision_working or _base. This function returns a (much overloaded)
revision number, which is derived from the parent in some added-cases. In
above case (switched-away parent from a locally added child), deriving from
the parent is not possible anymore. Regardless, we do not want to look up
parents at all.
... But chances are good that we can eliminate this case by refactoring a
few callers, properly defining svn_opt_revision_*'s meaning for added nodes
to be SVN_INVALID_REVNUM, and coining it an API error, or something.
- The second place is tree-conflict info. (This is a slightly odd concept,
but it makes sense:) If a tree-conflict happened on a locally added node,
'svn info' would like to tell the user at which URL@REV this added node was
*known to not exist*. We can get the URL easily, today. But the revision is
missing. That's why 'info' currently just prints "(none)" in that case.
I'm loosely thinking of an "added-at-rev" column in the NG-WORKING table,
which is derived from the parent during 'svn add', and is updated during
'svn update' and 'svn switch' (here --depth=infinity).
But frankly, this info output is not that pressing really. Just noting this
to indicate that this particular useful information is currently not
available in the schema.
(There is another situation that is related -- saying this here because it
illustrates how "real code" can just "cheat" around not having an
added-onto-revision. Philosophical experiment: If we had this data of
'revision-onto-which-node-is-added', a locally added node may,
hypothetically, *be detected to be out-of-date* in the same way that locally
modified nodes are; that would force an 'svn up', which would cause a
tree-conflict if this path has also been added in the repos. Currently, we
don't detect that, try to commit, which fails with an error and *not* a
tree-conflict. In reality, this can simply be fixed by saying 'out-of-date'
instead of erroring when we hit a local-add vs. repos-add during commit; it
obviously doesn't really need the revision.)
I'm, thirdly, wondering if we can really express all sorts of madly
cascaded/circular local copies/moves/deletes/replaces from/to subtrees that
make sense to our users... a proper test suite will tell, right, stsp? ;)
~Neels
Re: renames/moves in wc-ng (was: (Re: [PATCH] wc-ng - Remove use of
entry_t from) calculate_target_mergeinfo
Posted by Greg Stein <gs...@gmail.com>.
On Tue, Mar 2, 2010 at 08:48, Stefan Sperling <st...@elego.de> wrote:
> On Tue, Mar 02, 2010 at 07:53:30AM -0500, Greg Stein wrote:
>...
>> We do *not* record moves today.
>>
>> We *will* at some point in the future, which is most likely *after*
>> the 1.7 release.
>
> So the question of how exactly we'll trace moves is still up in the air?
>
> I've been pondering whether we should use a git-like approach of guessing
> moves at commit time based on content, rather than explicitly tracking
> what the user did. Recording tree refactorings explicitly is very
> complex and prone to failure. E.g. tracking directory replacements plus
> additional post-replace refactorings inside replaced directories is
> giving me headaches, and I have doubts that our current wc_db schema is
> up to this task. I'd rather like to avoid this complexity if possible.
> Since we'll have content checksums in the pristine store anyway, maybe
> using a lazy content-based guessing approach in addition to copyfrom
> info will be fine if we can figure out a way to make it track directory
> renames as well?
Oh no no no... the schema is fully set up to record moves properly,
without any guesswork. It is just that nobody will invoke the function
(in 1.7!) to do the recording.
I'm comfortable that the schema will work. Please feel free to analyze
it for holes. But you're looking at the 'moved_here' and 'moved_to'
columns. You're also going to want to learn about scan_addition() and
scan_deletion() in more detail.
Doing moves at commit time also implies you've updated the repository
to understand that, which is a completely separate problem. Not to
mention the editor interface between the WC and the RA layers.
Cheers,
-g
renames/moves in wc-ng (was: (Re: [PATCH] wc-ng - Remove use of
entry_t from) calculate_target_mergeinfo
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Mar 02, 2010 at 07:53:30AM -0500, Greg Stein wrote:
> On Tue, Mar 2, 2010 at 07:42, Philip Martin <ph...@wandisco.com> wrote:
> > Greg Stein <gs...@gmail.com> writes:
> >
> >> Thus, the notion of moved_here is not all that relevant because I
> >> would hope this function won't survive to the point where we start
> >> recording moves in wc_db (so, thus, we'll never record/generate that
> >> status).
> >
> > I don't understand that paragraph. Do we record moves now? Are we
> > going to record moves in the future? You seem to imply that we will
> > in future record moves and simultaneously that we won't generate a
> > svn_wc__db_status_moved_here. It doesn't really seem to make sense.
>
> We do *not* record moves today.
>
> We *will* at some point in the future, which is most likely *after*
> the 1.7 release.
So the question of how exactly we'll trace moves is still up in the air?
I've been pondering whether we should use a git-like approach of guessing
moves at commit time based on content, rather than explicitly tracking
what the user did. Recording tree refactorings explicitly is very
complex and prone to failure. E.g. tracking directory replacements plus
additional post-replace refactorings inside replaced directories is
giving me headaches, and I have doubts that our current wc_db schema is
up to this task. I'd rather like to avoid this complexity if possible.
Since we'll have content checksums in the pristine store anyway, maybe
using a lazy content-based guessing approach in addition to copyfrom
info will be fine if we can figure out a way to make it track directory
renames as well?
Stefan
renames/moves in wc-ng (was: (Re: [PATCH] wc-ng - Remove use of
entry_t from) calculate_target_mergeinfo
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Mar 02, 2010 at 07:53:30AM -0500, Greg Stein wrote:
> On Tue, Mar 2, 2010 at 07:42, Philip Martin <ph...@wandisco.com> wrote:
> > Greg Stein <gs...@gmail.com> writes:
> >
> >> Thus, the notion of moved_here is not all that relevant because I
> >> would hope this function won't survive to the point where we start
> >> recording moves in wc_db (so, thus, we'll never record/generate that
> >> status).
> >
> > I don't understand that paragraph. Do we record moves now? Are we
> > going to record moves in the future? You seem to imply that we will
> > in future record moves and simultaneously that we won't generate a
> > svn_wc__db_status_moved_here. It doesn't really seem to make sense.
>
> We do *not* record moves today.
>
> We *will* at some point in the future, which is most likely *after*
> the 1.7 release.
So the question of how exactly we'll trace moves is still up in the air?
I've been pondering whether we should use a git-like approach of guessing
moves at commit time based on content, rather than explicitly tracking
what the user did. Recording tree refactorings explicitly is very
complex and prone to failure. E.g. tracking directory replacements plus
additional post-replace refactorings inside replaced directories is
giving me headaches, and I have doubts that our current wc_db schema is
up to this task. I'd rather like to avoid this complexity if possible.
Since we'll have content checksums in the pristine store anyway, maybe
using a lazy content-based guessing approach in addition to copyfrom
info will be fine if we can figure out a way to make it track directory
renames as well?
Stefan
Re: [PATCH] wc-ng - Remove use of entry_t from calculate_target_mergeinfo
Posted by Greg Stein <gs...@gmail.com>.
On Tue, Mar 2, 2010 at 07:53, Greg Stein <gs...@gmail.com> wrote:
> On Tue, Mar 2, 2010 at 07:42, Philip Martin <ph...@wandisco.com> wrote:
>> Greg Stein <gs...@gmail.com> writes:
>>
>>> Thus, the notion of moved_here is not all that relevant because I
>>> would hope this function won't survive to the point where we start
>>> recording moves in wc_db (so, thus, we'll never record/generate that
>>> status).
>>
>> I don't understand that paragraph. Do we record moves now? Are we
>> going to record moves in the future? You seem to imply that we will
>> in future record moves and simultaneously that we won't generate a
>> svn_wc__db_status_moved_here. It doesn't really seem to make sense.
>
> We do *not* record moves today.
>
> We *will* at some point in the future, which is most likely *after*
> the 1.7 release.
>
> The node functions will *hopefully* die *before* 1.7.
>
> Thus, moved_here is not a strongly relevant concept for the node
> functions, *unless* we decide a given function's semantic is truly
> material and should survive as a (semi?) public WC API for use by
> libsvn_client.
And that said, we *are* trying to accommodate that returned status
wherever possible. As much code as possible should be
reading/expecting it to happen. It just won't for a while.
I think my point is that we can cut corners in the node functions, in
this particular case.
Cheers,
-g
Re: [PATCH] wc-ng - Remove use of entry_t from calculate_target_mergeinfo
Posted by Greg Stein <gs...@gmail.com>.
On Tue, Mar 2, 2010 at 07:42, Philip Martin <ph...@wandisco.com> wrote:
> Greg Stein <gs...@gmail.com> writes:
>
>> Thus, the notion of moved_here is not all that relevant because I
>> would hope this function won't survive to the point where we start
>> recording moves in wc_db (so, thus, we'll never record/generate that
>> status).
>
> I don't understand that paragraph. Do we record moves now? Are we
> going to record moves in the future? You seem to imply that we will
> in future record moves and simultaneously that we won't generate a
> svn_wc__db_status_moved_here. It doesn't really seem to make sense.
We do *not* record moves today.
We *will* at some point in the future, which is most likely *after*
the 1.7 release.
The node functions will *hopefully* die *before* 1.7.
Thus, moved_here is not a strongly relevant concept for the node
functions, *unless* we decide a given function's semantic is truly
material and should survive as a (semi?) public WC API for use by
libsvn_client.
Cheers,
-g
Re: [PATCH] wc-ng - Remove use of entry_t from calculate_target_mergeinfo
Posted by Philip Martin <ph...@wandisco.com>.
Greg Stein <gs...@gmail.com> writes:
> Thus, the notion of moved_here is not all that relevant because I
> would hope this function won't survive to the point where we start
> recording moves in wc_db (so, thus, we'll never record/generate that
> status).
I don't understand that paragraph. Do we record moves now? Are we
going to record moves in the future? You seem to imply that we will
in future record moves and simultaneously that we won't generate a
svn_wc__db_status_moved_here. It doesn't really seem to make sense.
--
Philip
Re: [PATCH] wc-ng - Remove use of entry_t from calculate_target_mergeinfo
Posted by Greg Stein <gs...@gmail.com>.
On Tue, Mar 2, 2010 at 05:39, Philip Martin <ph...@wandisco.com> wrote:
> Matthew Bentham <mj...@artvps.com> writes:
>
>> static svn_error_t *
>> calculate_target_mergeinfo(svn_ra_session_t *ra_session,
>> apr_hash_t **target_mergeinfo,
>> svn_wc_adm_access_t *adm_access,
>> const char *src_path_or_url,
>> svn_revnum_t src_revnum,
>> - svn_boolean_t no_repos_access,
>> svn_client_ctx_t *ctx,
>> apr_pool_t *pool)
>> {
>> - const svn_wc_entry_t *entry = NULL;
>> svn_boolean_t locally_added = FALSE;
>> const char *src_url;
>> apr_hash_t *src_mergeinfo = NULL;
>> @@ -94,13 +90,15 @@
>> bother checking. */
>> if (adm_access)
>> {
>> + svn_boolean_t added, copied;
>> const char *local_abspath;
>>
>> SVN_ERR(svn_dirent_get_absolute(&local_abspath, src_path_or_url, pool));
>> - SVN_ERR(svn_wc__get_entry_versioned(&entry, ctx->wc_ctx, local_abspath,
>> - svn_node_unknown, FALSE, FALSE,
>> - pool, pool));
>> - if (entry->schedule == svn_wc_schedule_add && (! entry->copied))
>> + SVN_ERR(svn_wc__node_is_status_added(&added, ctx->wc_ctx,
>> + local_abspath, pool));
>> + SVN_ERR(svn_wc__node_is_status_copied(&copied, ctx->wc_ctx,
>> + local_abspath, pool));
>
> It looks a bit odd to call _added just before _copied when _copied
> itself calls _added.
>
>> + if (added && !copied)
>> {
>> locally_added = TRUE;
>> }
>
>> +svn_wc__node_is_status_copied(svn_boolean_t *is_copied,
>> + svn_wc_context_t *wc_ctx,
>> + const char *local_abspath,
>> + apr_pool_t *scratch_pool)
>> +{
>> + svn_wc__db_status_t status;
>> + svn_boolean_t added;
>> +
>> + SVN_ERR(svn_wc__node_is_status_added(&added, wc_ctx, local_abspath,
>> + scratch_pool));
>> + if (!added)
>> + {
>> + *is_copied = FALSE;
>> + return SVN_NO_ERROR;
>> + }
>> +
>> + SVN_ERR(svn_wc__db_scan_addition(&status,
>> + NULL, NULL, NULL, NULL,
>> + NULL, NULL, NULL, NULL,
>> + wc_ctx->db, local_abspath,
>> + scratch_pool, scratch_pool));
>> + *is_copied = (status == svn_wc__db_status_copied);
>
> What about svn_wc__db_status_moved_here? That's a copy where the
> source happened to be deleted as well. What should _is_status_copied
> return? How should calculate_target_mergeinfo handle _moved_here?
> Like copied or like added?
The "node" functions will *hopefully* not survive to the 1.7 release.
Their intent is merely to provide a stopgap measure between "the world
of entries" and "the world of wc-ng". Getting us away from entry_t is
higher priority than getting a perfect reimplementation within terms
of wc_db and friends.
Thus, the notion of moved_here is not all that relevant because I
would hope this function won't survive to the point where we start
recording moves in wc_db (so, thus, we'll never record/generate that
status).
Once we lose all refs to entry_t, there is a lot of
compat/infrastructure that can be ripped out to *really* clean up our
code, and allow us to start moving forward with new kinds of progress.
But we will also need to come back to either rip/rebuild "node"-based
code, or to formalize the semantics of those functions.
Note that ^/trunk/tools/dev/wc-ng/count-progress.py includes
references to the node functions as something to eliminate "before
release".
Cheers,
-g
Re: [PATCH] wc-ng - Remove use of entry_t from
calculate_target_mergeinfo
Posted by Julian Foad <ju...@wandisco.com>.
Philip Martin wrote:
> Matthew Bentham <mj...@artvps.com> writes:
> > I understood the plan to be that for now we add small
> > routines that call __db routines internally?
>
> That still leaves the question about _status_moved_here. The original
> code checked entry->copied and I think that would have included moves,
> since they were just copy+delete, I think you will need
> svn_wc__node_is_status_moved as well. Perhaps svn_wc__node_get_status
> would be better?
We need to start being precise every time we use a word such as "copied"
in a doc string. Does the meaning include moved? Does it include being
the child or grandchild of a directory that was copied? And so on.
Rather than trying to write a precise prose description in each doc
string, we should strive to cross-reference to some place where the
required meaning is (or should be) documented: in this case, for
example, we could say "copied in the sense of
#svn_wc__db_status_copied", and then that gives the reader a reference
to find the precise meaning.
- Julian
Re: [PATCH] wc-ng - Remove use of entry_t from calculate_target_mergeinfo
Posted by Philip Martin <ph...@wandisco.com>.
Matthew Bentham <mj...@artvps.com> writes:
>> Perhaps calculate_target_mergeinfo should simply call _scan_addition
>> to get the added/copied/moved status?
>
> It could do, but _scan_addition is private to libsvn_wc right now, like
> _read_info (and indeed _status_copied and the other status
> enumerations).
Ok. I didn't spot that.
> I understood the plan to be that for now we add small
> routines that call __db routines internally?
That still leaves the question about _status_moved_here. The original
code checked entry->copied and I think that would have included moves,
since they were just copy+delete, I think you will need
svn_wc__node_is_status_moved as well. Perhaps svn_wc__node_get_status
would be better?
--
Philip
Re: [PATCH] wc-ng - Remove use of entry_t from calculate_target_mergeinfo
Posted by Matthew Bentham <mj...@artvps.com>.
On 02/03/2010 10:39, Philip Martin wrote:
> Matthew Bentham<mj...@artvps.com> writes:
>
>> +svn_wc__node_is_status_copied(svn_boolean_t *is_copied,
>> + svn_wc_context_t *wc_ctx,
>> + const char *local_abspath,
>> + apr_pool_t *scratch_pool)
>> +{
>> + svn_wc__db_status_t status;
>> + svn_boolean_t added;
>> +
>> + SVN_ERR(svn_wc__node_is_status_added(&added, wc_ctx, local_abspath,
>> + scratch_pool));
>> + if (!added)
>> + {
>> + *is_copied = FALSE;
>> + return SVN_NO_ERROR;
>> + }
>> +
>> + SVN_ERR(svn_wc__db_scan_addition(&status,
>> + NULL, NULL, NULL, NULL,
>> + NULL, NULL, NULL, NULL,
>> + wc_ctx->db, local_abspath,
>> + scratch_pool, scratch_pool));
>> + *is_copied = (status == svn_wc__db_status_copied);
>
> What about svn_wc__db_status_moved_here? That's a copy where the
> source happened to be deleted as well. What should _is_status_copied
> return? How should calculate_target_mergeinfo handle _moved_here?
> Like copied or like added?
>
>> +
>> + return SVN_NO_ERROR;
>
> Perhaps calculate_target_mergeinfo should simply call _scan_addition
> to get the added/copied/moved status?
It could do, but _scan_addition is private to libsvn_wc right now, like
_read_info (and indeed _status_copied and the other status
enumerations). I understood the plan to be that for now we add small
routines that call __db routines internally?
Matthew
Re: [PATCH] wc-ng - Remove use of entry_t from calculate_target_mergeinfo
Posted by Philip Martin <ph...@wandisco.com>.
Matthew Bentham <mj...@artvps.com> writes:
> static svn_error_t *
> calculate_target_mergeinfo(svn_ra_session_t *ra_session,
> apr_hash_t **target_mergeinfo,
> svn_wc_adm_access_t *adm_access,
> const char *src_path_or_url,
> svn_revnum_t src_revnum,
> - svn_boolean_t no_repos_access,
> svn_client_ctx_t *ctx,
> apr_pool_t *pool)
> {
> - const svn_wc_entry_t *entry = NULL;
> svn_boolean_t locally_added = FALSE;
> const char *src_url;
> apr_hash_t *src_mergeinfo = NULL;
> @@ -94,13 +90,15 @@
> bother checking. */
> if (adm_access)
> {
> + svn_boolean_t added, copied;
> const char *local_abspath;
>
> SVN_ERR(svn_dirent_get_absolute(&local_abspath, src_path_or_url, pool));
> - SVN_ERR(svn_wc__get_entry_versioned(&entry, ctx->wc_ctx, local_abspath,
> - svn_node_unknown, FALSE, FALSE,
> - pool, pool));
> - if (entry->schedule == svn_wc_schedule_add && (! entry->copied))
> + SVN_ERR(svn_wc__node_is_status_added(&added, ctx->wc_ctx,
> + local_abspath, pool));
> + SVN_ERR(svn_wc__node_is_status_copied(&copied, ctx->wc_ctx,
> + local_abspath, pool));
It looks a bit odd to call _added just before _copied when _copied
itself calls _added.
> + if (added && !copied)
> {
> locally_added = TRUE;
> }
> +svn_wc__node_is_status_copied(svn_boolean_t *is_copied,
> + svn_wc_context_t *wc_ctx,
> + const char *local_abspath,
> + apr_pool_t *scratch_pool)
> +{
> + svn_wc__db_status_t status;
> + svn_boolean_t added;
> +
> + SVN_ERR(svn_wc__node_is_status_added(&added, wc_ctx, local_abspath,
> + scratch_pool));
> + if (!added)
> + {
> + *is_copied = FALSE;
> + return SVN_NO_ERROR;
> + }
> +
> + SVN_ERR(svn_wc__db_scan_addition(&status,
> + NULL, NULL, NULL, NULL,
> + NULL, NULL, NULL, NULL,
> + wc_ctx->db, local_abspath,
> + scratch_pool, scratch_pool));
> + *is_copied = (status == svn_wc__db_status_copied);
What about svn_wc__db_status_moved_here? That's a copy where the
source happened to be deleted as well. What should _is_status_copied
return? How should calculate_target_mergeinfo handle _moved_here?
Like copied or like added?
> +
> + return SVN_NO_ERROR;
Perhaps calculate_target_mergeinfo should simply call _scan_addition
to get the added/copied/moved status?
--
Philip
Re: [PATCH] wc-ng - Remove use of entry_t from calculate_target_mergeinfo
Posted by Matthew Bentham <mj...@artvps.com>.
On 02/03/2010 17:18, Matthew Bentham wrote:
> On 02/03/2010 13:40, Matthew Bentham wrote:
>> On 02/03/2010 13:06, Greg Stein wrote:
...
>>> On Tue, Mar 2, 2010 at 04:45, Matthew Bentham<mj...@artvps.com> wrote:
>>> Because of the fragility around entry->copied, I can't really tell
>>> whether this patch is Right from a simple inspection. I presume that
>>> you've run the full test suite, and it continues to pass with this
>>> change?
>>
...
>> At the moment I'm trying to write a test that passes in trunk, and fails
>> if I force 'copied' to 'FALSE'.
>
> I've set up a Linux development environment and run the whole test
> suite. There are no test failures if all adds are treated as local adds
> in calculate_target_mergeinfo. So I think I need to work on improving
> the test coverage before I can go back to removing entry_t here.
I need help with this change. I can't figure out what
'calculate_target_mergeinfo' is supposed to be doing in the case where
"entry->copied == TRUE", so I can't figure out how to test it.
I _thought_ that it was trying to find inherited mergeinfo of the source
of a copy, and combine that into the explicit mergeinfo of the target of
the copy.
So I expected this to happen in a trunk build (in a test sandbox wc):
[[[
-bash-3.2$ /localdata/mjb67/svninstall/bin/svn cp A D
A D
-bash-3.2$ cat >> A/mu
foo
bar
-bash-3.2$ /localdata/mjb67/svninstall/bin/svn commit -m "" A
Sending A/mu
Transmitting file data .
Committed revision 18.
-bash-3.2$ /localdata/mjb67/svninstall/bin/svn merge -r 17:18 A D
--- Merging r18 into 'D':
U D/mu
--- Recording mergeinfo for merge of r18 into 'D':
U D
-bash-3.2$ /localdata/mjb67/svninstall/bin/svn mergeinfo A/mu D/mu
r18
-bash-3.2$ /localdata/mjb67/svninstall/bin/svn cp D/mu mu
A mu
-bash-3.2$ /localdata/mjb67/svninstall/bin/svn mergeinfo A/mu mu
**I expected to see "r18" here, but there is nothing **
]]]
calculate_target_mergeinfo only seems to get called when the source, or
target, or both, of the copy is in the repository, so I tried this:
[[[
-bash-3.2$ /localdata/mjb67/svninstall/bin/svn cp D E
A E
-bash-3.2$ /localdata/mjb67/svninstall/bin/svn mergeinfo A/mu E/mu
r18
-bash-3.2$ /localdata/mjb67/svninstall/bin/svn cp -m "" E/mu
file:///localdata/mjb67/subversion/builddir/subversion/tests/cmdline/svn-test-work/repositories/copy_tests-49/A/mumu
../subversion/svn/copy-cmd.c:144: (apr_err=160013)
../subversion/libsvn_client/copy.c:2177: (apr_err=160013)
../subversion/libsvn_client/copy.c:1372: (apr_err=160013)
../subversion/libsvn_client/copy.c:1372: (apr_err=160013)
../subversion/libsvn_client/copy.c:135: (apr_err=160013)
../subversion/libsvn_client/mergeinfo.c:455: (apr_err=160013)
../subversion/libsvn_client/mergeinfo.c:499: (apr_err=160013)
../subversion/libsvn_ra_local/ra_plugin.c:703: (apr_err=160013)
../subversion/libsvn_repos/fs-wrap.c:633: (apr_err=160013)
../subversion/libsvn_fs_fs/tree.c:3651: (apr_err=160013)
../subversion/libsvn_fs_fs/tree.c:3519: (apr_err=160013)
../subversion/libsvn_fs_fs/tree.c:669: (apr_err=160013)
svn: File not found: revision 19, path '/E/mu'
]]]
This stack is interesting, because copy.c:135 is in
calculate_target_mergeinfo, in the case where it tries to get the
source's mergeinfo from the repository.
So I'm leaning to the conclusion that whatever it's trying to do in this
case isn't working anyway, which kind of leaves me at a loose end in
terms of improving the testing and removing entry_t use.
Any ideas?
Matthew
Re: [PATCH] wc-ng - Remove use of entry_t from calculate_target_mergeinfo
Posted by Matthew Bentham <mj...@artvps.com>.
On 02/03/2010 13:40, Matthew Bentham wrote:
> On 02/03/2010 13:06, Greg Stein wrote:
>> On Tue, Mar 2, 2010 at 04:45, Matthew Bentham<mj...@artvps.com> wrote:
>>> Trying to remove more uses of svn_wc_entry_t, it seems there is no way at
>>> the moment to get the equivalent of entry->copied using node routines. Is
>>> this the sort of thing that is needed?
>>>
>>> [[[
>>> wc-ng: work towards eliminating svn_wc_entry_t
>>>
>>> * libsvn_wc/node.c, include/private/svn_wc_private.h
>>> Add svn_wc__node_is_status_copied()
>>>
>>> * subversion/libsvn_client/copy.c
>>> (calculate_target_mergeinfo): Remove unused 'no_repos_access'
>>> parameter, replace use of svn_wc__get_entry with node routines.
>>>
>>> Patch by: Matthew Bentham<mjb67{_AT_}artvps.com>
>>> ]]]
>>
>> Hey...
>>
>> Outside of the issues around the node functions that are being
>> discussed, I kind of worry about this code. entry->copied does not
>> truly correspond to status_copied (or status_moved_here). If you look
>> in entries.c where entry->copied is *set*, then you'll see the logic
>> is rather ugly and opaque and (from a human brain's standpoint) maybe
>> even non-deterministic :-P
>>
>> When walking into this area, it is helpful to step back and ensure
>> that the merge code is really looking for the right thing. It is
>> entirely possible it was looking at entry->copied, when it should have
>> been looking for status_(copied|moved_here).
>
> I _think_ it just needs to know whether this is an add with history.
>
>> Because of the fragility around entry->copied, I can't really tell
>> whether this patch is Right from a simple inspection. I presume that
>> you've run the full test suite, and it continues to pass with this
>> change?
>
> Mmmm, I've been running a lot of the tests, but if I run the full test
> suite it grinds to a halt after about 10hrs (and doesn't finish even
> after 28hrs, the longest I've tried so far). So I haven't been running
> the whole thing. I will probably need to change development platform to
> something non-cygwin.
>
> Nevertheless, I'm worried that calculate_target_mergeinfo seems to be
> fairly under-tested:
>
> - If I force 'copied' to 'TRUE', (ie. force it to treat every addition
> as an add with history) the only test I've found to fail is
> copy_tests.py:copy_added_paths_to_URL.
>
> - If I force 'copied' to 'FALSE', (treat every addition as a local
> addition), I can't find a test that fails. I would have expected
> something in merge_tests.py probably, but those seem to be the tests
> which are murdering my computer (there's been some relevant discussion
> about this in another thread).
>
> At the moment I'm trying to write a test that passes in trunk, and fails
> if I force 'copied' to 'FALSE'.
I've set up a Linux development environment and run the whole test
suite. There are no test failures if all adds are treated as local adds
in calculate_target_mergeinfo. So I think I need to work on improving
the test coverage before I can go back to removing entry_t here.
Matthew
Re: [PATCH] wc-ng - Remove use of entry_t from calculate_target_mergeinfo
Posted by Matthew Bentham <mj...@artvps.com>.
On 02/03/2010 13:06, Greg Stein wrote:
> On Tue, Mar 2, 2010 at 04:45, Matthew Bentham<mj...@artvps.com> wrote:
>> Trying to remove more uses of svn_wc_entry_t, it seems there is no way at
>> the moment to get the equivalent of entry->copied using node routines. Is
>> this the sort of thing that is needed?
>>
>> [[[
>> wc-ng: work towards eliminating svn_wc_entry_t
>>
>> * libsvn_wc/node.c, include/private/svn_wc_private.h
>> Add svn_wc__node_is_status_copied()
>>
>> * subversion/libsvn_client/copy.c
>> (calculate_target_mergeinfo): Remove unused 'no_repos_access'
>> parameter, replace use of svn_wc__get_entry with node routines.
>>
>> Patch by: Matthew Bentham<mjb67{_AT_}artvps.com>
>> ]]]
>
> Hey...
>
> Outside of the issues around the node functions that are being
> discussed, I kind of worry about this code. entry->copied does not
> truly correspond to status_copied (or status_moved_here). If you look
> in entries.c where entry->copied is *set*, then you'll see the logic
> is rather ugly and opaque and (from a human brain's standpoint) maybe
> even non-deterministic :-P
>
> When walking into this area, it is helpful to step back and ensure
> that the merge code is really looking for the right thing. It is
> entirely possible it was looking at entry->copied, when it should have
> been looking for status_(copied|moved_here).
I _think_ it just needs to know whether this is an add with history.
> Because of the fragility around entry->copied, I can't really tell
> whether this patch is Right from a simple inspection. I presume that
> you've run the full test suite, and it continues to pass with this
> change?
Mmmm, I've been running a lot of the tests, but if I run the full test
suite it grinds to a halt after about 10hrs (and doesn't finish even
after 28hrs, the longest I've tried so far). So I haven't been running
the whole thing. I will probably need to change development platform to
something non-cygwin.
Nevertheless, I'm worried that calculate_target_mergeinfo seems to be
fairly under-tested:
- If I force 'copied' to 'TRUE', (ie. force it to treat every addition
as an add with history) the only test I've found to fail is
copy_tests.py:copy_added_paths_to_URL.
- If I force 'copied' to 'FALSE', (treat every addition as a local
addition), I can't find a test that fails. I would have expected
something in merge_tests.py probably, but those seem to be the tests
which are murdering my computer (there's been some relevant discussion
about this in another thread).
At the moment I'm trying to write a test that passes in trunk, and fails
if I force 'copied' to 'FALSE'.
> Assuming so, then I might request some liberal sprinking of ###
> comments to explain that the code might need further tweaking, but
> that it seems to work to expectation. That future work on the section
> may need additional review to compare against entry->copied's
> definition. Something like that.
OK.
> And thanks! I appreciate the work you're doing to eliminate the
> demonspawn known as entry_t!
:-)
Matthew
Re: [PATCH] wc-ng - Remove use of entry_t from calculate_target_mergeinfo
Posted by Greg Stein <gs...@gmail.com>.
On Tue, Mar 2, 2010 at 04:45, Matthew Bentham <mj...@artvps.com> wrote:
> Trying to remove more uses of svn_wc_entry_t, it seems there is no way at
> the moment to get the equivalent of entry->copied using node routines. Is
> this the sort of thing that is needed?
>
> [[[
> wc-ng: work towards eliminating svn_wc_entry_t
>
> * libsvn_wc/node.c, include/private/svn_wc_private.h
> Add svn_wc__node_is_status_copied()
>
> * subversion/libsvn_client/copy.c
> (calculate_target_mergeinfo): Remove unused 'no_repos_access'
> parameter, replace use of svn_wc__get_entry with node routines.
>
> Patch by: Matthew Bentham <mjb67{_AT_}artvps.com>
> ]]]
Hey...
Outside of the issues around the node functions that are being
discussed, I kind of worry about this code. entry->copied does not
truly correspond to status_copied (or status_moved_here). If you look
in entries.c where entry->copied is *set*, then you'll see the logic
is rather ugly and opaque and (from a human brain's standpoint) maybe
even non-deterministic :-P
When walking into this area, it is helpful to step back and ensure
that the merge code is really looking for the right thing. It is
entirely possible it was looking at entry->copied, when it should have
been looking for status_(copied|moved_here).
Because of the fragility around entry->copied, I can't really tell
whether this patch is Right from a simple inspection. I presume that
you've run the full test suite, and it continues to pass with this
change?
Assuming so, then I might request some liberal sprinking of ###
comments to explain that the code might need further tweaking, but
that it seems to work to expectation. That future work on the section
may need additional review to compare against entry->copied's
definition. Something like that.
And thanks! I appreciate the work you're doing to eliminate the
demonspawn known as entry_t!
Cheers,
-g