Re: svn commit: r1808258 - /subversion/trunk/subversion/libsvn_client/conflicts.c

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1808258 - /subversion/trunk/subversion/libsvn_client/conflicts.c

Branko Čibej
On 13.09.2017 19:16, [hidden email] wrote:

> Author: stsp
> Date: Wed Sep 13 17:16:43 2017
> New Revision: 1808258
>
> URL: http://svn.apache.org/viewvc?rev=1808258&view=rev
> Log:
> Follow-up to r1808177:
>
> The change in r1808177 had a bug where moves were actually only searched
> within one revision back in history. Detect moves even if are revisions
> between the "branch-point" and the revision being cherry-picked.
>
> * subversion/libsvn_client/conflicts.c
>   (find_nearest_yca): New helper function.
>   (conflict_tree_get_details_local_missing): Use find_nearest_yca() to
>    find a lower bound for the revision range which must be searched for
>    moves.
>
> 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=1808258&r1=1808257&r2=1808258&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_client/conflicts.c (original)
> +++ subversion/trunk/subversion/libsvn_client/conflicts.c Wed Sep 13 17:16:43 2017
> @@ -482,6 +482,80 @@ find_yca(svn_client__pathrev_t **yca_loc
>    return SVN_NO_ERROR;
>  }
>  
> +/* Like find_yca, expect that a YCA could also be found via a brute-force
> + * search of parents of REPOS_RELPATH1 and REPOS_RELPATH2, if no "direct"
> + * YCA exists. An implicit assumption is that some parent of REPOS_RELPATH1
> + * is a branch of some parent of REPOS_RELPATH2.
> + *
> + * This function can guess a "good enough" YCA for 'missing nodes' which do
> + * not exist in the working copy, e.g. when a file edit is merged to a path
> + * which does not exist in the working copy.
> + */
> +static svn_error_t *
> +find_nearest_yca(svn_client__pathrev_t **yca_locp,
> +                 const char *repos_relpath1,
> +                 svn_revnum_t peg_rev1,
> +                 const char *repos_relpath2,
> +                 svn_revnum_t peg_rev2,
> +                 const char *repos_root_url,
> +                 const char *repos_uuid,
> +                 svn_ra_session_t *ra_session,
> +                 svn_client_ctx_t *ctx,
> +                 apr_pool_t *result_pool,
> +                 apr_pool_t *scratch_pool)
> +{
> +  svn_client__pathrev_t *yca_loc;
> +  svn_error_t *err;
> +  apr_pool_t *iterpool;
> +  const char *p1, *p2;
> +  int c1, c2;
> +
> +  *yca_locp = NULL;
> +
> +  iterpool = svn_pool_create(scratch_pool);
> +
> +  p1 = repos_relpath1;
> +  c1 = svn_path_component_count(repos_relpath1);

...subversion/libsvn_client/conflicts.c:524:8: warning: implicit conversion loses integer precision:
      'apr_size_t' (aka 'unsigned long') to 'int' [-Wshorten-64-to-32]
  c1 = svn_path_component_count(repos_relpath1);
     ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


> +  while (c1--)
> +    {
> +      svn_pool_clear(iterpool);
> +
> +      p2 = repos_relpath2;
> +      c2 = svn_path_component_count(repos_relpath2);

.../subversion/libsvn_client/conflicts.c:530:12: warning: implicit conversion loses integer precision:
      'apr_size_t' (aka 'unsigned long') to 'int' [-Wshorten-64-to-32]
      c2 = svn_path_component_count(repos_relpath2);
         ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


> +      while (c2--)
> +        {
> +          err = find_yca(&yca_loc, p1, peg_rev1, p2, peg_rev2,
> +                         repos_root_url, repos_uuid, ra_session, ctx,
> +                         result_pool, iterpool);
> +          if (err)
> +            {
> +              if (err->apr_err == SVN_ERR_FS_NOT_FOUND)
> +                {
> +                  svn_error_clear(err);
> +                  yca_loc = NULL;
> +                }
> +              else
> +                return svn_error_trace(err);
> +            }
> +
> +          if (yca_loc)
> +            {
> +              *yca_locp = yca_loc;
> +              svn_pool_destroy(iterpool);
> +              return SVN_NO_ERROR;
> +            }
> +
> +          p2 = svn_relpath_dirname(p2, scratch_pool);
> +        }
> +
> +      p1 = svn_relpath_dirname(p1, scratch_pool);
> +    }
> +
> +  svn_pool_destroy(iterpool);
> +
> +  return SVN_NO_ERROR;
> +}


-- Brane
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1808258 - /subversion/trunk/subversion/libsvn_client/conflicts.c

Branko Čibej
On 19.09.2017 17:56, Branko Čibej wrote:

> On 13.09.2017 19:16, [hidden email] wrote:
>> Author: stsp
>> Date: Wed Sep 13 17:16:43 2017
>> New Revision: 1808258
>>
>> URL: http://svn.apache.org/viewvc?rev=1808258&view=rev
>> Log:
>> Follow-up to r1808177:
>>
>> The change in r1808177 had a bug where moves were actually only searched
>> within one revision back in history. Detect moves even if are revisions
>> between the "branch-point" and the revision being cherry-picked.
>>
>> * subversion/libsvn_client/conflicts.c
>>   (find_nearest_yca): New helper function.
>>   (conflict_tree_get_details_local_missing): Use find_nearest_yca() to
>>    find a lower bound for the revision range which must be searched for
>>    moves.
>>
>> 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=1808258&r1=1808257&r2=1808258&view=diff
>> ==============================================================================
>> --- subversion/trunk/subversion/libsvn_client/conflicts.c (original)
>> +++ subversion/trunk/subversion/libsvn_client/conflicts.c Wed Sep 13 17:16:43 2017
>> @@ -482,6 +482,80 @@ find_yca(svn_client__pathrev_t **yca_loc
>>    return SVN_NO_ERROR;
>>  }
>>  
>> +/* Like find_yca, expect that a YCA could also be found via a brute-force
>> + * search of parents of REPOS_RELPATH1 and REPOS_RELPATH2, if no "direct"
>> + * YCA exists. An implicit assumption is that some parent of REPOS_RELPATH1
>> + * is a branch of some parent of REPOS_RELPATH2.
>> + *
>> + * This function can guess a "good enough" YCA for 'missing nodes' which do
>> + * not exist in the working copy, e.g. when a file edit is merged to a path
>> + * which does not exist in the working copy.
>> + */
>> +static svn_error_t *
>> +find_nearest_yca(svn_client__pathrev_t **yca_locp,
>> +                 const char *repos_relpath1,
>> +                 svn_revnum_t peg_rev1,
>> +                 const char *repos_relpath2,
>> +                 svn_revnum_t peg_rev2,
>> +                 const char *repos_root_url,
>> +                 const char *repos_uuid,
>> +                 svn_ra_session_t *ra_session,
>> +                 svn_client_ctx_t *ctx,
>> +                 apr_pool_t *result_pool,
>> +                 apr_pool_t *scratch_pool)
>> +{
>> +  svn_client__pathrev_t *yca_loc;
>> +  svn_error_t *err;
>> +  apr_pool_t *iterpool;
>> +  const char *p1, *p2;
>> +  int c1, c2;
>> +
>> +  *yca_locp = NULL;
>> +
>> +  iterpool = svn_pool_create(scratch_pool);
>> +
>> +  p1 = repos_relpath1;
>> +  c1 = svn_path_component_count(repos_relpath1);
> ...subversion/libsvn_client/conflicts.c:524:8: warning: implicit conversion loses integer precision:
>       'apr_size_t' (aka 'unsigned long') to 'int' [-Wshorten-64-to-32]
>   c1 = svn_path_component_count(repos_relpath1);
>      ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
>
>> +  while (c1--)
>> +    {
>> +      svn_pool_clear(iterpool);
>> +
>> +      p2 = repos_relpath2;
>> +      c2 = svn_path_component_count(repos_relpath2);
> .../subversion/libsvn_client/conflicts.c:530:12: warning: implicit conversion loses integer precision:
>       'apr_size_t' (aka 'unsigned long') to 'int' [-Wshorten-64-to-32]
>       c2 = svn_path_component_count(repos_relpath2);
>          ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~



Fixed in r1808258. But ... what is a "nearest youngest common ancestor?"

-- Brane
Reply | Threaded
Open this post in threaded view
|

"nearest youngest common ancestor" (was: Re: svn commit: r1808258 - /subversion/trunk/subversion/libsvn_client/conflicts.c)

Stefan Sperling
On Tue, Sep 19, 2017 at 05:59:52PM +0200, Branko Čibej wrote:
> Fixed in r1808258.

Thanks!

> But ... what is a "nearest youngest common ancestor?"

Oh dear... well, it's the YCA of a path component x of path A and
a path component y of path B.

I found no better way to determine a revision to use as a lower bound
during a log search for moves, when given two unrelated paths.

Consider:

  r1: import ^/trunk/foo/bar/alpha
  r2: cp ^/trunk ^/branch
  r3: mv ^/trunk/foo/bar ^/trunk/foo/moo
  r4: mv ^/trunk/foo/moo/alpha ^/trunk/foo/moo/alpha-renamed
  r5: edit ^/trunk/foo/moo/alpha-renamed

  co -r5 ^/branch branch-wc
  merge -c5 ^/trunk branch-wc

You get a 'local missing' tree conflict with:

  ^/trunk/foo/moo/alpha-renamed@4 merge-left
  ^/trunk/foo/moo/alpha-renamed@5 merge-right
  local conflict victim path: branch-wc/foo/moo/alpha-renamed
 
Our preferred solution is to ignore the move and apply the text edit only.
So we want the resolver to correlate the victim to ^/branch/foo/bar/alpha
and apply the edit there.

Take the parent dir of the conflict victim: ^/branch/foo/moo@5
(The implementation should do an existence check here, but currently does not.)
Is there a YCA with ^/trunk/foo/moo/alpha-renamed@5 ? -> No
Is there a YCA with ^/trunk/foo/moo@5 -> No
Is there a YCA with ^/trunk/foo@5 -> No (^/branch/foo/moo@5 does not exist)
Is there a YCA with ^/trunk@5 -> No

Take the parent dir again: ^/branch/foo@5
Is there a YCA with ^/trunk/foo/moo/alpha-renamed@5 ? -> No
Is there a YCA with ^/trunk/foo/moo@5 -> No
Is there a YCA with ^/trunk/foo@5 -> Yes: ^/trunk/foo@r1

The lower bound is r2.
Scan the log for moves between ^/trunk/foo@2 and ^/trunk/foo@5
  r3: mv ^/trunk/foo/bar ^/trunk/foo/moo
  r4: mv ^/trunk/foo/moo/alpha ^/trunk/foo/moo/alpha-renamed

In the conflict description, we can now enlighten the user by showing
relevant moves on ^/trunk's line of history.

Of course, this isn't foolproof. For instance, if someone had copied an
unrelated line of history to ^/trunk/foo, we'd never find a YCA. I expect
that many cases exist where it breaks. But it works in the "common" case
of basic branching/merging of one entire project. And if it doesn't work
we're leaving the user no worse off than before (apart from some time
wasted waiting for the computer).

This is as far as the current implementation has gotten.
The next, still missing, step is to locate the path branch-wc/foo/moo/alpha.
I already have an idea for that, and it's equally hackish.

You may shake your head in disbelief now :)