Re: [PATCH] Implicit detection of merge source URL and merge revision range

On Tue, 24 Apr 2007, Daniel Rall wrote:

> On Tue, 24 Apr 2007, Kamesh Jayachandran wrote:
...
> > >>--- subversion/svn/main.c (revision 24527)
> > >>+++ subversion/svn/main.c (working copy)
> > >>@@ -513,7 +513,7 @@
> > >> ("Apply the differences between two sources to a working copy
> > >> path.\n"
> > >> "usage: 1. merge sourceURL1[@N] sourceURL2[@M] [WCPATH]\n"
> > >> " 2. merge sourceWCPATH1@N sourceWCPATH2@M [WCPATH]\n"
> > >>- " 3. merge [-c M | -r N:M] SOURCE[@REV] [WCPATH]\n"
> > >>+ " 3. merge [-c M | -r N:M] [SOURCE[@REV]] [WCPATH]\n"
> > >>
> > >
> > >I'd expect to this to be:
> > >
> > > merge [-c M | -r N:M] [SOURCE[@REV] [WCPATH]]
> >
> > No. It fails to cover the following case.
> >
> > merge [-c M | -r N:M] MY_WCPATH
> >
> > According to above syntax MY_WCPATH would be equated with SOURCE which
> > is not correct.
>
> With this syntax, you can't differentiate between SOURCE and WCPATH,
> since SOURCE can be a WC path (which corresponds to the merge source
> URL). When SOURCE is itself a WC path, we've always assumed that the
> target WC path for the merge is ".", which effectively means that the
> WCPATH parameter can't be specified unless a SOURCE parameter is
> provided.
>
> Can you describe why it makes sense to change this now? Thanks!
>
> > >>- " 3. In the third form, SOURCE can be a URL, or working copy
> > >>item\n"
> > >>- " in which case the corresponding URL is used. This URL in\n"
> > >>+ " 3. In the third form, SOURCE should be a URL if given. \n"
> > >>
> > >
> > >Since SOURCE can be a WCPATH, the above wording needs to be changed.
> >
> > I am referring SOURCE in the above context not in a generic sense. All I
> > mean is SOURCE(If given) should be a absolute URI, If SOURCE is omitted,
> > we will derive the SOURCE URL from from WCPATH target.
> >
> > <snip from my original log>
> > This feature causes conflicts with one use case of existing feature.
> > Currently revision based ranges can be of the form
> > "3. merge [-c M | -r N:M] SOURCE[@REV] [WCPATH]\n"
> > Making SOURCE and REV_RANGE optional in the above command results in the
> > following type of invocation.
> > "merge WC_SOURCE[@REV] [WCPATH]\n", this causes the ambiguity with
> > "2. merge sourceWCPATH1@N sourceWCPATH2@M [WCPATH]\n".
> >
> > So to fix this we mandate SOURCE in 3rd style of merges to be a URL if
> > given.
> > </snip>
> > I think I should use 'sourceURL' instead of 'SOURCE' to make things less
> > ambiguous.
>
> This alters both the UI and the semantics of the command-line in an
> incompatible fashion. You're suggesting that we make the assumption
> that WCPATH is the merge source, in addition to its existing semantics
> of being the WC target path.

We need to:

a) Finish discussing this soon.

b) Write some tests for this, either way.

I'll start on (b).

...
> > >There's a mailing list thread about these APIs that I need to follow
> > >up on. We'll probably also want to bring existing merge sources into
> > >consideration where we use the svn_client__get_copy_source() API
> > >below.
>
> We still need to do this.

I've mostly taken care of this. I think that We still need to account
for moved parents of a merge target in
libsvn_client/log.c:copyfrom_info_receiver():

      for (hi = apr_hash_first(NULL, changed_paths);
           hi;
           hi = apr_hash_next(hi))
        {
          void *val;
          apr_hash_this(hi, (void *) &path, NULL, &val);
          changed_path = val;

          /* Consider only the path we're interested in. */
          /* ### FIXME: Look for moved parents of target_path. */
          if (changed_path->copyfrom_path &&
              SVN_IS_VALID_REVNUM(changed_path->copyfrom_rev) &&
              strcmp(path, copyfrom_info->target_path) == 0)
            {
              copyfrom_info->path = apr_pstrdup(copyfrom_info->pool,
                                                changed_path->copyfrom_path);
              copyfrom_info->rev = changed_path->copyfrom_rev;
              break;
            }
        }

> ...
> > >>+struct copy_source_baton
> > >>+{
> > >>+ const char **copy_source;
> > >>+ apr_pool_t *pool;
> > >>+};
> > >>
> > >
> > >Doesn't this need a copyfrom_rev field, too? Did you happen to see
> > >the patch I sent to the dev list on "Fri, 30 Mar 2007", attached to
> > >the message with "Subject: [RFC] Identifying copy/move sources", which
> > >supplies a very similar API?
> >
> > Sorry, I did not look at it.
> > Here I don't need copy_from_rev.
>
> Yeah, but I think the API should provide it nonetheless.

I took care of this, too.

...
> > >>--- subversion/tests/cmdline/merge_tests.py (revision 24527)
> > >>+++ subversion/tests/cmdline/merge_tests.py (working copy)
...
> > >>@@ -5370,6 +5384,7 @@
> > >> simple_property_merges,
> > >> merge_with_implicit_target_using_r,
> > >> merge_with_implicit_target_using_c,
> > >>+ merge_with_implicit_target_and_revs,
> > >>
> > >
> > >I seem to recall a second existing test case which could house this
> > >type of regression testing...merge_one_file_helper() it is. An
> > >additional consumer -- a la merge_one_file_using_r() and
> > >merge_one_file_using_c() -- could be added. (I didn't do this.)
...

I added a new test which is a merge_one_file_helper() consumer.