Re: [PATCH] Fix issue #2829

> [[[
> Fix for issue #2829
>
> ## See http://subversion.tigris.org/issues/show_bug.cgi?id=2829. ##
>
> * subversion/libsvn_client/merge.c
> (determine_merges_performed): Logic added to handle mergeinfo
> allocation

You need to detail out the Logic.

> for skipped path parent dir and their associated children.
>
> * subversion/tests/cmdline/merge_tests.py
> (merge_tree_deleted_in_target): Modified expectation.
> (merge_skips_obstructions): Modified expectation.
> (mergeinfo_and_skipped_paths): Modified expectation.
> (mergeinfo_recording_in_skipped_merge): Modified expectation.
> (test_list): Remove XFail mark from
> 'mergeinfo_recording_in_skipped_merge'
>
> Patch by: Senthil Kumaran <senthil@collab.net>
> Reviewed by: kameshj

I reviewed the progress of the patch not the full patch itself.

My review comments are as follows,

> const void *skipped_path;
>-
>+ const void *immediate_parent_path;
>+

Be cautious about spurious spaces.

> /* Override the mergeinfo for child paths which weren't
> actually merged. */
> for (hi = apr_hash_first(NULL, notify_b->skipped_paths); hi;
> hi = apr_hash_next(hi))
> {
>+ const void *parent_path;
>+ const svn_wc_entry_t *parent_dir_entry;
>+
>+ parent_dir_entry = NULL;

Assignment and declaration can be on one line.

> apr_hash_this(hi, &skipped_path, NULL, NULL);
>-
>+

Be cautious about spurious spaces.

>+ immediate_parent_path = parent_path;

You don't need 'immediate_parent_path' as it is scoped outside, it will
retain the last iteration path only.

>+ for (hi = apr_hash_first(NULL, skipped_parents); hi;

>+ hi = apr_hash_next(hi))

>+ {

>+ apr_hash_index_t *child_hi;

>+ apr_hash_t *child_entries;

>+ const void *parent_path;

>+

Spurious space(TAB) here.

>+ for (child_hi = apr_hash_first(NULL, child_entries); child_hi;

>+ child_hi = apr_hash_next(child_hi))

>+ {

>+ const void *child_path;

>+

>+ apr_hash_this(child_hi, &child_path, NULL, NULL);

>+ child_path = svn_path_join((const char *)parent_path,

>+ (const char *)child_path, pool);

>+

You need to ignore the "THIS_ENTRY".

>+ /* If child path is present in the merges hash table, then

>+ it has been assigned EMPTY or MERGE_RANGES as mergeinfo

>+ previously */

>+ if (!apr_hash_get(*merges, child_path, APR_HASH_KEY_STRING))

>+ {

>+ if (svn_path_compare_paths(immediate_parent_path,

>+ child_path))

This check is *bogus* as 'immediate_parent_path' is the last skipped
paths' dirname which is something we should not bother about, I hope
your intension about this block of code is to handle "THIS_ENTRY" which
you can handle outside.
You may need to take care of deleted child before setting mergeinfo. i.e
if B/E/alpha is getting skipped due to 'scheduled deletion of B/E', Your
code set's a mergeinfo on 'B/E' which is illogical.

>@@ -1005,13 +1005,14 @@

> })

> expected_disk = wc.State('', {

> '' : Item(props={SVN_PROP_MERGE_INFO : '/A/B:1,3'}),

>- 'F' : Item(),

>- 'lambda' : Item("This is the file 'lambda'.\nchange lambda.\n"),

>+ 'F' : Item(props={SVN_PROP_MERGE_INFO : '/A/B/F:3'}),

>+ 'lambda' : Item(contents="This is the file 'lambda'.\nchange lambda.\n",

>+ props={SVN_PROP_MERGE_INFO : '/A/B/lambda:3'}),

> })

Here '' should have a mergeinfo of '/A/B:1'
        'F' and 'lambda' should have a mergeinfo of '/A/B:1,3'

> expected_status = wc.State(I_path, {

> '' : Item(status=' M'),

As per above point here '' should not have prop modified.

>@@ -1834,7 +1835,7 @@

> })

> expected_disk = wc.State('', {

> '' : Item(props={SVN_PROP_MERGE_INFO : '/A/B/F:2'}),

Here '' should have no merge info as some of its child got skipped of merge.

>@@ -1881,7 +1882,8 @@

> expected_disk = wc.State('', {

> '' : Item(props={SVN_PROP_MERGE_INFO : '/A/B/F:2'}),

Here '' should have no merge info as some of its child got skipped of merge.

>@@ -7217,45 +7219,48 @@

> })

> expected_disk = wc.State('', {

> '' : Item(props={SVN_PROP_MERGE_INFO : '/A:1,5-8'}),

>+ 'D/H/psi' : Item(contents="New content",

>+ props={SVN_PROP_MERGE_INFO : '/A/D/H/psi:5-8'}),

Here the mergeinfo should be '/A/D/H/psi:1,5-8'.

>+ 'D/H/chi' : Item(contents="This is the file 'chi'.\n",

>+ props={SVN_PROP_MERGE_INFO : '/A/D/H/chi:5-8'}),

Here the mergeinfo should be '/A/D/H/chi:1,5-8'.

>- 'B/lambda' : Item("This is the file 'lambda'.\n"),

>+ 'B/lambda' : Item(contents="This is the file 'lambda'.\n",

>+ props={SVN_PROP_MERGE_INFO : '/A/B/lambda:5-8'}),

Here the mergeinfo should be '/A/B/lambda:1,5-8'.

> 'B/E' : Item(props={SVN_PROP_MERGE_INFO : '/A/B/E:1'}),

> 'B/E/alpha' : Item("This is the file 'alpha'.\n"),

> 'B/E/beta' : Item("This is the file 'beta'.\n"),

>- 'B/F' : Item(),

>- 'B' : Item(),

>+ 'B/F' : Item(props={SVN_PROP_MERGE_INFO : '/A/B/F:5-8'}),

Here the mergeinfo should be '/A/B/F:1,5-8'.

>@@ -7326,12 +7331,13 @@

> 'D/gamma' : Item("This is the file 'gamma'.\n",

> props={SVN_PROP_MERGE_INFO : '/A/D/gamma:1-2,5-8'}),

> 'D' : Item(props={SVN_PROP_MERGE_INFO : '/A/D:1-2,5-8*'}),

>- 'B/lambda' : Item("This is the file 'lambda'.\n"),

>+ 'B/lambda' : Item(contents="This is the file 'lambda'.\n",

>+ props={SVN_PROP_MERGE_INFO : '/A/B/lambda:5-8'}),

Here the mergeinfo should be '/A/B/lambda:1-2,5-8'

> 'B/E' : Item(props={SVN_PROP_MERGE_INFO : '/A/B/E:1-2'}),

> 'B/E/alpha' : Item("This is the file 'alpha'.\n"),

> 'B/E/beta' : Item("This is the file 'beta'.\n"),

>- 'B/F' : Item(),

>- 'B' : Item(),

>+ 'B/F' : Item(props={SVN_PROP_MERGE_INFO : '/A/B/F:5-8'}),

Here the mergeinfo should be '/A/B/F:1-2,5-8'

Thanks

With regards
Kamesh Jayachandran

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Oct 17 13:21:53 2007