On Tue, 13 Feb 2007, Kamesh Jayachandran wrote:
> Daniel Rall wrote:
> >On Wed, 31 Jan 2007, Kamesh Jayachandran wrote:
...
> >>+ svntest.actions.run_and_verify_svn(None,
> >>+ ['U ' + short_fs_dst_path +
> >>'\n'],
> >>+ [],
> >>+ 'merge', '-c', str(new_rev),
> >>+ sbox.repo_url + '/' +
> >>canon_src_path,
> >>+ short_fs_dst_path)
> >>
> >
> >Why don't we use run_and_verify_merge() here instead? Seems like this
> >should verify the "svn:mergeinfo" property too.
>
> actions.run_and_verify_merge assumes the target to be dir always. Here I do
> single file merge.
>
> Yes I should verify both disk_contents of the file and prop changes.
> Unfortunately the tree API 'build_tree_from_wc' works only for directories
> not for files.
>
> Will post a separate patch to trunk to make build_tree_from_wc work for
> 'file' paths.
Great! Once build_tree_from_wc() can handle files, will that be
sufficient to allow run_and_verify_merge() to handle them also?
...
> With the style you suggested above simplified the code a lot.
Cool! With that much condensing of the code, I'd say the added
conceptual complexity is a fair trade-off.
...
> [[[
> Reporter api calls should be made in a depth first order.
>
> * subversion/libsvn_client/diff.c
> (global): Include "svn_sorts.h".
Drop "(global): ". Subversion's convention is no prefix, or "(): " (I
personally hate the latter).
> (discover_and_merge_children): Change the signature to accept
> 'children_with_mergeinfo' of type 'apr_array_header_t **'
> as an out param instead of 'apr_hash_t *'.
> Sort 'children_with_mergeinfo_hash' a local hash by path and pass the
> sorted list to subsequent calls to do_merge.
> (do_merge): Change the signature to accept the '*depth first ordered list*
^
Odd formatting in here.
> of children with mergeinfo' instead of *hash*.
> (svn_client_merge3, svn_client_merge_peg3): Adjust for the above 2 API
> changes.
>
> * subversion/tests/cmdline/merge_tests.py
> (tweak_src_then_merge_to_dest,
> obey_reporter_api_semantics_while_doing_subtree_merges):
> New functions.
> (test_list): Include the new testcase.
>
> Patch by: kameshj
> Found by: plundblad
> Reviewed by: plundblad
> dlr
> ]]]
Two comments inline below.
...
> --- subversion/tests/cmdline/merge_tests.py (revision 23381)
> +++ subversion/tests/cmdline/merge_tests.py (working copy)
...
> +def tweak_src_then_merge_to_dest(sbox, src_path, dst_path,
> + canon_src_path, contents, cur_rev):
> + """Edit src and commit it. This results in new_rev.
> + Merge new_rev to dst_path. Return new_rev."""
...
> + return new_rev
> +
> +def obey_reporter_api_semantics_while_doing_subtree_merges(sbox):
> + "drive reporter api in depth first order"
> +
> + # Copy /A/D to /A/copy-of-D it results in rONE.
> + # Create children at different hierarchies having some merge-info
> + # to test the set_path calls on a reporter in a depth-first order.
> + # Children considered
> + # /A/copy-of-D/
> + # G/pi
> + # G/rho
> + # G/tau
> + # G
> + # gamma
> + # H/chi
> + # H/omega
> + # H/psi
> + # H all are made to have the mergeinfo.
> + # We run merges on individual files listed above.
> + # We create /A/D/umlaut directly over URL it results in rev rTWO.
> + # When we merge rONE+1:TWO of /A/D on /A/copy-of-D it should merge smoothly.
I was hoping that the amount of vertical screen real estate could be
compressed by putting all those paths on the same line here (I'm not
convinced we need all those paths listed again at all).
...
> + new_rev = 2
Isn't this more like "cur_rev" in this context (or maybe just "rev"
for brevity)?
> + for path in (["A", "D", "G", "pi"],
> + ["A", "D", "G", "rho"],
> + ["A", "D", "G", "tau"],
> + ["A", "D", "H", "chi"],
> + ["A", "D", "H", "omega"],
> + ["A", "D", "H", "psi"],
> + ["A", "D", "gamma"]):
> + path_name = os.path.join(wc_dir, *path)
> + canon_path_name = os.path.join(*path)
> + path[1] = "copy-of-D"
> + copy_of_path_name = os.path.join(wc_dir, *path)
> + var_name = 'new_content_for_' + path[len(path) - 1]
> + file_contents = "new content to " + path[len(path) - 1] + "\n"
> + globals()[var_name] = file_contents
> + new_rev = tweak_src_then_merge_to_dest(sbox, path_name,
> + copy_of_path_name, canon_path_name,
> + file_contents, new_rev)
> +
> + copy_of_A_D_wc_rev = new_rev
> + svntest.actions.run_and_verify_svn(None,
> + ['\n',
> + 'Committed revision ' + str(new_rev+1) + '.\n'],
> + [],
> + 'mkdir', sbox.repo_url + '/A/D/umlaut',
> + '-m', "log msg")
> + rev_to_merge_to_copy_of_D = new_rev + 1
...
Looks great, Kamesh. With the additional fixes suggested by Peter,
this is ready to commit and polish in the repository.
- application/pgp-signature attachment: stored
Received on Wed Feb 14 01:19:06 2007