You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by cm...@apache.org on 2012/11/28 20:48:41 UTC
svn commit: r1414880 - /subversion/trunk/subversion/libsvn_client/merge.c
Author: cmpilato
Date: Wed Nov 28 19:48:40 2012
New Revision: 1414880
URL: http://svn.apache.org/viewvc?rev=1414880&view=rev
Log:
After consultation with pburba, restore the 'added_path' merge context
baton member and related functionality removed in r1414810, using it
as a preferred sourch of information about added parent directories,
and only falling back to the more expensive consultation of the
'dry_run_added' hash when we have to. Though, rather than a *precise*
reintroduction of that baton member, call its restored form
'dry_run_last_added_dir' and position it alongside the 'dry_run_added'
baton member.
* subversion/libsvn_client/merge.c
(merge_cmd_baton_t): Add new 'dry_run_last_added_dir' member.
(dry_run_added_parent_p): Add 'local_abspath' parameter, and rename
'edit_relpath' to 'local_relpath'. Now check the incoming paths
against the merge baton's 'dry_run_last_added_dir' as an
optimization before consulting the 'dry_run_added' hash.
(merge_file_added): Update call to dry_run_added_parent_p().
(merge_dir_added): Manage the state of the new 'dry_run_last_added_dir'
baton member. Update call to dry_run_added_parent_p().
(do_merge): Reset the new 'dry_run_last_added_dir' context baton member, too.
Modified:
subversion/trunk/subversion/libsvn_client/merge.c
Modified: subversion/trunk/subversion/libsvn_client/merge.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/merge.c?rev=1414880&r1=1414879&r2=1414880&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_client/merge.c (original)
+++ subversion/trunk/subversion/libsvn_client/merge.c Wed Nov 28 19:48:40 2012
@@ -286,9 +286,10 @@ typedef struct merge_cmd_baton_t {
dry_run mode. */
apr_hash_t *dry_run_deletions;
- /* The list of paths for entries we've added, used only when in
- dry_run mode. */
+ /* The list of paths for entries we've added and the most
+ recently added directory. (Used only when in dry_run mode.) */
apr_hash_t *dry_run_added;
+ const char *dry_run_last_added_dir;
/* The list of any paths which remained in conflict after a
resolution attempt was made. We track this in-memory, rather
@@ -487,32 +488,45 @@ dry_run_added_p(const merge_cmd_baton_t
APR_HASH_KEY_STRING) != NULL);
}
-/* Return true iff we're in dry-run mode and a parent of EDIT_RELPATH
- would have been added by now if we weren't in dry-run mode.
- Used to avoid spurious notifications (e.g. conflicts) from a merge
- attempt into an existing target which would have been deleted if we
- weren't in dry_run mode (issue #2584). Assumes that WCPATH is
- still versioned (e.g. has an associated entry). */
+/* Return true iff we're in dry-run mode and a parent of
+ LOCAL_RELPATH/LOCAL_ABSPATH would have been added by now if we
+ weren't in dry-run mode. Used to avoid spurious notifications
+ (e.g. conflicts) from a merge attempt into an existing target which
+ would have been deleted if we weren't in dry_run mode (issue
+ #2584). Assumes that WCPATH is still versioned (e.g. has an
+ associated entry). */
static svn_boolean_t
dry_run_added_parent_p(const merge_cmd_baton_t *merge_b,
- const char *edit_relpath,
+ const char *local_relpath,
+ const char *local_abspath,
apr_pool_t *scratch_pool)
{
+ const char *abspath = local_abspath;
int i;
- const char *abspath;
if (!merge_b->dry_run)
return FALSE;
- abspath = svn_dirent_join(merge_b->target->abspath, edit_relpath,
- scratch_pool);
- for (i = 0; i < (svn_path_component_count(edit_relpath) - 1); i++)
+ /* See if LOCAL_ABSPATH is a child of the most recently added
+ directory. This is an optimization over searching through
+ dry_run_added that plays to the strengths of the editor's drive
+ ordering constraints. In fact, we need the fallback approach
+ below only because of ra_serf's insufficiencies in this area. */
+ if (merge_b->dry_run_last_added_dir
+ && svn_dirent_is_child(merge_b->dry_run_last_added_dir,
+ local_abspath, NULL))
+ return TRUE;
+
+ /* The fallback: see if any of LOCAL_ABSPATH's parents have been
+ added in this merge so far. */
+ for (i = 0; i < (svn_path_component_count(local_relpath) - 1); i++)
{
abspath = svn_dirent_dirname(abspath, scratch_pool);
if (apr_hash_get(merge_b->dry_run_added, abspath,
APR_HASH_KEY_STRING))
return TRUE;
}
+
return FALSE;
}
@@ -1884,7 +1898,8 @@ merge_file_added(svn_wc_notify_state_t *
if (obstr_state != svn_wc_notify_state_inapplicable)
{
if (merge_b->dry_run
- && dry_run_added_parent_p(merge_b, mine_relpath, scratch_pool))
+ && dry_run_added_parent_p(merge_b, mine_relpath,
+ mine_abspath, scratch_pool))
{
if (content_state)
*content_state = svn_wc_notify_state_changed;
@@ -2364,7 +2379,8 @@ merge_dir_added(svn_wc_notify_state_t *s
if (obstr_state != svn_wc_notify_state_inapplicable)
{
if (state && merge_b->dry_run
- && dry_run_added_parent_p(merge_b, local_relpath, scratch_pool))
+ && dry_run_added_parent_p(merge_b, local_relpath,
+ local_abspath, scratch_pool))
{
*state = svn_wc_notify_state_changed;
}
@@ -2386,10 +2402,10 @@ merge_dir_added(svn_wc_notify_state_t *s
/* Unversioned or schedule-delete */
if (merge_b->dry_run)
{
- const char *added_path = apr_pstrdup(merge_b->pool,
- local_abspath);
- apr_hash_set(merge_b->dry_run_added, added_path,
- APR_HASH_KEY_STRING, added_path);
+ merge_b->dry_run_last_added_dir =
+ apr_pstrdup(merge_b->pool, local_abspath);
+ apr_hash_set(merge_b->dry_run_added, merge_b->dry_run_last_added_dir,
+ APR_HASH_KEY_STRING, merge_b->dry_run_last_added_dir);
}
else
{
@@ -2425,10 +2441,8 @@ merge_dir_added(svn_wc_notify_state_t *s
}
else
{
- const char *added_path = apr_pstrdup(merge_b->pool,
- local_abspath);
- apr_hash_set(merge_b->dry_run_added, added_path,
- APR_HASH_KEY_STRING, added_path);
+ merge_b->dry_run_last_added_dir =
+ apr_pstrdup(merge_b->pool, local_abspath);
}
if (state)
*state = svn_wc_notify_state_changed;
@@ -2467,6 +2481,9 @@ merge_dir_added(svn_wc_notify_state_t *s
}
break;
case svn_node_file:
+ if (merge_b->dry_run)
+ merge_b->dry_run_last_added_dir = NULL;
+
if (is_versioned && dry_run_deleted_p(merge_b, local_abspath))
{
/* ### TODO: Retain record of this dir being added to
@@ -2488,6 +2505,8 @@ merge_dir_added(svn_wc_notify_state_t *s
}
break;
default:
+ if (merge_b->dry_run)
+ merge_b->dry_run_last_added_dir = NULL;
if (state)
*state = svn_wc_notify_state_unknown;
break;
@@ -9199,6 +9218,7 @@ do_merge(apr_hash_t **modified_subtrees,
dry_run ? apr_hash_make(iterpool) : NULL;
merge_cmd_baton.dry_run_added =
dry_run ? apr_hash_make(iterpool) : NULL;
+ merge_cmd_baton.dry_run_last_added_dir = NULL;
merge_cmd_baton.conflicted_paths = NULL;
merge_cmd_baton.paths_with_new_mergeinfo = NULL;
merge_cmd_baton.paths_with_deleted_mergeinfo = NULL;
Re: svn commit: r1414880 - /subversion/trunk/subversion/libsvn_client/merge.c
Posted by Paul Burba <pt...@gmail.com>.
On Wed, Nov 28, 2012 at 8:27 PM, C. Michael Pilato <cm...@collab.net> wrote:
> On 11/28/2012 03:39 PM, Julian Foad wrote:
>> @@ -2425,10 +2441,8 @@ merge_dir_added(svn_wc_notify_state_t *s
>>> }
>>> else
>>> {
>>> - const char *added_path = apr_pstrdup(merge_b->pool,
>>> - local_abspath);
>>> - apr_hash_set(merge_b->dry_run_added, added_path,
>>> - APR_HASH_KEY_STRING, added_path);
>>> + merge_b->dry_run_last_added_dir =
>>> + apr_pstrdup(merge_b->pool, local_abspath);
>>
>> Oops -- no longer setting the hash here. Maybe use a macro or function to encapsulate both parts of the "registration".
>
> Hrm... the original logic didn't add it to the hash here, either. I seem to
> have made that additional apr_hash_set() accidentally in r1414810. You can
> examine the sum of my changes to this file like so:
>
> svn diff -r1414809:1414880 subversion/libsvn_client/merge.c
>
> Doing so reveals that, in the end, I didn't actually changed the behavior in
> this code section. And yet ... it does make me wonder if this is just some
> latent bug waiting to be revealed. Certainly seems suspect.
>
> Paul: have you any opinions here? To summarize, I'm wondering why, in
> merge_dir_added(), in the switch that begins like so:
>
> /* Switch on the on-disk state of this path */
> switch (kind)
>
> ... the merged addition of a brand new directory earns that directory a
> registration in both the dry_run_last_added_dir slot *and* the dry_run_added
> hash, but a merged directory addition atop an unversioned or previously
> deleted directory only winds up in dry_run_last_added_dir (and *not* the
> dry_run_added hash). Any ideas?
Julian is correct, that's a bug in the original code. We can see the
problem in merge_tests.py 2 if we add some unversioned directories
which obstruct incoming directory adds:
>mkdir A\C\Q A\C\Q2
>svn st
? A\C\Q
? A\C\Q2
>svn merge ^^/A/B/F A\C -r1:2
--- Merging r2 into 'A\C':
A A\C\Q
A A\C\Q2
A A\C\Q\bar
A A\C\Q\bar2
A A\C\foo
A A\C\foo2
--- Recording mergeinfo for merge of r2 into 'A\C':
U A\C
Do the same with a --dry-run and the output is skipped (as per Mike's
earlier problems http://svn.haxx.se/dev/archive-2012-11/0681.shtml):
>svn revert -Rq . & mkdir A\C\Q A\C\Q2
>svn merge ^^/A/B/F A\C -r1:2 --dry-run
--- Merging r2 into 'A\C':
A A\C\Q
A A\C\Q2
Skipped 'A\C\Q\bar'
Skipped 'A\C\Q\bar2'
A A\C\foo
A A\C\foo2
Summary of conflicts:
Skipped paths: 2
Fixed and added test in r1415214.
>>> }
>>> if (state)
>>> *state = svn_wc_notify_state_changed;
>>> @@ -2467,6 +2481,9 @@ merge_dir_added(svn_wc_notify_state_t *s
>>> }
>>> break;
>>> case svn_node_file:
>>> + if (merge_b->dry_run)
>>> + merge_b->dry_run_last_added_dir = NULL;
>>
>> Clearing that path is just an optimization, right? (Also below.)
>
> As far as I can tell, yes. As above, I was merely restoring the behavior to
> what it was before I started mucking with this code at all.
Hmmm, that code goes *way* back. I 'm not sure it's even needed
anymore or if it ever was.
> --
> C. Michael Pilato <cm...@collab.net>
> CollabNet <> www.collab.net <> Enterprise Cloud Development
--
Paul T. Burba
CollabNet, Inc. -- www.collab.net -- Enterprise Cloud Development
Skype: ptburba
Re: svn commit: r1414880 - /subversion/trunk/subversion/libsvn_client/merge.c
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 11/28/2012 08:27 PM, C. Michael Pilato wrote:
> Paul: have you any opinions here? To summarize, I'm wondering why, in
> merge_dir_added(), in the switch that begins like so:
>
> /* Switch on the on-disk state of this path */
> switch (kind)
>
> ... the merged addition of a brand new directory earns that directory a
> registration in both the dry_run_last_added_dir slot *and* the dry_run_added
> hash, but a merged directory addition atop an unversioned or previously
> deleted directory only winds up in dry_run_last_added_dir (and *not* the
> dry_run_added hash). Any ideas?
Looks like Paul agreed that there was a potential problem here, given r1415214.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development
Re: svn commit: r1414880 - /subversion/trunk/subversion/libsvn_client/merge.c
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 11/28/2012 03:39 PM, Julian Foad wrote:
> @@ -2425,10 +2441,8 @@ merge_dir_added(svn_wc_notify_state_t *s
>> }
>> else
>> {
>> - const char *added_path = apr_pstrdup(merge_b->pool,
>> - local_abspath);
>> - apr_hash_set(merge_b->dry_run_added, added_path,
>> - APR_HASH_KEY_STRING, added_path);
>> + merge_b->dry_run_last_added_dir =
>> + apr_pstrdup(merge_b->pool, local_abspath);
>
> Oops -- no longer setting the hash here. Maybe use a macro or function to encapsulate both parts of the "registration".
Hrm... the original logic didn't add it to the hash here, either. I seem to
have made that additional apr_hash_set() accidentally in r1414810. You can
examine the sum of my changes to this file like so:
svn diff -r1414809:1414880 subversion/libsvn_client/merge.c
Doing so reveals that, in the end, I didn't actually changed the behavior in
this code section. And yet ... it does make me wonder if this is just some
latent bug waiting to be revealed. Certainly seems suspect.
Paul: have you any opinions here? To summarize, I'm wondering why, in
merge_dir_added(), in the switch that begins like so:
/* Switch on the on-disk state of this path */
switch (kind)
... the merged addition of a brand new directory earns that directory a
registration in both the dry_run_last_added_dir slot *and* the dry_run_added
hash, but a merged directory addition atop an unversioned or previously
deleted directory only winds up in dry_run_last_added_dir (and *not* the
dry_run_added hash). Any ideas?
>> }
>> if (state)
>> *state = svn_wc_notify_state_changed;
>> @@ -2467,6 +2481,9 @@ merge_dir_added(svn_wc_notify_state_t *s
>> }
>> break;
>> case svn_node_file:
>> + if (merge_b->dry_run)
>> + merge_b->dry_run_last_added_dir = NULL;
>
> Clearing that path is just an optimization, right? (Also below.)
As far as I can tell, yes. As above, I was merely restoring the behavior to
what it was before I started mucking with this code at all.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development
Re: svn commit: r1414880 - /subversion/trunk/subversion/libsvn_client/merge.c
Posted by Julian Foad <ju...@btopenworld.com>.
> Author: cmpilato
> URL: http://svn.apache.org/viewvc?rev=1414880&view=rev
> Log:
> After consultation with pburba, restore the 'added_path' merge context
> baton member and related functionality removed in r1414810, using it
> as a preferred sourch of information about added parent directories,
> and only falling back to the more expensive consultation of the
> 'dry_run_added' hash when we have to. Though, rather than a *precise*
> reintroduction of that baton member, call its restored form
> 'dry_run_last_added_dir' and position it alongside the
> 'dry_run_added'
> baton member.
>
> * subversion/libsvn_client/merge.c
> (merge_cmd_baton_t): Add new 'dry_run_last_added_dir' member.
> (dry_run_added_parent_p): Add 'local_abspath' parameter, and rename
> 'edit_relpath' to 'local_relpath'. Now check the incoming
> paths
> against the merge baton's 'dry_run_last_added_dir' as an
> optimization before consulting the 'dry_run_added' hash.
> (merge_file_added): Update call to dry_run_added_parent_p().
> (merge_dir_added): Manage the state of the new
> 'dry_run_last_added_dir'
> baton member. Update call to dry_run_added_parent_p().
> (do_merge): Reset the new 'dry_run_last_added_dir' context baton
> member, too.
> Modified: subversion/trunk/subversion/libsvn_client/merge.c
> ==============================================================================
> @@ -286,9 +286,10 @@ typedef struct merge_cmd_baton_t {
>
> - /* The list of paths for entries we've added, used only when in
> - dry_run mode. */
> + /* The list of paths for entries we've added and the most
> + recently added directory. (Used only when in dry_run mode.) */
> apr_hash_t *dry_run_added;
> + const char *dry_run_last_added_dir;
> @@ -487,32 +488,45 @@ dry_run_added_p(const merge_cmd_baton_t
> APR_HASH_KEY_STRING) != NULL);
> }
>
> -/* Return true iff we're in dry-run mode and a parent of EDIT_RELPATH
> - would have been added by now if we weren't in dry-run mode.
> - Used to avoid spurious notifications (e.g. conflicts) from a merge
> - attempt into an existing target which would have been deleted if we
> - weren't in dry_run mode (issue #2584). Assumes that WCPATH is
> - still versioned (e.g. has an associated entry). */
> +/* Return true iff we're in dry-run mode and a parent of
> + LOCAL_RELPATH/LOCAL_ABSPATH would have been added by now if we
> + weren't in dry-run mode. Used to avoid spurious notifications
> + (e.g. conflicts) from a merge attempt into an existing target which
> + would have been deleted if we weren't in dry_run mode (issue
> + #2584). Assumes that WCPATH is still versioned (e.g. has an
> + associated entry). */
> static svn_boolean_t
> dry_run_added_parent_p(const merge_cmd_baton_t *merge_b,
> - const char *edit_relpath,
> + const char *local_relpath,
> + const char *local_abspath,
> apr_pool_t *scratch_pool)
> {
> + const char *abspath = local_abspath;
> int i;
> - const char *abspath;
>
> if (!merge_b->dry_run)
> return FALSE;
>
> - abspath = svn_dirent_join(merge_b->target->abspath, edit_relpath,
> - scratch_pool);
> - for (i = 0; i < (svn_path_component_count(edit_relpath) - 1); i++)
> + /* See if LOCAL_ABSPATH is a child of the most recently added
> + directory. This is an optimization over searching through
> + dry_run_added that plays to the strengths of the editor's drive
> + ordering constraints. In fact, we need the fallback approach
> + below only because of ra_serf's insufficiencies in this area. */
> + if (merge_b->dry_run_last_added_dir
> + && svn_dirent_is_child(merge_b->dry_run_last_added_dir,
> + local_abspath, NULL))
> + return TRUE;
> +
> + /* The fallback: see if any of LOCAL_ABSPATH's parents have been
> + added in this merge so far. */
> + for (i = 0; i < (svn_path_component_count(local_relpath) - 1); i++)
> {
> abspath = svn_dirent_dirname(abspath, scratch_pool);
> if (apr_hash_get(merge_b->dry_run_added, abspath,
> APR_HASH_KEY_STRING))
> return TRUE;
> }
> +
> return FALSE;
> }
>
> @@ -2386,10 +2402,10 @@ merge_dir_added(svn_wc_notify_state_t *s
> /* Unversioned or schedule-delete */
> if (merge_b->dry_run)
> {
> - const char *added_path = apr_pstrdup(merge_b->pool,
> - local_abspath);
> - apr_hash_set(merge_b->dry_run_added, added_path,
> - APR_HASH_KEY_STRING, added_path);
> + merge_b->dry_run_last_added_dir =
> + apr_pstrdup(merge_b->pool, local_abspath);
> + apr_hash_set(merge_b->dry_run_added,
> merge_b->dry_run_last_added_dir,
> + APR_HASH_KEY_STRING,
> merge_b->dry_run_last_added_dir);
> }
> else
> {
> @@ -2425,10 +2441,8 @@ merge_dir_added(svn_wc_notify_state_t *s
> }
> else
> {
> - const char *added_path = apr_pstrdup(merge_b->pool,
> - local_abspath);
> - apr_hash_set(merge_b->dry_run_added, added_path,
> - APR_HASH_KEY_STRING, added_path);
> + merge_b->dry_run_last_added_dir =
> + apr_pstrdup(merge_b->pool, local_abspath);
Oops -- no longer setting the hash here. Maybe use a macro or function to encapsulate both parts of the "registration".
> }
> if (state)
> *state = svn_wc_notify_state_changed;
> @@ -2467,6 +2481,9 @@ merge_dir_added(svn_wc_notify_state_t *s
> }
> break;
> case svn_node_file:
> + if (merge_b->dry_run)
> + merge_b->dry_run_last_added_dir = NULL;
Clearing that path is just an optimization, right? (Also below.)
> if (is_versioned && dry_run_deleted_p(merge_b, local_abspath))
> {
> /* ### TODO: Retain record of this dir being added to
> @@ -2488,6 +2505,8 @@ merge_dir_added(svn_wc_notify_state_t *s
> }
> break;
> default:
> + if (merge_b->dry_run)
> + merge_b->dry_run_last_added_dir = NULL;
> if (state)
> *state = svn_wc_notify_state_unknown;
> break;
> @@ -9199,6 +9218,7 @@ do_merge(apr_hash_t **modified_subtrees,
> dry_run ? apr_hash_make(iterpool) : NULL;
> merge_cmd_baton.dry_run_added =
> dry_run ? apr_hash_make(iterpool) : NULL;
> + merge_cmd_baton.dry_run_last_added_dir = NULL;
> merge_cmd_baton.conflicted_paths = NULL;
> merge_cmd_baton.paths_with_new_mergeinfo = NULL;
> merge_cmd_baton.paths_with_deleted_mergeinfo = NULL;
>