Julian Foad wrote:
> On Fri, 2008-10-31 at 20:39 -0700, neels_at_tigris.org wrote:
>> Author: neels
>> Date: Fri Oct 31 20:39:02 2008
>> New Revision: 33989
>>
>> Log:
>> Merge (lightweight) branch tc-merge-notify to trunk.
>> * subversion/include/svn_wc.h (svn_wc_diff_callbacks3_t): Add a svn_boolean_t
>> *TREE_CONFLICTED argument to all of the callbacks functions, which
>> returns whether the callback wants this node to be notified as
>> tree-conflicted. Returning TRUE in *TREE_CONFLICTED makes a repos
>> diff (and consequently a merge) skip any child nodes.
>
> This description would go better in the code comment. I added a code
> comment about this new flag but I didn't write about the "skipping"
> effect of the flag.
I'd say should *also* be in the code comment... It's quite a change,
skipping everything in some cases.
>
> I wrote that the function must set the flag to true or false - i.e. it
> must not leave it untouched - but that may cause a problem (see below)
> so I don't know if it was what you intended.
... s.b.
>
>> * subversion/libsvn_wc/diff.c
>> (file_diff, directory_elements_diff, report_wc_file_as_added,
>> report_wc_directory_as_added, delete_entry, close_directory,
>> close_file): Pass NULL for new *TREE_CONFLICTED parameter upon calling
>> the svn_wc_diff_callbacks3_t callback functions. Currently, handling
>> tree-conflicts notifications here is not needed, since there is no
>> code using it. Note that this variant of diff will also not skip
>> child nodes of a tree-conflicted item for the same reason.
>
> Re. not skipping: It would be better in the long term, of course, to
> implement the skipping here as well, to keep the behaviour consistent,
> even though that part of the behaviour is not being used by the current
> "svn" client.
I agree. ...I tend to leave these "implementation details" that don't affect
the API to after we branched 1.6.
>
>> * subversion/libsvn_client/repos_diff.c
>> (dir_baton, file_baton): Add two booleans TREE_CONFLICTED and SKIPPED.
>> (make_dir_baton, make_file_baton): Default TREE_CONFLICTED to FALSE.
>> Default SKIPPED to FALSE.
>> (delete_entry, add_directory, open_directory, close_file, close_directory):
>> Pass the tree-conflict state through to the notify_func, and change some
>> of the logic around the notify action to not show tree-conflicts as
>> skipped paths.
>
> Here, the logic is setting TREE_CONFLICTED to false in the baton and
> then passing a pointer to it to each of the callbacks. If more than one
> callback is called for the same path, won't the subsequent callbacks
> overwrite the value set by the first callback?
Hm, yes, that should probably be decoupled... (s.b.)
>
> Observation: In tree_conflict_tests 14 (XFail) a directory-delete is
> merged onto a branch where the directory exists and has a property
> difference. There is a notification " C" during the merge, but then no
> conflict status is recorded in the working copy.
>
> It looks like there might be an error in merge_file_deleted() and
> merge_dir_deleted(): they don't set *tree_conflicted=FALSE anywhere. In
> default cases, merge_file_deleted() doesn't set it at all,
> merge_dir_deleted() sets it TRUE, and the other callbacks set it FALSE.
Heh, I expected someone to stumble across that. I found it just yesterday,
was at the point of committing but saw that more work was necessary and went
to bed. So I'm on it.
I've also got some more fixes in the patch. Looks like I really wasn't
paying attention to that at the time. E.g., apparently *all* directory
deletes in a merge will currently result in a tree-conflict -- oops!
We should test for that, I mean false positives. And, I found out, we should
also test for props in the deep tree cases, i.e. add P, DP and DDP
directories that do prop changes and deletes.
And I also think we don't have add-on-top-of-add tests, do we?
And Tests that have two different node kinds conflicting. And...
Is the current code actually aware of the fact that the same path may be a
file on the one side and a dir on the other? (I mean in the conflict
description and reporting.)
~Neels
Received on 2008-11-04 01:55:55 CET