You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Stefan Sperling <st...@apache.org> on 2016/04/21 09:24:09 UTC
Re: svn commit: r1740170
-/subversion/trunk/subversion/libsvn_client/conflicts.c
On Wed, Apr 20, 2016 at 10:19:56PM +0200, Bert Huijben wrote:
> The path calculation here 100% assumes that the working copy is always a clean check out from ^/. This might be the case in our test suite, but in almost every normal user scenario this isn’t the case.
>
> You can’t just calculate a repository root relative path ( ^/… ) from dirents. You always need the repos_relpath from somewhere else.
>
> And once you start looking at switched files/directories, things get even harder.
>
>
> Bert
>
I'm not sure what you mean. Perhaps you're lacking some additional
context for this diff?
What's happening here is that we show a message like:
delete X and copy ^/Y@Z here
where X is a local conflicted path (taken from the local_abspath which is
being resolved), Y is a repos-relpath stored in the conflict descriptor,
and Z is a peg revision stored in the conflict descriptor.
Assuming Y and Z were stored correctly when the conflict was created (the
conflict resolver has no reason to assume otherwise), I don't see where
this extrapolates a local path to a repostory path.
Can you elaborate? Are you saying that's wrong to say "delete X" because
X might not come from path ^/somewhere/X in the repository?
> From: stsp@apache.org
> Sent: woensdag 20 april 2016 19:05
> To: commits@subversion.apache.org
> Subject: svn commit: r1740170 -/subversion/trunk/subversion/libsvn_client/conflicts.c
>
> Author: stsp
> Date: Wed Apr 20 17:05:04 2016
> New Revision: 1740170
>
> URL: http://svn.apache.org/viewvc?rev=1740170&view=rev
> Log:
> * subversion/libsvn_client/conflicts.c
> (configure_option_merge_incoming_added_file_replace): Tweak this resolution
> option's description slightly.
>
> Modified:
> subversion/trunk/subversion/libsvn_client/conflicts.c
>
> Modified: subversion/trunk/subversion/libsvn_client/conflicts.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/conflicts.c?rev=1740170&r1=1740169&r2=1740170&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_client/conflicts.c (original)
> +++ subversion/trunk/subversion/libsvn_client/conflicts.c Wed Apr 20 17:05:04 2016
> @@ -4527,7 +4527,7 @@ configure_option_merge_incoming_added_fi
> conflict->local_abspath, scratch_pool,
> scratch_pool));
> option->description =
> - apr_psprintf(options->pool, _("delete '%s', copy '^/%s@%ld' here"),
> + apr_psprintf(options->pool, _("delete '%s' and copy '^/%s@%ld' here"),
> svn_dirent_local_style(
> svn_dirent_skip_ancestor(wcroot_abspath,
> conflict->local_abspath),
>
>
>
RE: svn commit: r1740170 -/subversion/trunk/subversion/libsvn_client/conflicts.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Stefan Sperling [mailto:stsp@elego.de]
> Sent: donderdag 21 april 2016 09:43
> To: dev@subversion.apache.org
> Cc: commits@subversion.apache.org
> Subject: Re: svn commit: r1740170 -
> /subversion/trunk/subversion/libsvn_client/conflicts.c
>
> On Thu, Apr 21, 2016 at 09:40:07AM +0200, Branko Čibej wrote:
> > It is wrong to use conflict->local_abspath to construct the relative
> > URL.
>
> Yes, it is.
>
> But where is the alleged code that does this? I don't see it.
> And I don't think I've written it.
I think I missed that the context isn't complete and the format string specifies a local path first, and a repos_relpath later.
Bert
RE: svn commit: r1740170 -/subversion/trunk/subversion/libsvn_client/conflicts.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Stefan Sperling [mailto:stsp@elego.de]
> Sent: donderdag 21 april 2016 09:43
> To: dev@subversion.apache.org
> Cc: commits@subversion.apache.org
> Subject: Re: svn commit: r1740170 -
> /subversion/trunk/subversion/libsvn_client/conflicts.c
>
> On Thu, Apr 21, 2016 at 09:40:07AM +0200, Branko Čibej wrote:
> > It is wrong to use conflict->local_abspath to construct the relative
> > URL.
>
> Yes, it is.
>
> But where is the alleged code that does this? I don't see it.
> And I don't think I've written it.
I think I missed that the context isn't complete and the format string specifies a local path first, and a repos_relpath later.
Bert
Re: svn commit: r1740170
-/subversion/trunk/subversion/libsvn_client/conflicts.c
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Apr 21, 2016 at 09:40:07AM +0200, Branko Čibej wrote:
> It is wrong to use conflict->local_abspath to construct the relative
> URL.
Yes, it is.
But where is the alleged code that does this? I don't see it.
And I don't think I've written it.
Re: svn commit: r1740170
-/subversion/trunk/subversion/libsvn_client/conflicts.c
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Apr 21, 2016 at 09:40:07AM +0200, Branko Čibej wrote:
> It is wrong to use conflict->local_abspath to construct the relative
> URL.
Yes, it is.
But where is the alleged code that does this? I don't see it.
And I don't think I've written it.
Re: svn commit: r1740170
-/subversion/trunk/subversion/libsvn_client/conflicts.c
Posted by Branko Čibej <br...@apache.org>.
On 21.04.2016 09:24, Stefan Sperling wrote:
> On Wed, Apr 20, 2016 at 10:19:56PM +0200, Bert Huijben wrote:
>> The path calculation here 100% assumes that the working copy is always a clean check out from ^/. This might be the case in our test suite, but in almost every normal user scenario this isn’t the case.
>>
>> You can’t just calculate a repository root relative path ( ^/… ) from dirents. You always need the repos_relpath from somewhere else.
>>
>> And once you start looking at switched files/directories, things get even harder.
>>
>>
>> Bert
>>
> I'm not sure what you mean. Perhaps you're lacking some additional
> context for this diff?
>
> What's happening here is that we show a message like:
>
> delete X and copy ^/Y@Z here
>
> where X is a local conflicted path (taken from the local_abspath which is
> being resolved), Y is a repos-relpath stored in the conflict descriptor,
> and Z is a peg revision stored in the conflict descriptor.
>
> Assuming Y and Z were stored correctly when the conflict was created (the
> conflict resolver has no reason to assume otherwise), I don't see where
> this extrapolates a local path to a repostory path.
>
> Can you elaborate? Are you saying that's wrong to say "delete X" because
> X might not come from path ^/somewhere/X in the repository?
It is wrong to use conflict->local_abspath to construct the relative
URL. In general the local path bears no relation to the location in the
repository.
-- Brane
>> From: stsp@apache.org
>> Sent: woensdag 20 april 2016 19:05
>> To: commits@subversion.apache.org
>> Subject: svn commit: r1740170 -/subversion/trunk/subversion/libsvn_client/conflicts.c
>>
>> Author: stsp
>> Date: Wed Apr 20 17:05:04 2016
>> New Revision: 1740170
>>
>> URL: http://svn.apache.org/viewvc?rev=1740170&view=rev
>> Log:
>> * subversion/libsvn_client/conflicts.c
>> (configure_option_merge_incoming_added_file_replace): Tweak this resolution
>> option's description slightly.
>>
>> Modified:
>> subversion/trunk/subversion/libsvn_client/conflicts.c
>>
>> Modified: subversion/trunk/subversion/libsvn_client/conflicts.c
>> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/conflicts.c?rev=1740170&r1=1740169&r2=1740170&view=diff
>> ==============================================================================
>> --- subversion/trunk/subversion/libsvn_client/conflicts.c (original)
>> +++ subversion/trunk/subversion/libsvn_client/conflicts.c Wed Apr 20 17:05:04 2016
>> @@ -4527,7 +4527,7 @@ configure_option_merge_incoming_added_fi
>> conflict->local_abspath, scratch_pool,
>> scratch_pool));
>> option->description =
>> - apr_psprintf(options->pool, _("delete '%s', copy '^/%s@%ld' here"),
>> + apr_psprintf(options->pool, _("delete '%s' and copy '^/%s@%ld' here"),
>> svn_dirent_local_style(
>> svn_dirent_skip_ancestor(wcroot_abspath,
>> conflict->local_abspath),
>>
>>
>>