You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Stefan Sperling <st...@elego.de> on 2009/08/20 22:35:43 UTC
Re: svn commit: r38896 - trunk/subversion/libsvn_client
On Thu, Aug 20, 2009 at 02:55:04PM -0700, Stephen Butler wrote:
> Author: sbutler
> Date: Thu Aug 20 14:55:04 2009
> New Revision: 38896
>
> Log:
> Report duplicate tree conflict errors in just one place (follow-up
> to r38872).
>
> * subversion/libsvn_client/merge.c
> (tree_conflict_on_add): Remove redundant error handling.
> + /* A merge may send two separate tree-conflicts if the merge
> + replaces the item. This means merge will first set a tree-conflict
> + with an incoming "delete", and then one with an incoming "add". */
> + if (existing_conflict != NULL
> + && existing_conflict->action == svn_wc_conflict_action_delete
> + && conflict->action == svn_wc_conflict_action_add)
Won't the above check also need similar treatment as done in
r38841 and r38848 ?
Stefan
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2385832
Re: svn commit: r38896 - trunk/subversion/libsvn_client
Posted by Neels Janosch Hofmeyr <ne...@elego.de>.
Stephen Butler wrote:
> Maybe we should check for (A,D) as well as (D,A).
A sequence of (A,D) should amount to nothing indeed. The editor driver sees
the file not existing in both left and right side and doesn't bother. And I
think it isn't allowed by the specs of the editor, either.
~Neels
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2386181
Re: svn commit: r38896 - trunk/subversion/libsvn_client
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Aug 21, 2009 at 01:03:48AM +0100, Stefan Sperling wrote:
> On Fri, Aug 21, 2009 at 01:29:50AM +0200, Stephen Butler wrote:
> > Is somebody calling add_file(mu), then delete_entry(mu)?
>
> Yes! I checked it in the debugger, the add happens before the
> delete. I think the culprit is in libsvn_repos/reporter.c, at
> least for replacements.
Captain, reverse, reverse!
I can't reproduce this anymore.
I will shut up for now and post again when I'm sure what I'm
talking about.
Stefan
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2385930
Re: svn commit: r38896 - trunk/subversion/libsvn_client
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Aug 21, 2009 at 01:29:50AM +0200, Stephen Butler wrote:
> Regarding your comment in 38848,
>
> Failing test merge_test 132:
> There is nothing that guarantees that the delete will come
> before the add. If you trace the merge done in merge_test 132
> and break at subversion/libsvn_client/merge.c:merge_file_added
> and subversion/libsvn_client/merge.c:merge_file_deleted, you
> can see that the add gets called first, then the delete (for
> the file "mu").
>
> That sounds to me like the delta editor driver is broken. Rule
> number one in svn_delta.h says,
>
> 1. The producer may call open_directory, add_directory, open_file,
> add_file at most once on any given directory entry. delete_entry may
> be called at most once on any given directory entry and may later be
> followed by add_directory or add_file on the same directory
> entry. delete_entry may not be called on any directory entry after
> open_directory, add_directory, open_file or add_file has been called
> on that directory entry.
>
> Is somebody calling add_file(mu), then delete_entry(mu)?
Yes! I checked it in the debugger, the add happens before the
delete. I think the culprit is in libsvn_repos/reporter.c, at
least for replacements. It has two lists, one for entries in
the source and one for entries in the target, and issues deletes
for items (identified by path) which are in source but not in target,
and then goes on to do a normal directory diff (during which the order
is, probably random, I haven't checked). That strategy breaks in the
replace case, I think, because the path appears on both sides
if there has been a replace.
I've tried the patch below but it doesn't seem to fix anything.
I don't know this code well so I might have missed something
or the approach I'm taking is totally wrong for a reason I don't
know. Will dig deeper into this tomorrow. I'm certain that merge_test
132 traverses this code path while replacing mu.
Any comments/help appreciated.
BTW, Greg told me that the convention about deletes coming first was
a convention in the docs but there was nothing in the code which
enforces it.
Stefan
Index: subversion/libsvn_repos/reporter.c
===================================================================
--- subversion/libsvn_repos/reporter.c (revision 38887)
+++ subversion/libsvn_repos/reporter.c (working copy)
@@ -1043,7 +1043,8 @@ delta_dirs(report_baton_t *b, svn_revnum_t s_rev,
}
/* Remove any deleted entries. Do this before processing the
- target, for graceful handling of case-only renames. */
+ target, for graceful handling of case-only renames, and
+ replacements. */
if (s_entries)
{
for (hi = apr_hash_first(pool, s_entries);
@@ -1051,33 +1052,29 @@ delta_dirs(report_baton_t *b, svn_revnum_t s_rev,
hi = apr_hash_next(hi))
{
const svn_fs_dirent_t *s_entry;
+ svn_revnum_t deleted_rev;
svn_pool_clear(subpool);
s_entry = svn_apr_hash_index_val(hi);
- if (apr_hash_get(t_entries, s_entry->name,
- APR_HASH_KEY_STRING) == NULL)
- {
- svn_revnum_t deleted_rev;
+ if (s_entry->kind == svn_node_file
+ && wc_depth < svn_depth_files)
+ continue;
- if (s_entry->kind == svn_node_file
- && wc_depth < svn_depth_files)
- continue;
+ if (s_entry->kind == svn_node_dir
+ && (wc_depth < svn_depth_immediates
+ || requested_depth == svn_depth_files))
+ continue;
- if (s_entry->kind == svn_node_dir
- && (wc_depth < svn_depth_immediates
- || requested_depth == svn_depth_files))
- continue;
-
- /* There is no corresponding target entry, so delete. */
- e_fullpath = svn_path_join(e_path, s_entry->name, subpool);
- SVN_ERR(svn_repos_deleted_rev(svn_fs_root_fs(b->t_root),
- svn_path_join(t_path,
- s_entry->name,
- subpool),
- s_rev, b->t_rev,
- &deleted_rev, subpool));
-
+ e_fullpath = svn_path_join(e_path, s_entry->name, subpool);
+ SVN_ERR(svn_repos_deleted_rev(svn_fs_root_fs(b->t_root),
+ svn_path_join(t_path,
+ s_entry->name,
+ subpool),
+ s_rev, b->t_rev,
+ &deleted_rev, subpool));
+ if (deleted_rev != SVN_INVALID_REVNUM)
+ {
SVN_ERR(b->editor->delete_entry(e_fullpath,
deleted_rev,
dir_baton, subpool));
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2385854
Re: svn commit: r38896 - trunk/subversion/libsvn_client
Posted by Stephen Butler <sb...@elego.de>.
Quoting Stephen Butler <sb...@elego.de>:
> Quoting Stefan Sperling <st...@elego.de>:
>
>> On Thu, Aug 20, 2009 at 02:55:04PM -0700, Stephen Butler wrote:
>>> Author: sbutler
>>> Date: Thu Aug 20 14:55:04 2009
>>> New Revision: 38896
>>>
>>> Log:
>>> Report duplicate tree conflict errors in just one place (follow-up
>>> to r38872).
>>>
>>> * subversion/libsvn_client/merge.c
>>> (tree_conflict_on_add): Remove redundant error handling.
>>
>>> + /* A merge may send two separate tree-conflicts if the merge
>>> + replaces the item. This means merge will first set a tree-conflict
>>> + with an incoming "delete", and then one with an incoming "add". */
>>> + if (existing_conflict != NULL
>>> + && existing_conflict->action == svn_wc_conflict_action_delete
>>> + && conflict->action == svn_wc_conflict_action_add)
>>
>> Won't the above check also need similar treatment as done in
>> r38841 and r38848 ?
>
> Maybe we should check for (A,D) as well as (D,A).
>
> It's weird that in repos_diff.c an item is either a tree conflict
> *or* an add or a delete, etc. (in the svn_wc_notify_t_* enum).
>
> In merge.c (in the conflict struct) the tree conflict status
> is orthogonal to the action.
>
> I guess we thought a tree conflict victim didn't need any other
> notification, so it should stand alone. But since then we've
> avoided skipping the victims, at least for update. It'd be nice
> to fix the nofification to show what happened (e.g., 'R C mu').
Regarding your comment in 38848,
Failing test merge_test 132:
There is nothing that guarantees that the delete will come
before the add. If you trace the merge done in merge_test 132
and break at subversion/libsvn_client/merge.c:merge_file_added
and subversion/libsvn_client/merge.c:merge_file_deleted, you
can see that the add gets called first, then the delete (for
the file "mu").
That sounds to me like the delta editor driver is broken. Rule
number one in svn_delta.h says,
1. The producer may call open_directory, add_directory, open_file,
add_file at most once on any given directory entry. delete_entry may
be called at most once on any given directory entry and may later be
followed by add_directory or add_file on the same directory
entry. delete_entry may not be called on any directory entry after
open_directory, add_directory, open_file or add_file has been called
on that directory entry.
Is somebody calling add_file(mu), then delete_entry(mu)?
Steve
--
Stephen Butler | Software Developer
elego Software Solutions GmbH
Gustav-Meyer-Allee 25 | 13355 Berlin | Germany
fon: +49 30 2345 8696 | mobile: +49 163 25 45 015
fax: +49 30 2345 8695 | http://www.elegosoft.com
Geschäftsführer: Olaf Wagner | Sitz der Gesellschaft: Berlin
Amtsgericht Charlottenburg HRB 77719 | USt-IdNr: DE163214194
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2385846
Re: svn commit: r38896 - trunk/subversion/libsvn_client
Posted by Stephen Butler <sb...@elego.de>.
Quoting Stefan Sperling <st...@elego.de>:
> On Thu, Aug 20, 2009 at 02:55:04PM -0700, Stephen Butler wrote:
>> Author: sbutler
>> Date: Thu Aug 20 14:55:04 2009
>> New Revision: 38896
>>
>> Log:
>> Report duplicate tree conflict errors in just one place (follow-up
>> to r38872).
>>
>> * subversion/libsvn_client/merge.c
>> (tree_conflict_on_add): Remove redundant error handling.
>
>> + /* A merge may send two separate tree-conflicts if the merge
>> + replaces the item. This means merge will first set a tree-conflict
>> + with an incoming "delete", and then one with an incoming "add". */
>> + if (existing_conflict != NULL
>> + && existing_conflict->action == svn_wc_conflict_action_delete
>> + && conflict->action == svn_wc_conflict_action_add)
>
> Won't the above check also need similar treatment as done in
> r38841 and r38848 ?
Maybe we should check for (A,D) as well as (D,A).
It's weird that in repos_diff.c an item is either a tree conflict
*or* an add or a delete, etc. (in the svn_wc_notify_t_* enum).
In merge.c (in the conflict struct) the tree conflict status
is orthogonal to the action.
I guess we thought a tree conflict victim didn't need any other
notification, so it should stand alone. But since then we've
avoided skipping the victims, at least for update. It'd be nice
to fix the nofification to show what happened (e.g., 'R C mu').
Steve
--
Stephen Butler | Software Developer
elego Software Solutions GmbH
Gustav-Meyer-Allee 25 | 13355 Berlin | Germany
fon: +49 30 2345 8696 | mobile: +49 163 25 45 015
fax: +49 30 2345 8695 | http://www.elegosoft.com
Geschäftsführer: Olaf Wagner | Sitz der Gesellschaft: Berlin
Amtsgericht Charlottenburg HRB 77719 | USt-IdNr: DE163214194
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2385842