Re: [PATCH] New client diff callbacks

[Stefan Sperling <st...@elego.de>]

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?

> 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.

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. 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.

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?

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.
> 
> 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.
>   (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
> 
> ]]]

-- 
Stefan Sperling <st...@elego.de>                 Software Developer
elego Software Solutions GmbH                            HRB 77719
Gustav-Meyer-Allee 25, Gebaeude 12        Tel:  +49 30 23 45 86 96 
13355 Berlin                              Fax:  +49 30 23 45 86 95
http://www.elego.de                 Geschaeftsfuehrer: Olaf Wagner

Re: [PATCH] New client diff callbacks

[Julian Foad <ju...@btopenworld.com>]

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@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org