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

Hi Kamesh,

Attached are my tweaks to Dan's version of your patch which fixes the
problem with merge_test-48 and also allows compilation on Win32 (as
previously mentioned in
http://svn.haxx.se/dev/archive-2007-04/0676.shtml).

The fix for merge test 48 is here, in merge.c:do_merge():

   const svn_wc_entry_t *entry;
   int i;
   svn_boolean_t inherited;
+ svn_opt_revision_t assumed_initial_revision1,
assumed_initial_revision2;
   apr_size_t target_count, merge_target_count;
 
- ENSURE_VALID_REVISION_KINDS(initial_revision1->kind,
- initial_revision2->kind);
+ /* Establish first RA session to initial_URL1. */
+ SVN_ERR(svn_client__open_ra_session_internal(&ra_session,
initial_URL1, NULL,
+ NULL, NULL, FALSE, TRUE,
+ ctx, pool));
+ SVN_ERR(assume_default_rev_range(initial_revision1,
+ &assumed_initial_revision1,
+ initial_revision2,
+ &assumed_initial_revision2,
+ ra_session,
+ pool));
 
+ ENSURE_VALID_REVISION_KINDS(assumed_initial_revision1.kind,
+ assumed_initial_revision2.kind);
+
+
   /* If we are performing a pegged merge, we need to find out what our
      actual URLs will be. */
   if (peg_revision->kind != svn_opt_revision_unspecified)
@@ -1673,10 +1740,13 @@
                                           initial_path2 ? initial_path2
                                           : initial_URL2,
                                           peg_revision,
- initial_revision1,
- initial_revision2,
+ &assumed_initial_revision1,
+ &assumed_initial_revision2,
                                           ctx, pool));

+ /* Reparent session if actual URL changed. */
+ if (strcmp(initial_URL1, URL1))
+ SVN_ERR(svn_ra_reparent(ra_session, URL1, pool));
+
       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Merge test 48 failed, not because of anything to do with switched paths,
but because the initial session opened in do_merge() was for the wrong
URL for the revision being merged in. Reparenting the session if the
actual URL was different solved the problem.

Anyway, per discussion with Dan in IRC, please go ahead and commit this
version of the patch.

Thanks,

Paul B.

> -----Original Message-----
> From: Daniel Rall [mailto:dlr@collab.net]
> Sent: Tuesday, April 24, 2007 7:24 PM
> To: Kamesh Jayachandran
> Cc: dev@subversion.tigris.org
> Subject: Re: [PATCH] Implicit detection of merge source URL
> and merge revision range
>
> On Tue, 24 Apr 2007, Kamesh Jayachandran wrote:
>
> > Hi Dan,
> > Find the attached patch and log incorporating your review.
> > Somehow merge_tests 48 (new testcase added by Paul
> yesterday, earlier
> > it was in XFail) is failing with this patch.
>
> Thanks Kamesh! I really appreciate you cranking another
> revision of this patch out, and responding so thoroughly to
> the review.
>
> This is a really important change that I'd like to start
> exercising ASAP.
>
> > >>--- 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.
>
> > >>+ " If SOURCE is not given SOURCE URL is deduced
> from the \n"
> > >>+ " copy source URL of WCPATH. If the WCPATH is
> not a copy of
> > >>some \n"
> > >>+ " other node error is thrown asking for URL.
> This URL in\n"
> > >>
> > >
> > >We should talk about SOURCE here, rather than WCPATH.
> >
> > I believe you meant we should use 'SOURCE' rather than 'URL'.
>
> Nope -- I meant SOURCE.
>
> > >
> > >> " revision REV is compared as it existed
> between revisions N
> > >> and \n"
> > >> " M. If REV is not specified, HEAD is assumed.\n"
> > >> " The '-c M' option is equivalent to '-r N:M'
> where N = M-1.\n"
> > >> " Using '-c -M' does the reverse: '-r M:N'
> where N = M-1.\n"
> > >>+ " If revision range is not specified, it is
> assumed to be \n"
> > >>+ " -r oldest_source_node_rev:HEAD.\n"
> > >>
> > >
> > >I don't have a great alternate suggestion, but think that
> > >"oldest_source_node_rev" should be formatted differently.
> > >
> > Changed it to OLDEST_SOURCE_NODE_REV.
>
> The word "node" shouldn't appear in our end user docs for the client.
>
> > >>--- subversion/svn/merge-cmd.c (revision 24527)
> > >>+++ subversion/svn/merge-cmd.c (working copy)
> ...
> > >> svn_boolean_t using_rev_range_syntax = FALSE;
> > >> svn_error_t *err;
> > >>- svn_opt_revision_t peg_revision;
> > >>+ svn_opt_revision_t peg_revision, peg_revision1, peg_revision2;
> > >
> > >Do we need three peg revision variables? In the
> > >using_rev_range_syntax block, it looks like the two new
> ones are only
> > >temporary.
> >
> > Yes we need this new variables, they are there to avoid multiple
> > 'svn_opt_parse_path' calls on different invocation scenarios.
>
> But all we really do with peg_revision is set peg_revision =
> peg_revision1
> -- we should be able to dump one of them.
>
> ...
> > >>+ peg_revision = peg_revision1;
> > >
> > >I think we can do away with this statement by removing the two new
> > >peg rev variables.
> ...
> > No. We need them as explained above. Thanks for catching I
> need such
> > assignments inside !using_rev_range block.
>
> Yup, I noticed that we need one more peg rev (as shown in patch v7).
> However, we don't need two more, so we can do away with this
> statement.
>
> ...
> > >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.
>
> ...
> > >>+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.
>
> > >>+/* A log callback conforming to the svn_log_message_receiver_t
> > >>+ interface for obtaining the copy source of a node at
> a path and
> > >>+ storing it in *BATON (a struct copy_source_baton *).
> */ static
> > >>+svn_error_t * copy_source_receiver(void *baton,
> > >>+ apr_hash_t *changed_paths,
> > >>+ svn_revnum_t revision,
> > >>+ const char *author,
> > >>+ const char *date,
> > >>+ const char *message,
> > >>+ apr_pool_t *pool) {
> > >>+ apr_hash_index_t *hi;
> > >>+ void *val;
> > >>+ const char *copy_source_path;
> > >>+ svn_log_changed_path_t *changed_path;
> > >>+ struct copy_source_baton *copy_source_baton;
> > >>+ copy_source_baton = baton;
> > >>+ /*FIXME: if the rev at which this node is created has
> few other node
> > >>+ changes too extract only our node. */
> > >>+ for (hi = apr_hash_first(pool, changed_paths); hi; hi =
> > >>apr_hash_next(hi))
> > >>+ {
> > >>+ apr_hash_this(hi, NULL, NULL, &val);
> > >>+ changed_path = val;
> > >>+ }
> > >>+ copy_source_path = changed_path->copyfrom_path;
> > >>+
> > >>+ *((char **) copy_source_baton->copy_source) =
> > >>+ apr_pstrdup(copy_source_baton->pool,
> > >>copy_source_path);
> > >>+ return SVN_NO_ERROR;
> > >>+}
> > >>
> > >
> > >You probably want to look at my receiver implementation.
> >
> > Will look at it.
> >
> > >>--- subversion/libsvn_client/merge.c (revision 24527)
> > >>+++ subversion/libsvn_client/merge.c (working copy)
> ...
> > >>+static svn_error_t *
> > >>+assume_default_rev_range(const svn_opt_revision_t *revision1,
> > >>+ svn_opt_revision_t *assumed_revision1,
> > >>+ const svn_opt_revision_t *revision2,
> > >>+ svn_opt_revision_t *assumed_revision2,
> > >>+ svn_ra_session_t *ra_session,
> > >>+ apr_pool_t *pool) {
> ...
> > >>+ if (revision2->kind == svn_opt_revision_unspecified)
> > >>+ {
> > >>+ if (head_revnum == SVN_INVALID_REVNUM)
> > >>
> > >
> > >...and here (reversing the blocks).
> >
> > I could not get you here.
>
> if (SVN_IS_VALID_REVNUM(head_revnum))
> {
> assumed_revision2->value.number = head_revnum;
> assumed_revision2->kind = svn_opt_revision_number;
> }
> else
> {
> assumed_revision2->kind = svn_opt_revision_head;
> }
>
> > >>+ assumed_revision2->kind = svn_opt_revision_head;
> > >>+ else
> > >>+ {
> > >>+ assumed_revision2->value.number = head_revnum;
> > >>+ assumed_revision2->kind = svn_opt_revision_number;
> > >>+ }
> ...
> > >>+ /* Establish first RA session to URL1. */
> > >>+ /*FIXME: use initial_URL1 for now which may barf for
> pegged rev
> > >>+ merges
> > >>*/
> > >>
> > >
> > >What are these FIXME's all about? Can you give more context?
> > >
> > I was not sure how things would behave if URL given to
> > 'svn_client__open_ra_session_internal' if of form
> > scheme://hostname/path/to/repo/subdir_at_173. So I put a FIXME marker.
>
> Okay. I think it's assumed that the peg revision has already
> been parsed off of the URL, no? It is already a separate
> parameter, at least.
>
> ...
> > >>--- subversion/tests/cmdline/merge_tests.py (revision 24527)
> > >>+++ subversion/tests/cmdline/merge_tests.py (working copy)
> > >>@@ -1243,7 +1243,7 @@
> > >> def merge_with_implicit_target_helper(sbox, arg_flav):
> > >> "ARG_FLAV is one of 'r' (revision range) or 'c'
> (single change)."
> > >>
> > >>- if arg_flav not in ('r', 'c'):
> > >>+ if arg_flav not in ('r', 'c', '*'):
> > >> raise svntest.Failure("Unrecognized flavor of merge
> argument")
> > >>
> > >> sbox.build()
> > >>@@ -1294,9 +1294,15 @@
> > >> svntest.actions.run_and_verify_svn(None, ['U mu\n'], [],
> > >> 'merge', '-c', '-2',
> > >>mu_url)
> > >>
> > >>+ #implicit rev and url detection is for forward merge only.
> > >>+ elif arg_flav == '*':
> > >>+ svntest.actions.run_and_verify_svn(None, ['U mu\n'], [],
> > >>+ 'merge', '-c', '-2',
> > >>+ mu_url)
> > >>
> > >
> > >We should document this in the command-line client.
> ...
> > Fixed the command line help text.
>
> I see "which is a forward merge", which is implied by the
> preceding text. Maybe we should just leave that off, hmm...
>
> > Here we should undo the forward merge of r2 to continue
> with rest of
> > the testcase, instead of reverting, I would prefer merge -c
> -2 for now.
> > Later would see if possible to make it something like
> this(implicitly
> > detecting the source atleast).
>
> Okay.
>
> ...
> > >>@@ -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.)
> >
> > Will write such a test later.
>
> Sounds good.
>
> I removed SVN_ERR_CLIENT_PATH_HAS_NO_COPY_SOURCE and its
> usage, which is really more about auto-detecting a merge
> source. An error like PATH_HAS_NO_COPY_SOURCE is too
> algorithm-specific; there's more information than just
> copy-source that we can use here (e.g. most recent merge source).
>
>
> I'm attaching a revised version of the patch (v9) which
> accounts for my tweaks from v7, plus most of the discussion above.
>
> > [[[
> > 'merge' subcommand should automatically identify the source
> from which
> > to merge and the applicable revisions to merge for
> 'revision based merges'.
> >
> > 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.
> >
> > * subversion/svn/main.c
> > (svn_cl__cmd_table):
> > Update help text for merge subcommand.
> > * subversion/svn/merge-cmd.c
> > (svn_cl__merge):
> > Implementing the above change.
> > * subversion/libsvn_client/client.h
> > (svn_client__oldest_rev_at_path): Prototype the new function.
> > (svn_client__get_copy_source): Prototype the new function.
> > * subversion/libsvn_client/copy.c
> > (revnum_receiver): Moved to subversion/libsvn_client/log.c.
> > (get_implied_merge_info): Implementation to find the rev
> at which a
> > given path is created has been moved to
> 'svn_client__oldest_rev_at_path'.
> > This function calls 'svn_client__oldest_rev_at_path'.
> > * subversion/libsvn_client/log.c
> > (revnum_receiver): Moved from subversion/libsvn_client/copy.c.
> > (svn_client__oldest_rev_at_path): New function.
> > (struct copy_source_baton): New baton to hold the
> copy_source_path.
> > (copy_source_receiver): New function.
> > (svn_client__get_copy_source): New function.
> > * subversion/libsvn_client/merge.c
> > (assume_default_rev_range): New function.
> > (do_merge, do_single_file_merge): Deduce the revison
> range to merge in case
> > not provided.
> > (svn_client_merge_peg3): Deduce source if not given. Fix
> the error message
> > for 'undeterminable source URL'
> > * subversion/include/svn_error_codes.h
> > (SVN_ERR_CLIENT_PATH_HAS_NO_COPY_SOURCE): define new
> error code if merge
> > source could not be deduced.
> >
> > * subversion/tests/cmdline/merge_tests.py
> > (merge_with_implicit_target_helper):
> > Support implicit merge URL and revisions feature like
> other 2 variants.
> > (merge_with_implicit_target_and_revs): New function.
> > (test_list): include
> 'merge_with_implicit_target_and_revs' test case.
> >
> > Patch by: kameshj
> > dlr
> > ]]]
>

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org