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
> ]]]
- application/pgp-signature attachment: stored
Received on Wed Apr 25 01:24:20 2007