Stefan Sperling wrote:
> On Wed, Feb 27, 2008 at 05:12:42PM +0100, Stephen Butler wrote:
>
>>Hi Julian and other conflict-resolvers,
>>
>>Here is a diff that is useful for tree-conflict detection, but which
>>is not strictly part of the tree-conflict code. Can it be merged into
>>the Subversion trunk?
>
> I'd like to bump this long-sleeping thread.
>
> AFAIK the patch has not been reviewed yet.
> Would anyone volunteer reviewing it?
OK, I will.
>>We want to do two things that will rely on this patch.
>>
>>1. Stop an update/switch/merge operation if it tries to enter a
>>directory containing old, unresolved conflicts, including text or
>>property conflicts. This would resolve issue #2202. Requires a
>>callback for opening a directory.
Issue #2202 is about exceptions on Windows. I think you meant issue #2022 "Stop
doing conflict-producing operations on already conflicted files".
> This point is now invalid, since Mike Pilato pointed out that
> this check should rather be done in the initial working copy
> crawl preceding a commit.
Rather, "preceding an update/switch/merge operation", I assume. There is
already a check for conflicts before commit (which just needs to be extended to
check for tree conflicts as well as text and property conflicts), and I assume
that check is done in the crawl which locks the WC before commit. The new thing
we want is to check for conflicts before attempting an update/switch/merge
operation. If I understand correctly, the best place for doing that is in the
crawl which locks the WC before those operations, which is presumably very much
the same as the crawl before a commit.
> He said since we check for conflicts
> during that crawl anyway, we might as well adapt that code instead
> of adding open_dir() callbacks to the editor.
>
> However, Mike also said that open_dir() callbacks might be nice
> to have anyway. So we might still want to leave them in.
>
> I myself would in principle object to adding dead code, but if
> changing the editor interface really requires a big ceremony every
> time, we might as well give open_dir() callbacks a free piggyback
> ride into the tree and hope they get used by clients at some point.
> Any client API consumers around for comments?
>
>
>>2. Notify the user if (new) tree conflicts have been detected in a
>>directory. Requires a callback for closing a directory.
So, whenever "svn merge" completes making changes in one subdirectory, we want
it to report any tree conflicts that it created there (or at least an
indication of whether there were any), and this "close directory" callback is
the place to do that. Without this callback, there is no place where that
notification can be generated.
By "(new) tree conflicts" you mean ones that have just been created by the
currently executing merge. In fact, we expect that any conflicts found will be
new ones, because before starting the operation we intend to check for old
conflicts and bail out if there are any. Therefore we don't need to store any
explicit indication of whether a conflict is "new".
> This point is still valid.
>
> Since the tree-conflicts branch is already carrying a lot of changes,
> and there are a few more to come, it would be nice for us to put this
> helper functionality into trunk straight away. The farther the branches
> divert the more headaches we'll have to face when merging tree-conflicts
> into trunk. We already have a few libsvn_wc API changes in the tree-conflicts
> branch.
>
> But given that a different patch I submitted (which only changed white
> space!) was rejected because there is still backporting to 1.5 going on,
> I guess there will be objections committing the new diff callbacks to
> trunk straight away, right?
I don't expect there will be objections now.
> If so, I will have to add these callbacks to the tree-conflicts
> branch before they get into trunk (and I can live with that, it's
> not that big of an issue, really).
>
> Opinions?
>
>
>
>>For update/switch, appropriate callbacks already exist in the struct
>>svn_delta_editor_t.
>>
>>For merge, the code in libsvn_client implements a smaller set of
>>callbacks, declared in svn_wc_diff_callbacks2_t. This patch adds the
>>two missing callbacks.
Hmm. Can someone explain briefly why update/switch use a different set of
callbacks from diff/merge? (Not historically why, but what are the significant
differences and benefits.)
I guess it will become pretty obvious when I think about it, but it doesn't
seem obvious right away.
I don't need this in order to review the patch, of course.
>>Due to client code shared by merge and diff, adding the new callbacks
>>for the merge-specific code requires a slew of changes to the
>>diff-specific code in order to compile. If there's a better way,
>>please point me in the right direction.
>>
>>Regards,
>>Steve
>>
>>[[[
>>
>>Extend the diff-callbacks struct to handle the opening and closing of
>>a directory explicitly. For backward compatibility, name the new
>>struct svn_wc_diff_callbacks3_t.
>>
>>In libsvn_wc, create the new struct, adjust all relevant function args
>>and comments to track the new version, and add code to call the new
>>callbacks (dir_opened() and dir_closed()).
>>
>>In libsvn_client, add new callback implementations for diff and merge,
>>and track the version change.
>>
>>For merge, the new callbacks don't do anything yet, but we plan to use
>>them to improve conflict detection. The dir_opened() function will be
>>useful in blocking merge into a conflicted directory. The
>>dir_closed() function will be useful in reporting tree conflicts.
>>
>>For diff, the new callbacks exist for compatibility only.
>>
>>* subversion/include/svn_wc.h
>> (svn_wc_diff_callbacks3_t): New struct.
>> (svn_wc_diff_callbacks2_t): Moved comments to the new struct,
>> editing slightly to avoid repetition. Added deprecation comments.
>> (svn_wc_get_diff_editor5): New function.
>> (svn_wc_get_diff_editor4): Deprecated (edit to comment only).
>> (svn_wc_diff5): New function.
>> (svn_wc_diff4): Deprecated (edit to comment only).
>>
>>* subversion/libsvn_wc/diff.c
>> (edit_baton): Track diff-callbacks version in struct field.
>> (make_editor_baton,
>> svn_wc_get_diff_editor4): Track diff-callbacks version.
>> (file_changed,
>> file_added,
>> file_deleted,
>> dir_added,
>> dir_deleted,
>> dir_props_changed): Track diff-callbacks version in comments only.
>> (dir_opened,
>> dir_closed,
>> svn_wc_diff5): New functions.
>> (svn_wc_diff4): Deprecated (edit to comment only).
>> (callbacks2_wrapper): New struct.
>>
>>* subversion/libsvn_client/repos_diff.c
>> (edit_baton): Track diff-callbacks version bump in struct.
>> (close_directory): Move the call to get_path_access() so that it is
>> done unconditionally, because a valid adm_access is always needed
>> by the call to the new dir_closed() callback.
The essence of the change in (close_directory) is:
Call the new dir_closed() callback.
That's all you really need to say; moving the call to get_path_access() is just
a detail that's required in order to call dir_closed().
>> (svn_client__get_diff_editor): Track diff-callbacks version.
>>
>>* subversion/libsvn_client/client.h
>> (svn_client__get_diff_editor): Track diff-callbacks version.
>>
>>* subversion/libsvn_client/diff.c
>> (diff_cmd_baton,
>> diff_props_changed,
>> diff_file_changed,
>> diff_file_added,
>> diff_file_deleted_with_diff,
>> diff_file_deleted_no_diff,
>> diff_dir_added,
>> diff_dir_deleted: Track diff-callbacks version in comment only.
>> (diff_dir_opened,
>> diff_dir_closed): New functions.
>> (diff_wc_wc,
>> diff_repos_repos,
>> diff_repos_wc): Track diff-callbacks version in args; track new
>> public function names.
>> (svn_client_diff4): Track diff-callbacks version internally.
>> Include the new functions in the diff-callback initialization.
>>
>>* subversion/libsvn_client/diff.c
>> (merge_props_changed,
>> merge_file_changed,
>> merge_file_added,
>> merge_file_deleted_with_diff,
>> merge_file_deleted_no_diff,
>> merge_dir_added,
>> merge_dir_deleted: Track diff-callbacks version in comment only.
>> (merge_dir_opened,
>> merge_dir_closed): New functions.
>> (drive_merge_report_editor): Track diff-callbacks version in args
>>
>>]]]
> Index: subversion/include/svn_wc.h
[...]
> + * Common parameters:
[...]
> + * If @a state is non-NULL, set @a *state to the state of the item
> + * after the operation has been performed. (In practice, this is only
> + * useful with merge, not diff; diff callbacks will probably set
> + * @a *state to @c svn_wc_notify_state_unknown, since they do not change
> + * the state and therefore do not bother to know the state after the
> + * operation.)
This refers to an argument named "state" but the arguments seem to be named
"contentstate", "propstate" and "treestate".
> + /** The same as @c file_deleted in @c svn_wc_diff_callbacks3_t. */
Typo: one of the two lines like this should say "file_added" instead of
"file_deleted".
> Index: subversion/libsvn_wc/diff.c
[...]
> + struct callbacks2_wrapper_baton *b = apr_pcalloc(pool, sizeof(*b));
[...]
> + struct callbacks2_wrapper_baton *b = apr_pcalloc(pool, sizeof(*b));
These two don't need to initialise the memory to zero so "apr_palloc" is fine.
> Index: subversion/libsvn_client/repos_diff.c
[...]
> Index: subversion/libsvn_client/diff.c
[...]
> -/* A svn_wc_diff_callbacks2_t function. */
> +/* An svn_wc_diff_callbacks3_t function. */
> static svn_error_t *
> diff_dir_deleted(svn_wc_adm_access_t *adm_access,
> svn_wc_notify_state_t *state,
> const char *path,
> void *diff_baton)
> {
> + return SVN_NO_ERROR;
> +}
> +
> +/* An svn_wc_diff_callbacks3_t function. */
> +static svn_error_t *
> +diff_dir_opened(svn_wc_adm_access_t *adm_access,
> + const char *path,
> + svn_revnum_t rev,
> + void *diff_baton)
> +{
> + return SVN_NO_ERROR;
> +}
> +
> +/* An svn_wc_diff_callbacks3_t function. */
> +static svn_error_t *
> +diff_dir_closed(svn_wc_adm_access_t *adm_access,
> + svn_wc_notify_state_t *state,
> + const char *path,
> + void *diff_baton)
> +{
> if (state)
> *state = svn_wc_notify_state_unknown;
Here you seem to have (inadvertently?) changed the function diff_dir_deleted()
to do nothing, where previously it set *state to svn_wc_notify_state_unknown.
> Index: subversion/libsvn_client/merge.c
[...]
> +/* An svn_wc_diff_callbacks3_t function. */
> +static svn_error_t *
> +merge_dir_opened(svn_wc_adm_access_t *adm_access,
> + const char *path,
> + svn_revnum_t rev,
> + void *baton)
> +{
> + merge_cmd_baton_t *merge_b = baton;
No need for this unused variable. Same in the following function
merge_dir_closed().
I can't see any problems with the structure or logic of this patch.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-03-27 17:43:49 CET