Re: svn commit: r1160204 - WC-WC diff --summarize

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

Reviewing my own commit ...

On Mon, 2011-08-22 at 11:11 +0000, julianfoad@apache.org wrote:
> URL: http://svn.apache.org/viewvc?rev=1160204&view=rev
> Log:
> Extend 'svn diff --summarize' to support WC-WC diffs. Before this it only
> supported repo-repo diffs.
[...]
> * subversion/libsvn_wc/diff_local.c
>   (diff_status_callback): Call the dir_added, dir_deleted, dir_opened and
>     dir_closed callbacks at the appropriate times.
[...]
> Modified: subversion/trunk/subversion/libsvn_wc/diff_local.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/diff_local.c?rev=1160204&r1=1160203&r2=1160204&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_wc/diff_local.c (original)
> +++ subversion/trunk/subversion/libsvn_wc/diff_local.c Mon Aug 22 11:11:50 2011
> @@ -474,8 +474,29 @@ diff_status_callback(void *baton,
>            SVN_ERR(file_diff(eb, local_abspath, path, scratch_pool));
>          }
>      }
> -  else
> +  else  /* it's a directory */
>      {
> +      const char *path = svn_dirent_skip_ancestor(eb->anchor_abspath,
> +                                                  local_abspath);
> +
> +      /* Report the directory as deleted and/or opened or added. */
> +      if (status->node_status == svn_wc_status_deleted
> +          || status->node_status == svn_wc_status_replaced)
> +        SVN_ERR(eb->callbacks->dir_deleted(NULL, NULL, path,
> +                                           eb->callback_baton, scratch_pool));
> +
> +      if (status->node_status == svn_wc_status_added
> +          || status->node_status == svn_wc_status_replaced)
> +        SVN_ERR(eb->callbacks->dir_added(NULL, NULL, NULL, NULL,
> +                                         path, status->revision,
> +                                         path, status->revision /* ### ? */,
> +                                         eb->callback_baton, scratch_pool));
> +      else
> +        SVN_ERR(eb->callbacks->dir_opened(NULL, NULL, NULL,
> +                                          path, status->revision,
> +                                          eb->callback_baton, scratch_pool));
> +
> +      /* Report the prop change. */
>        /* ### This case should probably be extended for git-diff, but this
>               is what the old diff code provided */
>        if (status->node_status == svn_wc_status_deleted
> @@ -484,9 +505,6 @@ diff_status_callback(void *baton,
>          {
>            apr_array_header_t *propchanges;
>            apr_hash_t *baseprops;
> -          const char *path = svn_dirent_skip_ancestor(eb->anchor_abspath,
> -                                                      local_abspath);
> -
>  
>            SVN_ERR(svn_wc__internal_propdiff(&propchanges, &baseprops,
>                                              eb->db, local_abspath,
> @@ -498,6 +516,15 @@ diff_status_callback(void *baton,
>                                                     eb->callback_baton,
>                                                     scratch_pool));
>          }
> +
> +      /* Close the dir.
> +       * ### This should be done after all children have been processed, not
> +       *     yet.  The current Subversion-internal callers don't care. */

What's happening here is when we encounter a prop change on directory
'A/B', this 'status walk' callback reports it to the 'diff callbacks' as
follows:

  dir_opened('A/B')
  dir_prop_changed('A/B', ...)
  dir_closed('A/B')

and then maybe it will process a prop change on 'A/B/C' which it will
report as:

  dir_opened('A/B/C')
  dir_prop_changed('A/B/C', ...)
  dir_closed('A/B/C')

That's not a depth-first order, and it hasn't even called
dir_opened('A') at all unless 'A' had a prop change.  The 'diff
callbacks' documentation doesn't say exactly what the ordering
requirements are, but it would make sense to require something similar
to the delta editor.

So far, the 'diff --summarize' code is the only code that uses these
dir_opened/dir_closed callbacks, and it's perfectly happy with this.

But we need to decide what to do with this.

- Julian


> +      SVN_ERR(eb->callbacks->dir_closed(
> +                        NULL, NULL, NULL, path,
> +                        (status->node_status == svn_wc_status_added
> +                         || status->node_status == svn_wc_status_replaced),
> +                        eb->callback_baton, scratch_pool));
>      }
>    return SVN_NO_ERROR;
>  }