Philip -- this is great news & great progress, thanks for the update.
Comments on some of your points, and on the patch:
> changes are correctly represented in the output, the file *paths* are
> not. This makes it difficult to use the output as an input to
> patch. The new code also uses the empty file, but it also provides an
> additional label option to diff, something my earlier patch failed to
> do.
Yeah, we've encountered this problem before (in the `svnlook' program,
I believe). Glad you fixed it; I think maybe we can use a similar fix
in `svnlook', will have to investigate.
> The following patch is "incorect, incomplet and inCONsisteNt", there
> are bits that aren't finished, and cases it doesn't handle. It's here
> to find out what you think of the approach, and what I would need to
> do to get a patch accepted. It's currently implemented as a new
> 'rdiff' command because it always makes a repository connection even
> when the requested revision is the same as the working copy. Thus diff
> remains a 'local only' diff.
Ok for now, if it makes your life easier. However, I'm pretty sure
the consensus is that there be a single diff command, "svn diff". The
equivalent of "cvs rdiff" is simply to pass URLs for the arguments.
(Hmm, looking over the patch, you may want to do that reorg now, as
you've gone so far as to add a new `rdiff-cmd.c' file.)
> Things I know about: need to consider multiple pools, need to track
> new directories to handle the addition of files in new directories,
> need to pass command line options through the diff command baton, need
> to consider error handling, need to considered locking of the working
> copy, need some tests.
And add to that a log message (see guidelines in HACKING). You
probably were already aware of that, just making sure.
On to the patch. I made a lot of comments, as you were hoping, but
I'd like to say up front that it's very impressive. You've really got
the delta editor idiom down pat, & as a general approach to the
problem it seems quite solid.
Here we go:
> Index: svn/subversion/include/.svn/text-base/svn_wc.h
> ===================================================================
> --- svn/subversion/include/.svn/text-base/svn_wc.h Wed Oct 31 19:28:59 2001
> +++ svn/subversion/include/svn_wc.h Fri Nov 2 17:44:53 2001
> @@ -550,7 +550,23 @@
> void **edit_baton,
> apr_pool_t *pool);
>
> -
> +typedef svn_error_t *(*svn_wc_diff_cmd)(svn_stringbuf_t *path1,
> + svn_stringbuf_t *path2,
> + svn_stringbuf_t *label,
> + void *baton,
> + apr_pool_t *pool);
No need for it to take both baton and pool -- just baton is enough.
If it needs a pool, it should put one in the baton. :-)
Generally, function typedefs in Subversion end in `_t', so
`svn_wc_diff_cmd_t'.
The type needs a doc string, and any implementation of it should
mention (in its documentation) that it is an instance of
`svn_wc_diff_cmd_t'.
> +/*
> + * Return an editor for diffing a working copy against the repository.
> + *
> + */
> +svn_error_t *svn_wc_get_diff_editor (svn_stringbuf_t *anchor,
> + svn_stringbuf_t *target,
> + svn_wc_diff_cmd diff_cmd,
> + void *diff_cmd_baton,
> + svn_boolean_t recurse,
> + const svn_delta_edit_fns_t **editor,
> + void **edit_baton,
> + apr_pool_t *pool);
Need much more verbose doc string :-). Function documentation should
mention every parameter the function takes (see other functions in
svn_wc.h for examples, and see also the guidelines in the HACKING
file).
> /*
> * Set *WC_ROOT to TRUE if PATH represents a "working copy root",
> * FALSE otherwise. Use POOL for any intermediate allocations.
> Index: svn/subversion/include/.svn/text-base/svn_client.h
> ===================================================================
> --- svn/subversion/include/.svn/text-base/svn_client.h Wed Oct 31 19:28:58 2001
> +++ svn/subversion/include/svn_client.h Fri Nov 2 14:17:08 2001
> @@ -396,6 +396,17 @@
> svn_stringbuf_t **pristine_copy_path,
> apr_pool_t *pool);
>
> +/* Given a path in the working copy, compare the elements against the given
> + revision in the repository.
> +
> + TODO: Pass in time as well as revision.
> +*/
> +svn_error_t *svn_client_diff (svn_stringbuf_t *path,
> + svn_client_auth_baton_t *auth_baton,
> + svn_revnum_t revision,
> + svn_boolean_t recurse,
> + apr_pool_t *pool);
> +
Same thing about the doc string.
By the way, we seem to have settled into a convention of preceding
"TODO" comments with the string "###", which catches the eye and is
also a known, consistent string to grep for when reviewing. You may
wish to use "### TODO: ", to make it easier for others to find/notice
your todo items.
(I should really put something about that in HACKING, hmmm...)
I have this sneaking feeling that you're soon going to want an
`apr_array_header_t *' of paths, not just a single path, but no need
to make that change now unless you want to. It'll be obvious when the
time comes.
> /* Recursively cleanup a working copy directory DIR, finishing any
> incomplete operations, removing lockfiles, etc. */
> Index: svn/subversion/libsvn_wc/.svn/empty-file
> ===================================================================
> --- svn/subversion/libsvn_wc/.svn/empty-file Wed Oct 31 19:31:45 2001
> +++ svn/subversion/libsvn_wc/diff.c Fri Nov 2 17:49:05 2001
> @@ -0,0 +1,458 @@
> +/*
> + * diff.c -- The diff editor for comparing the working copy against the
> + * repository. This implemtation is based on the update editor
> + * svn_wc_get_update_editor.
> + *
> + * ====================================================================
> + * Copyright (c) 2001 Philip Martin. All rights reserved.
When it comes time to include this in Subversion, will you be willing
to assign copyright to CollabNet? The terms of the copyright are the
same, of course, so it's not as if anyone (including you) will lose
their right to modify and redistribute the code. But CollabNet's
lawyers are more comfortable if we can consolidate copyright into one
holder.
> + *
> + * This software is licensed as described in the file COPYING, which
> + * you should have received as part of this distribution. The terms
> + * are also available at http://subversion.tigris.org/license-1.html.
> + * If newer versions of this license are posted there, you may use a
> + * newer version instead, at your option.
> + *
> + * This software consists of voluntary contributions made by many
> + * individuals. For exact contribution history, see the revision
> + * history and logs, available at http://subversion.tigris.org/.
> + * ====================================================================
> + */
> +
> +#include <apr_hash.h>
> +#include "wc.h"
> +#include "svn_pools.h"
> +
> +#define printf(...)
Huh? Some reason not to #include stdio.h? :-)
> +struct edit_baton {
> + svn_stringbuf_t *anchor;
> + svn_wc_diff_cmd diff_cmd;
> + void *diff_cmd_baton;
> + svn_boolean_t recurse;
> + apr_pool_t *pool;
> +};
> +
> +struct dir_baton {
> + svn_stringbuf_t *path;
> + apr_hash_t *entries;
> + struct dir_baton *dir_baton;
> + struct edit_baton *edit_baton;
> +};
> +
> +struct file_baton {
> + svn_boolean_t added;
> + apr_file_t *original_file;
> + apr_file_t *temp_file;
> + svn_txdelta_window_handler_t apply_handler;
> + void *apply_baton;
> + svn_stringbuf_t *path;
> + struct dir_baton *dir_baton;
> + struct edit_baton *edit_baton;
> +};
Documentation. :-)
Specifically, document the fields in these batons. For example, I
would never have guessed that the `entries' field in `dir_baton' holds
files already diff'ed -- I can tentatively deduce it from reading the
code below, but a comment here would have allowed me to know it sooner
and with more certainty.
> +static struct dir_baton*
> +make_dir_baton (svn_stringbuf_t *name,
> + struct dir_baton *parent_baton,
> + struct edit_baton *edit_baton,
> + apr_pool_t *pool)
> +{
> + struct dir_baton *dir_baton = apr_pcalloc (pool, sizeof (*dir_baton));
> +
> + dir_baton->dir_baton = parent_baton;
> + dir_baton->edit_baton = edit_baton;
> +
> + dir_baton->entries = apr_hash_make (dir_baton->edit_baton->pool);
> +
> + if (parent_baton)
> + dir_baton->path = svn_stringbuf_dup (parent_baton->path, pool);
> + else
> + dir_baton->path = svn_stringbuf_dup (edit_baton->anchor, pool);
> +
> + if (name)
> + svn_path_add_component (dir_baton->path, name, svn_path_local_style);
> +
> + return dir_baton;
> +}
> +
> +static struct file_baton*
> +make_file_baton (svn_stringbuf_t *name,
> + struct dir_baton *parent_baton)
> +{
> + struct file_baton *file_baton = apr_pcalloc (parent_baton->edit_baton->pool,
> + sizeof (*file_baton));
> +
> + file_baton->dir_baton = parent_baton;
> + file_baton->edit_baton = parent_baton->edit_baton;
> +
> + file_baton->path = svn_stringbuf_dup (parent_baton->path,
> + parent_baton->edit_baton->pool);
> + svn_path_add_component (file_baton->path, name, svn_path_local_style);
> +
> + return file_baton;
> +}
Minor stylistic nit: on the return types of functions like these, we
generally put a space before the "*" (as you did below with the
"svn_error_t *").
> +
> +static svn_error_t *
> +directory_elements_diff (struct dir_baton *dir_baton)
> +{
> + apr_hash_t *entries;
> + apr_hash_index_t *hi;
> + svn_boolean_t modified;
> + svn_wc_diff_cmd diff_cmd = dir_baton->edit_baton->diff_cmd;
> + svn_stringbuf_t *pristine_copy;
> + svn_stringbuf_t *name;
> + struct dir_baton *subdir_baton;
> +
> + SVN_ERR (svn_wc_entries_read (&entries, dir_baton->path,
> + dir_baton->edit_baton->pool));
> +
> + for (hi = apr_hash_first (dir_baton->edit_baton->pool, entries); hi;
> + hi = apr_hash_next (hi))
> + {
> + svn_wc_entry_t *entry;
> + const char *key;
> + svn_stringbuf_t *path;
> +
> + apr_hash_this(hi, (const void **)&key, NULL, (void **)&entry);
> +
> + /* skip entry for the directory itself */
> + if (strcmp (key, SVN_WC_ENTRY_THIS_DIR) == 0)
> + continue;
> +
> + path = svn_stringbuf_dup (dir_baton->path, dir_baton->edit_baton->pool);
> + svn_path_add_component_nts (path, key, svn_path_local_style);
> +
> + /* skip entry if it is in the list of entries already diff'd */
> + if (apr_hash_get (dir_baton->entries, path->data, path->len))
> + continue;
> +
> + switch (entry->kind)
> + {
> + case svn_node_file:
> + SVN_ERR (svn_wc_text_modified_p (&modified, path,
> + dir_baton->edit_baton->pool));
> + if (modified)
> + {
> + if (entry->schedule == svn_wc_schedule_add)
> + pristine_copy =
> + svn_wc__empty_file_path (path, dir_baton->edit_baton->pool);
> + else
> + pristine_copy =
> + svn_wc__text_base_path (path, FALSE,
> + dir_baton->edit_baton->pool);
We've been putting the "=" signs on the same line as the rvalue.
> + SVN_ERR (diff_cmd (pristine_copy, path, NULL,
> + dir_baton->edit_baton->diff_cmd_baton,
> + dir_baton->edit_baton->pool));
> + }
> + break;
> + case svn_node_dir:
> + svn_path_split (path, NULL, &name, svn_path_local_style,
> + dir_baton->edit_baton->pool);
> + subdir_baton = make_dir_baton (name, dir_baton,
> + dir_baton->edit_baton,
> + dir_baton->edit_baton->pool);
> +
> + SVN_ERR (directory_elements_diff (subdir_baton));
> + break;
> + default:
> + break;
> + }
> + }
> +
> + return SVN_NO_ERROR;
> +}
> +
> +static svn_error_t *
> +set_target_revision (void *edit_baton,
> + svn_revnum_t target_revision)
> +{
> + printf ("set_target_revision: %d\n", target_revision);
> + return SVN_NO_ERROR;
> +}
>
> +static svn_error_t *
> +replace_root (void *edit_baton,
> + svn_revnum_t base_revision,
> + void **root_baton)
> +{
> + struct edit_baton *eb = edit_baton;
> + struct dir_baton *b;
> +
> + b = make_dir_baton (NULL, NULL, eb, eb->pool);
> + *root_baton = b;
> +
> + printf ("replace_root: %s %d\n", b->path->data, base_revision);
> + return SVN_NO_ERROR;
> +}
> +
> +static svn_error_t *
> +delete_entry (svn_stringbuf_t *name,
> + void *parent_baton)
> +{
> + printf ("delete_entry: %s\n", name->data);
> + return SVN_NO_ERROR;
> +}
> +
> +static svn_error_t *
> +add_directory (svn_stringbuf_t *name,
> + void *parent_baton,
> + svn_stringbuf_t *copyfrom_path,
> + svn_revnum_t copyfrom_revision,
> + void **child_baton)
> +{
> + struct dir_baton *pb = parent_baton;
> + struct dir_baton *b;
> +
> + b = make_dir_baton (name, pb, pb->edit_baton, pb->edit_baton->pool);
> + *child_baton = b;
> +
> + printf ("add_directory: %s %s %d\n",
> + b->path->data,
> + (copyfrom_path ? copyfrom_path->data : ""), copyfrom_revision);
> + return SVN_NO_ERROR;
> +}
> +
> +static svn_error_t *
> +replace_directory (svn_stringbuf_t *name,
> + void *parent_baton,
> + svn_revnum_t base_revision,
> + void **child_baton)
> +{
> + struct dir_baton *pb = parent_baton;
> + struct dir_baton *b;
> +
> + b = make_dir_baton (name, pb, pb->edit_baton, pb->edit_baton->pool);
> + *child_baton = b;
> +
> + printf ("replace_directory: %s %d\n", b->path->data, base_revision);
> + return SVN_NO_ERROR;
> +}
> +
> +static svn_error_t *
> +change_dir_prop (void *dir_baton,
> + svn_stringbuf_t *name,
> + svn_stringbuf_t *value)
> +{
> + printf ("change_dir_prop: %s %s\n", name->data, value->data);
> + return SVN_NO_ERROR;
> +}
> +
> +static svn_error_t *
> +close_directory (void *dir_baton)
> +{
> + struct dir_baton *b = dir_baton;
> +
> + SVN_ERR (directory_elements_diff (dir_baton));
> +
> + if (b->dir_baton)
> + apr_hash_set (b->dir_baton->entries, b->path->data, b->path->len,
> + (void*)TRUE);
> +
> + printf ("close_directory:\n");
> + return SVN_NO_ERROR;
> +}
> +
> +static svn_error_t *
> +add_file (svn_stringbuf_t *name,
> + void *parent_baton,
> + svn_stringbuf_t *copyfrom_path,
> + svn_revnum_t copyfrom_revision,
> + void **file_baton)
> +{
> + struct dir_baton *pb = parent_baton;
> + struct file_baton *b;
> + svn_stringbuf_t *temp_loc;
> +
> + b = make_file_baton (name, pb);
> + *file_baton = b;
> + b->added = TRUE;
> +
> + SVN_ERR (svn_wc__open_empty_file (&b->original_file, b->path,
> + b->edit_baton->pool));
> +
> + /* This will be the new version in the temporary directory */
> + SVN_ERR (svn_wc__open_text_base (&b->temp_file, b->path,
> + (APR_WRITE | APR_TRUNCATE | APR_CREATE),
> + b->edit_baton->pool));
> +
> + svn_txdelta_apply (svn_stream_from_aprfile (b->original_file,
> + b->edit_baton->pool),
> + svn_stream_from_aprfile (b->temp_file,
> + b->edit_baton->pool),
> + b->edit_baton->pool,
> + &b->apply_handler, &b->apply_baton);
> +
> + printf ("add_file: %s %s %d\n", b->path->data,
> + (copyfrom_path ? copyfrom_path->data : ""), copyfrom_revision);
> + return SVN_NO_ERROR;
> +}
> +
> +static svn_error_t *
> +replace_file (svn_stringbuf_t *name,
> + void *parent_baton,
> + svn_revnum_t base_revision,
> + void **file_baton)
> +{
> + struct dir_baton *pb = parent_baton;
> + struct file_baton *b;
> +
> + b = make_file_baton (name, pb);
> + *file_baton = b;
> + b->added = FALSE;
> +
> + /* The current text-base is the starting point for the new version */
> + SVN_ERR (svn_wc__open_text_base (&b->original_file, b->path,
> + APR_READ, b->edit_baton->pool));
> +
> + /* This will be the new version in the temporary directory */
> + SVN_ERR (svn_wc__open_text_base (&b->temp_file, b->path,
> + (APR_WRITE | APR_TRUNCATE | APR_CREATE),
> + b->edit_baton->pool));
> +
> + svn_txdelta_apply (svn_stream_from_aprfile (b->original_file,
> + b->edit_baton->pool),
> + svn_stream_from_aprfile (b->temp_file,
> + b->edit_baton->pool),
> + b->edit_baton->pool,
> + &b->apply_handler, &b->apply_baton);
> +
> + printf ("replace_file: %s %d\n", b->path->data, base_revision);
> + return SVN_NO_ERROR;
> +}
I'm not sure I understand what is going on here, but probably would if
I studied the patch longer. Wouldn't mind a comment, though; it's
highly unusual to be calling svn_txdelta_apply() directly in an
editor's replace_file() function -- it leaves one wondering what's
left for the editor's apply_textdelta() & friends? :-)
By the way, if you want to make a special subdir in .svn/ for diff's
temporary files, we could do that. Better to use existing tmp space
*if* it is safe & crash-proof to do so, of course; I haven't thought
as much about it as you, so can't say which would be better, just
wanted to sound out the possibilities...
> +static svn_error_t *
> +window_handler (svn_txdelta_window_t *window,
> + void *window_baton)
> +{
> + struct file_baton *b = window_baton;
> +
> + SVN_ERR (b->apply_handler (window, b->apply_baton));
> +
> + printf ("window_handler:\n");
> + return SVN_NO_ERROR;
> +}
> +
> +static svn_error_t *
> +apply_textdelta (void *file_baton,
> + svn_txdelta_window_handler_t *handler,
> + void **handler_baton)
> +{
> + printf ("apply_textdelta:\n");
> + *handler = window_handler;
> + *handler_baton = file_baton;
> + return SVN_NO_ERROR;
> +}
> +
> +static svn_error_t *
> +change_file_prop (void *file_baton,
> + svn_stringbuf_t *name,
> + svn_stringbuf_t *value)
> +{
> + printf ("change_file_prop: %s %s\n", name->data, value->data);
> + return SVN_NO_ERROR;
> +}
> +
> +static svn_error_t *
> +close_file (void *file_baton)
> +{
> + struct file_baton *b = file_baton;
> + svn_wc_diff_cmd diff_cmd = b->edit_baton->diff_cmd;
> + svn_stringbuf_t *temp_file_path =
> + svn_wc__text_base_path (b->path, TRUE, b->edit_baton->pool);
> +
> + SVN_ERR (svn_wc__close_text_base (b->original_file, b->path, 0,
> + b->edit_baton->pool));
> + SVN_ERR (svn_wc__close_text_base (b->temp_file, b->path, 0,
> + b->edit_baton->pool));
> +
> + if (b->added)
> + {
> + /* TODO: need an empty file at the correct place in the hierarchy to
> + get an correct diff. b->path does not exist in the working
> + copy. */
> + SVN_ERR (diff_cmd (svn_wc__empty_file_path (b->path, b->edit_baton->pool),
> + temp_file_path, b->path,
> + b->edit_baton->diff_cmd_baton,
> + b->edit_baton->pool));
> +
> + }
> + else
> + {
> + SVN_ERR (diff_cmd (b->path, temp_file_path, NULL,
> + b->edit_baton->diff_cmd_baton,
> + b->edit_baton->pool));
> + }
> +
> + apr_hash_set (b->dir_baton->entries, b->path->data, b->path->len,
> + (void*)TRUE);
> +
> + apr_file_remove (temp_file_path->data, b->edit_baton->pool);
> +
> + printf ("close_file:\n");
> + return SVN_NO_ERROR;
> +}
> +
> +static svn_error_t *
> +close_edit (void *edit_baton)
> +{
> + printf ("close_edit:\n");
> + return SVN_NO_ERROR;
> +}
> +
> +static svn_error_t *
> +abort_edit (void *edit_baton)
> +{
> + printf ("abort_edit:\n");
> + return SVN_NO_ERROR;
> +}
> +
> +svn_error_t *
> +svn_wc_get_diff_editor (svn_stringbuf_t *anchor,
> + svn_stringbuf_t *target,
> + svn_wc_diff_cmd diff_cmd,
> + void *diff_cmd_baton,
> + svn_boolean_t recurse,
> + const svn_delta_edit_fns_t **editor,
> + void **edit_baton,
> + apr_pool_t *pool)
> +{
> + apr_pool_t *subpool = svn_pool_create (pool);
> + svn_delta_edit_fns_t *tree_editor = svn_delta_default_editor (pool);
> + struct edit_baton *eb;
> +
> + eb = apr_palloc (subpool, sizeof (*eb));
> + eb->anchor = anchor;
> + eb->diff_cmd = diff_cmd;
> + eb->diff_cmd_baton = diff_cmd_baton;
> + eb->recurse = recurse;
> + eb->pool = subpool;
> +
> + /* I know I don't need to replace all the functions, but I wanted to see
> + when they were called :-) */
> + tree_editor->set_target_revision = set_target_revision;
> + tree_editor->replace_root = replace_root;
> + tree_editor->delete_entry = delete_entry;
> + tree_editor->add_directory = add_directory;
> + tree_editor->replace_directory = replace_directory;
> + tree_editor->change_dir_prop = change_dir_prop;
> + tree_editor->close_directory = close_directory;
> + tree_editor->add_file = add_file;
> + tree_editor->replace_file = replace_file;
> + tree_editor->apply_textdelta = apply_textdelta;
> + tree_editor->change_file_prop = change_file_prop;
> + tree_editor->close_file = close_file;
> + tree_editor->close_edit = close_edit;
> + tree_editor->abort_edit = abort_edit;
> +
> + *edit_baton = eb;
> + *editor = tree_editor;
> +
> + return SVN_NO_ERROR;
> +}
> +
Yup, got that editor idiom down like a native language. :-)
> +
> +/* ----------------------------------------------------------------
> + * local variables:
> + * eval: (load-file "../svn-dev.el")
> + * end:
> + */
> Index: svn/subversion/libsvn_wc/.svn/text-base/wc.h
> ===================================================================
> --- svn/subversion/libsvn_wc/.svn/text-base/wc.h Wed Oct 31 19:31:48 2001
> +++ svn/subversion/libsvn_wc/wc.h Fri Nov 2 12:30:48 2001
> @@ -100,6 +100,7 @@
> #define SVN_WC__ADM_LOG "log"
> #define SVN_WC__ADM_KILLME "KILLME"
> #define SVN_WC__ADM_AUTH_DIR "auth"
> +#define SVN_WC__ADM_EMPTY_FILE "empty-file"
>
>
> /* The basename of the ".prej" file, if a directory ever has property
> @@ -110,6 +111,10 @@
> /* Return a string containing the admin subdir name. */
> svn_stringbuf_t *svn_wc__adm_subdir (apr_pool_t *pool);
>
> +/* Return a string containing the admin empty file. */
> +svn_stringbuf_t *svn_wc__empty_file_path (const svn_stringbuf_t *path,
> + apr_pool_t *pool);
> +
>
> /* Return a path to something in PATH's administrative area.
> * Return path to the thing in the tmp area if TMP is non-zero.
> @@ -182,6 +187,18 @@
> svn_error_t *svn_wc__remove_adm_file (svn_stringbuf_t *path,
> apr_pool_t *pool,
> ...);
> +
> +/* Open *readonly* the empty file in the in adm area of PATH */
> +svn_error_t *svn_wc__open_empty_file (apr_file_t **handle,
> + svn_stringbuf_t *path,
> + apr_pool_t *pool);
> +
> +/* Close the empty file in the adm area of PATH
> + * FP was obtain from svn_wc__open_empty_file().
> + */
> +svn_error_t *svn_wc__close_empty_file (apr_file_t *fp,
> + svn_stringbuf_t *path,
> + apr_pool_t *pool);
I see a prototype for svn_wc__close_empty_file(), and I see its
definition, but I don't see any calls to it. Did you have a use in
mind? If not, is there any reason the empty file needs its own
dedicated close function?
> /* Open the text-base for FILE.
> * FILE can be any kind of path ending with a filename.
> Index: svn/subversion/libsvn_wc/.svn/text-base/adm_files.c
> ===================================================================
> --- svn/subversion/libsvn_wc/.svn/text-base/adm_files.c Wed Oct 31 19:31:52 2001
> +++ svn/subversion/libsvn_wc/adm_files.c Fri Nov 2 12:30:29 2001
> @@ -376,6 +376,16 @@
> return thing_path (path, SVN_WC__ADM_TEXT_BASE, tmp, pool);
> }
>
> +svn_stringbuf_t *
> +svn_wc__empty_file_path (const svn_stringbuf_t *path,
> + apr_pool_t *pool)
> +{
> + svn_stringbuf_t *empty_file_path = svn_stringbuf_dup (path, pool);
> + svn_path_remove_component (empty_file_path, svn_path_local_style);
> + extend_with_adm_name (empty_file_path, 0, pool, SVN_WC__ADM_EMPTY_FILE, NULL);
> + return empty_file_path;
> +}
> +
>
> static svn_error_t *
> prop_path_internal (svn_stringbuf_t **prop_path,
> @@ -705,6 +715,27 @@
> return close_adm_file (fp, (svn_stringbuf_t *) path, sync, pool, fname, NULL);
> }
>
> +svn_error_t *
> +svn_wc__open_empty_file (apr_file_t **handle,
> + svn_stringbuf_t *path,
> + apr_pool_t *pool)
> +{
> + svn_stringbuf_t *newpath, *basename;
> + svn_path_split (path, &newpath, &basename, svn_path_local_style, pool);
> + return open_adm_file (handle, newpath, APR_READ, pool,
> + SVN_WC__ADM_EMPTY_FILE, NULL);
> +}
> +
> +svn_error_t *
> +svn_wc__close_empty_file (apr_file_t *fp,
> + svn_stringbuf_t *path,
> + apr_pool_t *pool)
> +{
> + svn_stringbuf_t *newpath, *basename;
> + svn_path_split (path, &newpath, &basename, svn_path_local_style, pool);
> + return close_adm_file (fp, newpath, 0, pool,
> + SVN_WC__ADM_EMPTY_FILE, NULL);
> +}
>
> svn_error_t *
> svn_wc__open_text_base (apr_file_t **handle,
> @@ -1180,6 +1211,10 @@
>
> /* SVN_WC__ADM_ENTRIES */
> SVN_ERR (svn_wc__entries_init (path, ancestor_path, pool));
> +
> + /* SVN_WC__ADM_EMPTY_FILE */
> + SVN_ERR (svn_wc__make_adm_thing (path, SVN_WC__ADM_EMPTY_FILE, svn_node_file,
> + APR_UREAD, 0, pool));
>
> /* THIS FILE MUST BE CREATED LAST:
> After this exists, the dir is considered complete. */
> Index: svn/subversion/libsvn_client/.svn/text-base/diff.c
> ===================================================================
> --- svn/subversion/libsvn_client/.svn/text-base/diff.c Wed Oct 31 19:31:31 2001
> +++ svn/subversion/libsvn_client/diff.c Fri Nov 2 17:47:56 2001
> @@ -1,5 +1,5 @@
> /*
> - * diff.c: return two temporary file paths that can be diffed
> + * diff.c: Compare working copy with text-base or repository.
> *
> * ====================================================================
> * Copyright (c) 2000-2001 CollabNet. All rights reserved.
> @@ -33,7 +33,72 @@
> #include "svn_path.h"
> #include "svn_test.h"
> #include "svn_io.h"
> +#include "svn_pools.h"
> +#include "client.h"
> +#include "svn_private_config.h" /* for SVN_CLIENT_DIFF */
> +#include <assert.h>
> +
> +static svn_error_t *
> +svn_client_diff_cmd (svn_stringbuf_t *path1,
> + svn_stringbuf_t *path2,
> + svn_stringbuf_t *label,
> + void *baton,
> + apr_pool_t *pool)
Non-public functions should use two underscores, as in
"svn_client__diff_cmd". The relevant bit from HACKING reads:
* Signify internal variables by two underscores after the prefix.
That is, when a symbol must (for technical reasons) reside in the
global namespace despite not being part of a published interface,
then use two underscores following the module prefix. For
example:
svn_fs_get_rev_prop () /* Part of published API. */
svn_fs__parse_props () /* For internal use only. */
> +{
> + apr_status_t status;
> + const char **args;
> + int i = 0;
> + int nargs = 5;
> + apr_file_t *outhandle = NULL;
> +
> + /* Get an apr_file_t representing stdout, which is where we'll have
> + the diff program print to. */
> + status = apr_file_open_stdout (&outhandle, pool);
> + if (status)
> + return svn_error_create (status, 0, NULL, pool,
> + "error: can't open handle to stdout");
> +
> + /* Execute local diff command on these two paths, print to stdout. */
> +
> + if (label)
> + nargs += 2;
> + args = apr_palloc(pool, nargs*sizeof(char*));
> +
> + args[i++] = SVN_CLIENT_DIFF; /* the autoconfiscated system diff program */
> + args[i++] = "-u";
> + if (label)
> + {
> + args[i++] = "-L";
> + args[i++] = label->data;
> + }
> + args[i++] = path1->data;
> + args[i++] = path2->data;
> + args[i++] = NULL;
> + assert (i==nargs);
> +
> + /* todo: This printf is NOT "my final answer" -- placeholder for
> + real work to be done. */
Heh, ok, gotcha. :-)
> + apr_file_printf (outhandle, "Index: %s\n", path1->data);
> + apr_file_printf (outhandle, "===================================================================\n");
> +
> + SVN_ERR(svn_io_run_cmd (".", SVN_CLIENT_DIFF, args, NULL, NULL,
> + NULL, outhandle, NULL, pool));
> +
> + /* TODO: Handle exit code == 2 (i.e. errors with diff) here */
> +
> + /* TODO: someday we'll need to worry about two things here:
>
> + 1. svn_client_file_diff may be returning a file from RA instead
> + of the WC's text-base. If this is so, it will need to provide a
> + "clean up" routine to remove the temporary file created by RA.
> +
> + 2. we're going to need to write a diff plug-in mechanism that
> + makes use of the two paths, instead of just blindly running
> + SVN_CLIENT_DIFF.
> + */
> +
> + return SVN_NO_ERROR;
> +}
>
>
>
> @@ -60,9 +125,59 @@
> }
>
>
> +svn_error_t *
> +svn_client_diff (svn_stringbuf_t *path,
> + svn_client_auth_baton_t *auth_baton,
> + svn_revnum_t revision,
> + svn_boolean_t recurse,
> + apr_pool_t *pool)
> +{
> + svn_stringbuf_t *anchor, *target;
> + svn_wc_entry_t *entry;
> + svn_stringbuf_t *URL;
> + void *ra_baton, *session, *cb_baton;
> + svn_ra_plugin_t *ra_lib;
> + svn_ra_callbacks_t *ra_callbacks;
> + const svn_ra_reporter_t *reporter;
> + void *report_baton;
> + const svn_delta_edit_fns_t *diff_editor;
> + void *diff_edit_baton;
> +
> + SVN_ERR (svn_wc_get_actual_target (path, &anchor, &target, pool));
> + SVN_ERR (svn_wc_entry (&entry, anchor, pool));
> + URL = svn_stringbuf_create (entry->ancestor->data, pool);
> +
> + SVN_ERR (svn_ra_init_ra_libs (&ra_baton, pool));
> + SVN_ERR (svn_ra_get_ra_library (&ra_lib, ra_baton, URL->data, pool));
> +
> + SVN_ERR (svn_client__get_ra_callbacks (&ra_callbacks, &cb_baton,
> + auth_baton,
> + anchor,
> + TRUE,
> + TRUE,
> + pool));
> + SVN_ERR (ra_lib->open (&session, URL, ra_callbacks, cb_baton, pool));
> +
> + SVN_ERR (svn_wc_get_diff_editor (anchor, target,
> + svn_client_diff_cmd, NULL,
> + recurse,
> + &diff_editor, &diff_edit_baton,
> + pool));
> +
> + SVN_ERR (ra_lib->do_update (session,
> + &reporter, &report_baton,
> + revision,
> + target,
> + recurse,
> + diff_editor, diff_edit_baton));
>
> + SVN_ERR (svn_wc_crawl_revisions (path, reporter, report_baton,
> + FALSE, FALSE, recurse, pool));
>
> + SVN_ERR (ra_lib->close (session));
>
> + return SVN_NO_ERROR;
> +}
>
>
>
> Index: svn/subversion/clients/cmdline/.svn/text-base/cl.h
> ===================================================================
> --- svn/subversion/clients/cmdline/.svn/text-base/cl.h Wed Oct 31 19:30:46 2001
> +++ svn/subversion/clients/cmdline/cl.h Fri Nov 2 00:34:54 2001
> @@ -151,6 +151,7 @@
> svn_cl__revert,
> svn_cl__status,
> svn_cl__diff,
> + svn_cl__rdiff,
> svn_cl__update;
Yup; assuming all this rdiff stuff will be part of `svn_cl__diff'
someday.
> Index: svn/subversion/clients/cmdline/.svn/text-base/main.c
> ===================================================================
> --- svn/subversion/clients/cmdline/.svn/text-base/main.c Wed Oct 31 19:30:48 2001
> +++ svn/subversion/clients/cmdline/main.c Fri Nov 2 00:34:24 2001
> @@ -134,6 +134,11 @@
> { "stat", TRUE, NULL, NULL },
> { "st", TRUE, NULL, NULL },
>
> + { "rdiff", FALSE, svn_cl__rdiff,
> + "Display changes between the working copy and the repository\n"
> + "as contextual diffs.\n"
> + "usage: rdiff [-r REV] [TARGETS]\n" },
> +
> { "diff", FALSE, svn_cl__diff,
> "Display local file changes as contextual diffs.\n"
> "usage: diff [TARGETS]\n" },
> Index: svn/subversion/clients/cmdline/.svn/empty-file
> ===================================================================
> --- svn/subversion/clients/cmdline/.svn/empty-file Wed Oct 31 19:30:42 2001
> +++ svn/subversion/clients/cmdline/rdiff-cmd.c Fri Nov 2 17:31:35 2001
> @@ -0,0 +1,66 @@
> +/*
> + * rdiff-cmd.c -- Compare the working copy against the repository. This
> + * is based on the update command.
> + *
> + * ====================================================================
> + * Copyright (c) 2001 Philip Martin. All rights reserved.
> + *
> + * This software is licensed as described in the file COPYING, which
> + * you should have received as part of this distribution. The terms
> + * are also available at http://subversion.tigris.org/license-1.html.
> + * If newer versions of this license are posted there, you may use a
> + * newer version instead, at your option.
> + *
> + * This software consists of voluntary contributions made by many
> + * individuals. For exact contribution history, see the revision
> + * history and logs, available at http://subversion.tigris.org/.
> + * ====================================================================
> + */
> +
> +#include "svn_path.h"
> +#include "svn_wc.h"
> +#include "svn_pools.h"
> +#include "cl.h"
> +
> +
> +svn_error_t *
> +svn_cl__rdiff (apr_getopt_t *os,
> + svn_cl__opt_state_t *opt_state,
> + apr_pool_t *pool)
> +{
> + apr_array_header_t *targets;
> + apr_array_header_t *condensed_targets;
> + svn_client_auth_baton_t *auth_baton;
> + int i;
> +
> + targets = svn_cl__args_to_target_array (os, pool);
> + svn_cl__push_implicit_dot_target (targets, pool);
> + SVN_ERR (svn_path_remove_redundancies (&condensed_targets,
> + targets,
> + svn_path_local_style,
> + pool));
> + auth_baton = svn_cl__make_auth_baton (opt_state, pool);
> +
> + for (i = 0; i < condensed_targets->nelts; ++i)
> + {
> + svn_stringbuf_t *target =
> + ((svn_stringbuf_t **) (condensed_targets->elts))[i];
> + svn_stringbuf_t *parent_dir, *entry;
> +
> + SVN_ERR (svn_wc_get_actual_target (target, &parent_dir, &entry, pool));
> +
> + SVN_ERR (svn_client_diff (target,
> + auth_baton,
> + opt_state->start_revision,
> + TRUE,
> + pool));
> + }
> +
> + return SVN_NO_ERROR;
> +}
> +
> +/* ----------------------------------------------------------------
> + * local variables:
> + * eval: (load-file "../../svn-dev.el")
> + * end:
> + */
Nice work, looking forward to the next iteration...
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Oct 21 14:36:47 2006