You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Neels J Hofmeyr <ne...@elego.de> on 2011/08/12 02:56:28 UTC
moved-to and moved-from in status output
Hi stsp,
check this WIP patch out, attached. (I've also appended a wild test script
that runs all sorts of things. Reading its bottom-most output may suffice.)
I'd prefer printing the paths relative to the "current working dir" instead
of relative to the WC root (as this patch does now).
Say there was an
[[[
~/wc$ svn mv x/alpha a/b/beta
]]]
currently:
[[[
~/wc$ cd a/b/
~/wc/a/b$ svn st
A + beta
> moved from x/alpha
]]]
(relative to WC_ROOT)
rather:
[[[
~/wc/a/b$ svn st
A + beta
> moved from ../../x/alpha
]]]
but I haven't figured out whether there's code that does this already. And,
to that end, I think those batons should rather pass abspaths (see comments
in the code).
What do you say?
~Neels
Re: moved-to and moved-from in status output
Posted by Neels J Hofmeyr <ne...@elego.de>.
On 08/12/2011 10:44 AM, Stefan Sperling wrote:
> How badly does changing this output break "make check"?
Interestingly enough, my 'make check' had *no errors* at all.
Seems like the test fu ignores extraneous lines altogether. Possibly you
already fixed all that when adding the tree conflicts info line.
The rest of your remarks will be taken care of until I return with another
patch. :)
~Neels
Re: moved-to and moved-from in status output
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Aug 12, 2011 at 02:56:28AM +0200, Neels J Hofmeyr wrote:
> Hi stsp,
>
> check this WIP patch out, attached. (I've also appended a wild test script
> that runs all sorts of things. Reading its bottom-most output may suffice.)
>
> I'd prefer printing the paths relative to the "current working dir" instead
> of relative to the WC root (as this patch does now).
>
> Say there was an
> [[[
> ~/wc$ svn mv x/alpha a/b/beta
> ]]]
>
> currently:
> [[[
> ~/wc$ cd a/b/
> ~/wc/a/b$ svn st
> A + beta
> > moved from x/alpha
> ]]]
> (relative to WC_ROOT)
>
> rather:
> [[[
> ~/wc/a/b$ svn st
> A + beta
> > moved from ../../x/alpha
> ]]]
>
> but I haven't figured out whether there's code that does this already. And,
> to that end, I think those batons should rather pass abspaths (see comments
> in the code).
>
> What do you say?
Thanks!
How badly does changing this output break "make check"?
See inline comments below.
>
> ~Neels
> First steps to show moved-to and moved-from in 'svn status'.
>
> * subversion/include/svn_client.h
>
> * subversion/include/svn_wc.h
>
> * subversion/libsvn_client/status.c
> (svn_client_status_dup):
> (svn_client__create_status):
>
> * subversion/libsvn_wc/status.c
> (assemble_status):
> (svn_wc_dup_status3):
>
> * subversion/svn/status.c
> (print_status):
> Index: subversion/include/svn_client.h
> ===================================================================
> --- subversion/include/svn_client.h (revision 1156828)
> +++ subversion/include/svn_client.h (working copy)
> @@ -2206,6 +2206,18 @@ typedef struct svn_client_status_t
> * to other data in future versions. */
> const void *backwards_compatibility_baton;
>
> + /** Set to @c true if this file or directory is scheduled for
> + * addition-with-history that is part of an explicit move operation (or part
I think we should just say "copied" instead of "add-with-history".
It doesn't matter how the copy operation is implemented, it is still a copy.
> + * of a subtree that is scheduled as such).
> + */
The boolean variable isn't declared. (Maybe the above comment should
not be here?)
> + /** Set to the path (relative to the working copy root) that this node was
> + * moved from, if this file or directory has been moved here locally. */
> + const char *moved_from_relpath;
As you said, in the APIs we should always use absolute paths.
I used relpaths in svn_wc APIs at one point, giving a bad example,
but they've since been converted to abspaths.
The 'svn' client should perform an abspath->relpath conversion
for display if needed.
> +
> + /** Set to the path (relative to the working copy root) that this node was
> + * moved to, if this file or directory has been moved away locally. */
> + const char *moved_to_relpath;
As above.
> +
> /* NOTE! Please update svn_client_status_dup() when adding new fields here. */
> } svn_client_status_t;
>
> Index: subversion/include/svn_wc.h
> ===================================================================
> --- subversion/include/svn_wc.h (revision 1156828)
> +++ subversion/include/svn_wc.h (working copy)
> @@ -3619,6 +3619,14 @@ typedef struct svn_wc_status3_t
>
> /** @} */
>
> + /* Set to the move source path if this item has been explicitly moved here
> + * (path relative to the working copy root) */
> + const char *moved_from_relpath;
Same, please use abspaths.
> +
> + /* Set to the move destination path if this item has been explicitly moved
> + * away (path relative to the working copy root) */
> + const char *moved_to_relpath;
> +
> /* NOTE! Please update svn_wc_dup_status3() when adding new fields here. */
> } svn_wc_status3_t;
>
> Index: subversion/libsvn_client/status.c
> ===================================================================
> --- subversion/libsvn_client/status.c (revision 1156828)
> +++ subversion/libsvn_client/status.c (working copy)
> @@ -563,6 +563,12 @@ svn_client_status_dup(const svn_client_s
> result_pool);
> }
>
> + if (status->moved_from_relpath)
> + st->moved_from_relpath = apr_pstrdup(result_pool, status->moved_from_relpath);
> +
> + if (status->moved_to_relpath)
> + st->moved_to_relpath = apr_pstrdup(result_pool, status->moved_to_relpath);
> +
> return st;
> }
>
> @@ -667,6 +673,9 @@ svn_client__create_status(svn_client_sta
> (*cst)->node_status = svn_wc_status_conflicted;
> }
>
> + (*cst)->moved_from_relpath = status->moved_from_relpath;
> + (*cst)->moved_to_relpath = status->moved_to_relpath;
> +
> return SVN_NO_ERROR;
> }
>
> Index: subversion/libsvn_wc/status.c
> ===================================================================
> --- subversion/libsvn_wc/status.c (revision 1156828)
> +++ subversion/libsvn_wc/status.c (working copy)
> @@ -403,6 +403,8 @@ assemble_status(svn_wc_status3_t **statu
> const char *repos_relpath;
> const char *repos_root_url;
> const char *repos_uuid;
> + const char *moved_from_relpath = NULL;
> + const char *moved_to_relpath = NULL;
> svn_filesize_t filesize = (dirent && (dirent->kind == svn_node_file))
> ? dirent->filesize
> : SVN_INVALID_FILESIZE;
> @@ -608,6 +610,42 @@ assemble_status(svn_wc_status3_t **statu
> }
> }
>
> + /* Get moved_to info. */
"moved-away" (for consistency)
> + if (info->status == svn_wc__db_status_deleted)
> + {
> + const char *moved_to_abspath = NULL;
> + SVN_ERR(svn_wc__db_scan_deletion(NULL,
> + &moved_to_abspath,
> + NULL, NULL,
> + db, local_abspath,
> + result_pool, scratch_pool));
> + if (moved_to_abspath)
> + /* ### This should probably be different, possibly returning a
> + * ### local_abspath and letting the client construct a pwd-relative
> + * ### path. */
Yes :)
> + SVN_ERR(svn_wc__db_to_relpath(&moved_to_relpath, db, local_abspath,
> + moved_to_abspath,
> + result_pool, scratch_pool));
> +
> + }
> +
> + /* Get moved_here info. */
"moved-here" (for consistency)
> + if (info->status == svn_wc__db_status_added)
> + {
Rest looks fine.
Re: moved-to and moved-from in status output
Posted by Neels J Hofmeyr <ne...@elego.de>.
On 08/13/2011 05:22 PM, Stefan Sperling wrote:
> On Sat, Aug 13, 2011 at 04:45:18PM +0200, Neels J Hofmeyr wrote:
>> I very much agree that saying "moved from revision 42" is straight nonsense.
>> Saying "Copied From" and "Moved From" in the same info output, while it may
>> be correct and sensible to us devs, is nevertheless ambiguous.
>>
>> Can you acknowledge that?
>
> Yes, that can confuse people.
Cool, thanks.
> A possible misunderstanding is that move
> information shown at the client-side will be used in the repository as-is.
> Which isn't the case.
I was thinking more about the misunderstanding that svn prints "Copied"
although the user definitely remembers having said 'svn move'. (User being
unaware that actual real moves don't exist in computer memory.)
I'm skipping the rest as I don't really understand what you're trying to say
there, and 'cause I'm eager to drop this thread :)
~Neels
> In the WC the move is relative to the BASE tree.
> But the server needs to concern itself with the question "did a move occur
> within the revision range -rN:M?",
>which is true if the revision in which the
> move was committed is in the range N:M.
> So once the move has been committed
> the BASE tree of the working copy becomes irrelevant.
Re: moved-to and moved-from in status output
Posted by Stefan Sperling <st...@elego.de>.
On Sat, Aug 13, 2011 at 04:45:18PM +0200, Neels J Hofmeyr wrote:
> I very much agree that saying "moved from revision 42" is straight nonsense.
> Saying "Copied From" and "Moved From" in the same info output, while it may
> be correct and sensible to us devs, is nevertheless ambiguous.
>
> Can you acknowledge that?
Yes, that can confuse people. A possible misunderstanding is that move
information shown at the client-side will be used in the repository as-is.
Which isn't the case. In the WC the move is relative to the BASE tree.
But the server needs to concern itself with the question "did a move occur
within the revision range -rN:M?", which is true if the revision in which the
move was committed is in the range N:M. So once the move has been committed
the BASE tree of the working copy becomes irrelevant.
Re: moved-to and moved-from in status output
Posted by Neels J Hofmeyr <ne...@elego.de>.
On 08/13/2011 12:31 PM, Stefan Sperling wrote:
> Maybe this is a more convincing argument:
>
> If the client printed 'Moved from URL@REV' it would be printing nonsense.
> It is a nonsense thing to say "I moved the node foo as found in revision 42
> to the local node bar". Revisions are immutable so foo@42 cannot be removed
> from revision 42 and added to the working copy under a different name.
yes, it is :)
> I don't think command line client should be printing information which
> can be easily misunderstood, and misleading.
This statement is a universal truth and valid to any side of any argument.
The Q is: which confuses more people? In the average user's world, printing
"copied from" when there was a known explicit local move is about as
misleading as what you described.
I very much agree that saying "moved from revision 42" is straight nonsense.
Saying "Copied From" and "Moved From" in the same info output, while it may
be correct and sensible to us devs, is nevertheless ambiguous.
Can you acknowledge that?
And then I'll let it go. Because changing it isn't worth the trouble.
(But I wasn't gonna take your lecture just like that :P You probably didn't
mean it the way it came out on my end, but it came out pretty dry indeed.)
~Neels
Re: moved-to and moved-from in status output
Posted by Stefan Sperling <st...@elego.de>.
On Sat, Aug 13, 2011 at 03:01:02AM +0200, Neels J Hofmeyr wrote:
> stsp, you are whipping out fancy words, issue numbers and mail threads mixed
> with trivial info, about things entirely unrelated to *just* printing
> "Moved" instead of "Copied" in 'svn' command line client output.
>
> You said it, copyfrom belongs in the repository. Then why go ballistic about
> whether the cmdline client prints "Copied From" or "Moved From"? A client's
> local move has nothing to do with a committed copy! Indeed!
Maybe this is a more convincing argument:
If the client printed 'Moved from URL@REV' it would be printing nonsense.
It is a nonsense thing to say "I moved the node foo as found in revision 42
to the local node bar". Revisions are immutable so foo@42 cannot be removed
from revision 42 and added to the working copy under a different name.
I don't think command line client should be printing information which
can be easily misunderstood, and misleading.
Re: moved-to and moved-from in status output
Posted by Neels J Hofmeyr <ne...@elego.de>.
On 08/12/2011 11:04 PM, Stefan Sperling wrote:
> On Fri, Aug 12, 2011 at 10:11:03PM +0200, Neels J Hofmeyr wrote:
>> Discussing 'svn info' output:
>>
>> What do you think, for moved-here items, should we rename "Copied From URL"
>> and "Copied From Rev" to "Moved From *"? Leaving "Copied From *" as-is will
>> be confusing to the average user person...
>> #svn: "lol it says both copied and moved !?!1! Should be a tree conflict!1!"
>> Same discussion pending for --xml output.
>
> No, please let's not conflate copyfrom information with move information.
>
> Move information pertains to the working copy.
> It specifies how nodes were moved relative to the BASE tree.
>
> Copyfrom information pertains to the repository.
* neels looks up 'to conflate'
* neels looks up 'to pertain'
> I don't think there will be moved-from/moved-to info in the repository
> because this would require a fix for issue #898 (which I don't intend to fix).
> There will be copy-to information, and we'll use that together with
> copyfrom to figure out whether a node A was moved to B within the delta
> between two arbitrary revisions. This approach is like issue #3630.
>
> See this IRC discussion for information on my use of "move" vs. "true
> rename" and how all this relates to issues #898 and #3630/#3631:
> http://colabti.org/irclogger/irclogger_log/svn-dev?date=2011-07-29#l171
>
This reads like the P.A.T.R.I.O.T. Act...
* neels indulges in a look-up frenzy
stsp, you are whipping out fancy words, issue numbers and mail threads mixed
with trivial info, about things entirely unrelated to *just* printing
"Moved" instead of "Copied" in 'svn' command line client output.
You said it, copyfrom belongs in the repository. Then why go ballistic about
whether the cmdline client prints "Copied From" or "Moved From"? A client's
local move has nothing to do with a committed copy! Indeed!
I'd be fine with a '-1', but I don't buy your (un-)reasoning.
>> We probably don't want to add a "Copied From Path" line: it's neither as
>> interesting nor as easy. A copy doesn't need to commit two paths in one, and
>> last time I checked we didn't store local paths for copied-from.
>>
>> BTW, do we have any ambition to provide 'svn mv' using a URL source? :)
>
> No. Move information is only valid within the working copy.
Again, I get the impression that your mind has overlayed my question with an
unrelated concept you feel strongly about... But yeah, let's drop the 'svn
mv'-from-URL, indefinitely.
(though it *could* be a convenience thing that does a remote-delete + a
local-add-with-history, upon which you could've said "But that wouldn't be
committed in a single rev!", and you'd've been right about *that*.)
~Neels
Re: moved-to and moved-from in status output
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Aug 12, 2011 at 10:11:03PM +0200, Neels J Hofmeyr wrote:
> Discussing 'svn info' output:
>
> What do you think, for moved-here items, should we rename "Copied From URL"
> and "Copied From Rev" to "Moved From *"? Leaving "Copied From *" as-is will
> be confusing to the average user person...
> #svn: "lol it says both copied and moved !?!1! Should be a tree conflict!1!"
> Same discussion pending for --xml output.
No, please let's not conflate copyfrom information with move information.
Move information pertains to the working copy.
It specifies how nodes were moved relative to the BASE tree.
Copyfrom information pertains to the repository.
I don't think there will be moved-from/moved-to info in the repository
because this would require a fix for issue #898 (which I don't intend to fix).
There will be copy-to information, and we'll use that together with
copyfrom to figure out whether a node A was moved to B within the delta
between two arbitrary revisions. This approach is like issue #3630.
See this IRC discussion for information on my use of "move" vs. "true
rename" and how all this relates to issues #898 and #3630/#3631:
http://colabti.org/irclogger/irclogger_log/svn-dev?date=2011-07-29#l171
> We probably don't want to add a "Copied From Path" line: it's neither as
> interesting nor as easy. A copy doesn't need to commit two paths in one, and
> last time I checked we didn't store local paths for copied-from.
>
> BTW, do we have any ambition to provide 'svn mv' using a URL source? :)
No. Move information is only valid within the working copy.
Re: moved-to and moved-from in status output
Posted by Neels J Hofmeyr <ne...@elego.de>.
On 08/12/2011 12:55 PM, Julian Foad wrote:
> Stefan Sperling wrote:
>> On Fri, Aug 12, 2011 at 08:33:43AM +0100, Julian Foad wrote:
>>> Neels J Hofmeyr wrote:
>>>> Hi stsp,
>>>>
>>>> check this WIP patch out, attached. (I've also appended a wild test script
>>>> that runs all sorts of things. Reading its bottom-most output may suffice.)
>>>>
>>>> I'd prefer printing the paths relative to the "current working dir" instead
>>>> of relative to the WC root (as this patch does now).
>>>
>>> +1, for consistency with other 'svn' output.
That comma makes all the difference :)
>>
>> If this is done for 'svn status' the same should be done for 'svn info'
>> which currently prints moved-from/-to relative the WC root as well.
> Ah, right, I should have spotted that. +1 to both.
Discussing 'svn info' output:
What do you think, for moved-here items, should we rename "Copied From URL"
and "Copied From Rev" to "Moved From *"? Leaving "Copied From *" as-is will
be confusing to the average user person...
#svn: "lol it says both copied and moved !?!1! Should be a tree conflict!1!"
Same discussion pending for --xml output.
We probably don't want to add a "Copied From Path" line: it's neither as
interesting nor as easy. A copy doesn't need to commit two paths in one, and
last time I checked we didn't store local paths for copied-from.
BTW, do we have any ambition to provide 'svn mv' using a URL source? :)
~Neels
Re: moved-to and moved-from in status output
Posted by Julian Foad <ju...@wandisco.com>.
Stefan Sperling wrote:
> On Fri, Aug 12, 2011 at 08:33:43AM +0100, Julian Foad wrote:
> > Neels J Hofmeyr wrote:
> > > Hi stsp,
> > >
> > > check this WIP patch out, attached. (I've also appended a wild test script
> > > that runs all sorts of things. Reading its bottom-most output may suffice.)
> > >
> > > I'd prefer printing the paths relative to the "current working dir" instead
> > > of relative to the WC root (as this patch does now).
> >
> > +1, for consistency with other 'svn' output.
>
> If this is done for 'svn status' the same should be done for 'svn info'
> which currently prints moved-from/-to relative the WC root as well.
Ah, right, I should have spotted that. +1 to both.
- Julian
Re: moved-to and moved-from in status output
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Aug 12, 2011 at 08:33:43AM +0100, Julian Foad wrote:
> Neels J Hofmeyr wrote:
> > Hi stsp,
> >
> > check this WIP patch out, attached. (I've also appended a wild test script
> > that runs all sorts of things. Reading its bottom-most output may suffice.)
> >
> > I'd prefer printing the paths relative to the "current working dir" instead
> > of relative to the WC root (as this patch does now).
>
> +1, for consistency with other 'svn' output.
If this is done for 'svn status' the same should be done for 'svn info'
which currently prints moved-from/-to relative the WC root as well.
Re: moved-to and moved-from in status output
Posted by Julian Foad <ju...@wandisco.com>.
Neels J Hofmeyr wrote:
> Hi stsp,
>
> check this WIP patch out, attached. (I've also appended a wild test script
> that runs all sorts of things. Reading its bottom-most output may suffice.)
>
> I'd prefer printing the paths relative to the "current working dir" instead
> of relative to the WC root (as this patch does now).
+1, for consistency with other 'svn' output.
> currently:
> [[[
> ~/wc$ cd a/b/
> ~/wc/a/b$ svn st
> A + beta
> > moved from x/alpha
> ]]]
> (relative to WC_ROOT)
>
> rather:
> [[[
> ~/wc/a/b$ svn st
> A + beta
> > moved from ../../x/alpha
A side-note about showing ".." in the output:
When the relpath would involve ".." components as in this example, then
"moved from /abs/path/to/x/alpha" would be an acceptable alternative. I
don't recall us having any code at the moment for generating a "../"
output as the difference of two abspaths. We do display such outputs
sometimes, but I think only when we append a relpath to a "../" style
path that was given as input:
$ (cd subversion/libsvn_client/ && svn st ../libsvn_fs)
M ../libsvn_fs/access.c
I would not be opposed to adding and using some code that can generate a
"../" path as the difference between two (abs|rel)paths.
- Julian
> ]]]
>
> but I haven't figured out whether there's code that does this already. And,
> to that end, I think those batons should rather pass abspaths (see comments
> in the code).
>
> What do you say?
>
> ~Neels