[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH][merge-tracking] Reporter api calls should be made in depth first order (Fix and TestCase)

From: Daniel Rall <dlr_at_collab.net>
Date: 2007-02-10 06:30:40 CET

On Wed, 31 Jan 2007, Kamesh Jayachandran wrote:
...
> --- subversion/tests/cmdline/merge_tests.py (revision 23311)
> +++ subversion/tests/cmdline/merge_tests.py (working copy)
> @@ -4378,6 +4378,267 @@
...
> +def modify_and_commit_source_merge_the_resulting_rev_to_dst(sbox,
> + fs_src_path,
> + fs_dst_path,

The abbreviation "fs" is a bit overloaded in Subversion land. How
about "wc" instead? Or, drop the prefix entirely.

> + canon_src_path,
> + contents,
> + cur_rev):

How about one of these names instead:

  tweak_src_then_merge_to_dest
  merge_tweaked_src_to_dest

Move the specifics of the helper function into the doc string.
Indicate that it returns the new revision number created by the
commit.

> + # Edit src and commit it, creating revision cur_rev+1.

Either use the actual variable name fs_src_path, or use the word
source.

> + wc_dir = sbox.wc_dir
> + new_rev = cur_rev + 1
> + svntest.main.file_write(fs_src_path, contents)
> +
> + expected_output = svntest.wc.State(fs_src_path, {
> + '': Item(verb='Sending'),
> + })
> +
> + expected_status = wc.State(fs_src_path,
> + { '': Item(wc_rev=new_rev, status=' ')})
> +
> + svntest.actions.run_and_verify_commit(fs_src_path, expected_output,
> + expected_status, None,
> + None, None, None, None, fs_src_path)
> +
> + # Update the WC to new_rev so that it would be easier to expect everyone
> + # to be at new_rev.
> + svntest.actions.run_and_verify_svn(None, None, [], 'update', wc_dir)
> +
> + # Merge new_rev of fs_src_path to fs_dst_path
                                                  ^
I like to end sentences with a period, even in a comment. :)

> +
> + # Search for the comment entitled "The Merge Kluge" elsewhere in
> + # this file, to understand why we shorten and chdir() below.
> + short_fs_dst_path = shorten_path_kludge(fs_dst_path)
> + expected_status = wc.State(fs_dst_path,
> + { '': Item(wc_rev=new_rev, status='MM')})
> +
> + saved_cwd = os.getcwd()
> + try:
> + os.chdir(svntest.main.work_dir)
> + 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.

> + finally:
> + os.chdir(saved_cwd)
> +
> + svntest.actions.run_and_verify_status(fs_dst_path, expected_status)
> +
> + return new_rev
> +
> +def subtrees_with_merge_info_should_be_excluded_in_depth_first_order(sbox):
> + "exclude subtrees with mergeinfo in DF order"

I'd move the "_in_depth_first_order" suffix on the function name into
the doc string.

> +
> + #copy /A/D to /A/copy-of-D it results in RONE
                                              ^^^^
What's RONE? rONE? Please clarify. :)

> + #create children at different hierarchies having some merge-info
> + #to test the set_path calls on a reporter in a non-depth-first order.
> + #Childrens considered

I like # Spaces after comment characters.
I think # They're easier to read.

> + #/A/copy-of-D/G/pi
> + #/A/copy-of-D/G/rho
> + #/A/copy-of-D/G/tau
> + #/A/copy-of-D/G
> + #/A/copy-of-D/gamma
> + #/A/copy-of-D/H/chi
> + #/A/copy-of-D/H/omega
> + #/A/copy-of-D/H/psi
> + #/A/copy-of-D/H would have the mergeinfo.

If you think it's necessary to list all these paths, I'd probably drop
the redundant parent paths and list'em all on one or two lines.

> + #We run merges on individual files listed above.
> + #We create /A/D/umlaut directly over URL it results in rev RTWO
> + #When we merge /A/D with rRONE+1:RTWO it should merge smoothly

RTWO? R2D2! (A bad joke for Star Wars fans.)

> +
> + sbox.build()
> + wc_dir = sbox.wc_dir
> +
> + A_path = os.path.join(wc_dir, 'A')
> +
> + A_D_path = os.path.join(A_path, 'D')
> + copy_of_A_D_path = os.path.join(A_path, 'copy-of-D')
> +
> + A_D_gamma_path = os.path.join(A_D_path, 'gamma')
> + copy_of_A_D_gamma_path = os.path.join(copy_of_A_D_path, 'gamma')
> +
> + A_D_G_path = os.path.join(A_D_path, 'G')
> + copy_of_A_D_G_path = os.path.join(copy_of_A_D_path, 'G')
> +
> + A_D_G_pi_path = os.path.join(A_D_G_path, 'pi')
> + copy_of_A_D_G_pi_path = os.path.join(copy_of_A_D_G_path, 'pi')
> +
> + A_D_G_rho_path = os.path.join(A_D_G_path, 'rho')
> + copy_of_A_D_G_rho_path = os.path.join(copy_of_A_D_G_path, 'rho')
> +
> + A_D_G_tau_path = os.path.join(A_D_G_path, 'tau')
> + copy_of_A_D_G_tau_path = os.path.join(copy_of_A_D_G_path, 'tau')
> +
> + A_D_H_path = os.path.join(A_D_path, 'H')
> + copy_of_A_D_H_path = os.path.join(copy_of_A_D_path, 'H')
> +
> + A_D_H_chi_path = os.path.join(A_D_H_path, 'chi')
> + copy_of_A_D_H_chi_path = os.path.join(copy_of_A_D_H_path, 'chi')
> +
> + A_D_H_omega_path = os.path.join(A_D_H_path, 'omega')
> + copy_of_A_D_H_omega_path = os.path.join(copy_of_A_D_H_path, 'omega')
> +
> + A_D_H_psi_path = os.path.join(A_D_H_path, 'psi')
> + copy_of_A_D_H_psi_path = os.path.join(copy_of_A_D_H_path, 'psi')

I don't know if the added complexity is really worth it, but you could
reduce the code size of all these assignments to something like:

for path in (["A", "D"],
             ["A", "D", "gamma"],
             ["A", "D", "G"],
             ["A", "D", "G", "pi"],
             ["A", "D", "G", "rho"],
             ["A", "D", "G", "tau"],
             ["A", "D", "H"],
             ["A", "D", "H", "chi"],
             ["A", "D", "H", "omega"],
             ["A", "D", "H", "psi"]):
  var_name = "_".join(path) + "_path"
  locals()[var_name] = os.path.join(wc_dir, *path)
  path[len(path) - 1] = "copy-of-" + path[-1:][0]
  locals()["copy_of_" + var_name] = os.path.join(wc_dir, *path)

> + svntest.main.run_svn(None, "cp", A_D_path, copy_of_A_D_path)
> +
> + expected_output = svntest.wc.State(wc_dir, {
> + 'A/copy-of-D' : Item(verb='Adding'),
> + })
> + expected_status = svntest.actions.get_virginal_state(wc_dir, 1)
> + expected_status.add({
> + 'A/copy-of-D' : Item(status=' ', wc_rev=2),
> + 'A/copy-of-D/G' : Item(status=' ', wc_rev=2),
> + 'A/copy-of-D/G/pi' : Item(status=' ', wc_rev=2),
> + 'A/copy-of-D/G/rho' : Item(status=' ', wc_rev=2),
> + 'A/copy-of-D/G/tau' : Item(status=' ', wc_rev=2),
> + 'A/copy-of-D/H' : Item(status=' ', wc_rev=2),
> + 'A/copy-of-D/H/chi' : Item(status=' ', wc_rev=2),
> + 'A/copy-of-D/H/omega' : Item(status=' ', wc_rev=2),
> + 'A/copy-of-D/H/psi' : Item(status=' ', wc_rev=2),
> + 'A/copy-of-D/gamma' : Item(status=' ', wc_rev=2),
> + })
> + svntest.actions.run_and_verify_commit(wc_dir, expected_output,
> + expected_status, None,
> + None, None, None, None, wc_dir)
> +
> + # Edit A/D/G/pi and commit it, creating revision new_rev.
> + new_content_for_pi = 'new content to pi\n'
> + new_rev = modify_and_commit_source_merge_the_resulting_rev_to_dst(sbox,
> + A_D_G_pi_path,
> + copy_of_A_D_G_pi_path,
> + 'A/D/G/pi',
> + new_content_for_pi,
> + 2)
> +
> + # Edit A/D/G/rho and commit it, creating revision new_rev.
> + new_content_for_rho = 'new content to rho\n'
> + new_rev = modify_and_commit_source_merge_the_resulting_rev_to_dst(sbox,
> + A_D_G_rho_path,
> + copy_of_A_D_G_rho_path,
> + 'A/D/G/rho',
> + new_content_for_rho,
> + new_rev)
> +
> + # Edit A/D/G/tau and commit it, creating revision new_rev.
> + new_content_for_tau = 'new content to tau\n'
> + new_rev = modify_and_commit_source_merge_the_resulting_rev_to_dst(sbox,
> + A_D_G_tau_path,
> + copy_of_A_D_G_tau_path,
> + 'A/D/G/tau',
> + new_content_for_tau,
> + new_rev)
> +
> + # Edit A/D/H/chi and commit it, creating revision new_rev.
> + new_content_for_chi = 'new content to chi\n'
> + new_rev = modify_and_commit_source_merge_the_resulting_rev_to_dst(sbox,
> + A_D_H_chi_path,
> + copy_of_A_D_H_chi_path,
> + 'A/D/H/chi',
> + new_content_for_chi,
> + new_rev)
> +
> + # Edit A/D/H/omega and commit it, creating revision new_rev.
> + new_content_for_omega = 'new content to omega\n'
> + new_rev = modify_and_commit_source_merge_the_resulting_rev_to_dst(sbox,
> + A_D_H_omega_path,
> + copy_of_A_D_H_omega_path,
> + 'A/D/H/omega',
> + new_content_for_omega,
> + new_rev)
> +
> + # Edit A/D/H/psi and commit it, creating revision new_rev.
> + new_content_for_psi = 'new content to psi\n'
> + new_rev = modify_and_commit_source_merge_the_resulting_rev_to_dst(sbox,
> + A_D_H_psi_path,
> + copy_of_A_D_H_psi_path,
> + 'A/D/H/psi',
> + new_content_for_psi,
> + new_rev)
> +
> + # Edit A/D/gamma and commit it, creating revision new_rev.
> + new_content_for_gamma = 'new content to gamma\n'
> + new_rev = modify_and_commit_source_merge_the_resulting_rev_to_dst(sbox,
> + A_D_gamma_path,
> + copy_of_A_D_gamma_path,
> + 'A/D/gamma',
> + new_content_for_gamma,

I'd probably make a loop out of all these operations, using the same
class of code change I showed above.

> + 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
> +
> + # Search for the comment entitled "The Merge Kluge" elsewhere in
> + # this file, to understand why we shorten and chdir() below.
> + short_copy_of_A_D_path = shorten_path_kludge(copy_of_A_D_path)
> +
> + expected_output = wc.State(short_copy_of_A_D_path, {
> + 'umlaut' : Item(status='A '),
> + })
> +
> + expected_status = wc.State(short_copy_of_A_D_path, {
> + '' : Item(status=' M', wc_rev=copy_of_A_D_wc_rev),
> + 'G' : Item(status=' ', wc_rev=copy_of_A_D_wc_rev),
> + 'G/pi' : Item(status='MM', wc_rev=copy_of_A_D_wc_rev),
> + 'G/rho' : Item(status='MM', wc_rev=copy_of_A_D_wc_rev),
> + 'G/tau' : Item(status='MM', wc_rev=copy_of_A_D_wc_rev),
> + 'H' : Item(status=' ', wc_rev=copy_of_A_D_wc_rev),
> + 'H/chi' : Item(status='MM', wc_rev=copy_of_A_D_wc_rev),
> + 'H/omega' : Item(status='MM', wc_rev=copy_of_A_D_wc_rev),
> + 'H/psi' : Item(status='MM', wc_rev=copy_of_A_D_wc_rev),
> + 'gamma' : Item(status='MM', wc_rev=copy_of_A_D_wc_rev),
> + 'umlaut' : Item(status='A ', copied='+', wc_rev='-'),
> + })
> +
> + svn_merged_rev_val = "3-" + str(rev_to_merge_to_copy_of_D)

I'd give svn_merged_rev_val a name which reflects that it's a
rangelist, and probably have used a format specifier here; just think
it reads better:

     merged_rangelist = "3-%d" % rev_to_merge_to_copy_of_D

> + expected_disk = wc.State('', {
> + '' : Item(props={SVN_PROP_MERGE_INFO : '/A/D:' + svn_merged_rev_val}),
> + 'G' : Item(),
> + 'G/pi' : Item(new_content_for_pi,
> + props={SVN_PROP_MERGE_INFO : '/A/D/G/pi:' + svn_merged_rev_val}),
> + 'G/rho' : Item(new_content_for_rho,
> + props={SVN_PROP_MERGE_INFO : '/A/D/G/rho:' + svn_merged_rev_val}),
> + 'G/tau' : Item(new_content_for_tau,
> + props={SVN_PROP_MERGE_INFO : '/A/D/G/tau:' + svn_merged_rev_val}),
> + 'H' : Item(),
> + 'H/chi' : Item(new_content_for_chi,
> + props={SVN_PROP_MERGE_INFO : '/A/D/H/chi:' + svn_merged_rev_val}),
> + 'H/omega' : Item(new_content_for_omega,
> + props={SVN_PROP_MERGE_INFO : '/A/D/H/omega:' + svn_merged_rev_val}),
> + 'H/psi' : Item(new_content_for_psi,
> + props={SVN_PROP_MERGE_INFO : '/A/D/H/psi:' + svn_merged_rev_val}),
> + 'gamma' : Item(new_content_for_gamma,
> + props={SVN_PROP_MERGE_INFO : '/A/D/gamma:' + svn_merged_rev_val}),
> + 'umlaut' : Item(),
> + })
> + expected_skip = wc.State(short_copy_of_A_D_path, { })
> + saved_cwd = os.getcwd()
> + try:
> + os.chdir(svntest.main.work_dir)
> + svntest.actions.run_and_verify_merge(short_copy_of_A_D_path,
> + 2,
> + str(rev_to_merge_to_copy_of_D),
> + sbox.repo_url + '/A/D',
> + expected_output,
> + expected_disk,
> + expected_status,
> + expected_skip,
> + None,
> + None,
> + None,
> + None,
> + None, 1)
> + finally:
> + os.chdir(saved_cwd)

Nice work, Kamesh. Looking forward to the next patch.

- Dan

  • application/pgp-signature attachment: stored
Received on Sat Feb 10 06:30:51 2007

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.