You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by David Glasser <gl...@davidglasser.net> on 2007/09/28 17:56:15 UTC
Re: svn commit: r26828 - in branches/svnpatch-diff/subversion: include libsvn_client libsvn_wc svn tests/cmdline
On 9/28/07, cacknin@tigris.org <ca...@tigris.org> wrote:
> Author: cacknin
> Date: Fri Sep 28 07:03:42 2007
> New Revision: 26828
>
> Log:
> Use copyfrom-args when creating/applying (svn)patch in a WC->WC diff
> context towards file-copy (directory-copy needs some work).
>
> * subversion/include/svn_wc.h
> (svn_wc_notify_state_t): add svn_wc_notify_state_source_missing field.
> (svn_wc_diff_callbacks3_t): new; rev-up svn_wc_diff_callbacks2_t to
> add copyfrom-args support.
>
> * subversion/libsvn_wc/diff.c
> (path_driver_cb_func): use relative-to-root path instead of basename.
>
> * subversion/libsvn_client/patch.c:
> (docstrings): update from svn_wc_diff_callbacks2_t to
> svn_wc_diff_callbacks3_t.
> (merge_file_added): update to svn_wc_diff_callbacks3_t; split the
> merge in two depending on whether this is scheduled-*with-history*.
> (merge_dir_added, patch_callbacks, edit_baton): update to use
> svn_wc_diff_callbacks3_t.
> (file_baton, make_file_baton): add/use copyfrom_path and copyfrom_rev.
> (delete_entry, open_file): update make_file_baton call.
> (add_directory, add_file): make use of copyfrom_path and
> copyfrom_revision args according to svn_wc_diff_callbacks3_t.
> (close_file): update file_added call with copyfrom-args; make use of
> svn_wc_notify_state_source_missing for notification.
> (make_editor_baton): switch to svn_wc_diff_callbacks3_t use.
>
> * subversion/tests/cmdline/diff_tests.py:
> (diff_svnpatch): add-file now has a copyfrom-path in a
> relative-to-root fashion.
>
> * subversion/svn/notify.c:
> (notify): add case for svn_wc_notify_state_source_missing.
>
>
> Modified:
> branches/svnpatch-diff/subversion/include/svn_wc.h
> branches/svnpatch-diff/subversion/libsvn_client/patch.c
> branches/svnpatch-diff/subversion/libsvn_wc/diff.c
> branches/svnpatch-diff/subversion/svn/notify.c
> branches/svnpatch-diff/subversion/tests/cmdline/diff_tests.py
>
> Modified: branches/svnpatch-diff/subversion/include/svn_wc.h
> URL: http://svn.collab.net/viewvc/svn/branches/svnpatch-diff/subversion/include/svn_wc.h?pathrev=26828&r1=26827&r2=26828
> ==============================================================================
> --- branches/svnpatch-diff/subversion/include/svn_wc.h (original)
> +++ branches/svnpatch-diff/subversion/include/svn_wc.h Fri Sep 28 07:03:42 2007
> @@ -767,7 +767,10 @@
> svn_wc_notify_state_merged,
>
> /** Modified state got conflicting mods. */
> - svn_wc_notify_state_conflicted
> + svn_wc_notify_state_conflicted,
> +
> + /** The source to copy the file from is missing. */
> + svn_wc_notify_state_source_missing
If this is really copyfrom-specific, perhaps the word "copy" should be
in the enum name.
>
> } svn_wc_notify_state_t;
>
> @@ -1079,12 +1082,13 @@
>
> /**
> * A callback vtable invoked by our diff-editors, as they receive
> - * diffs from the server. 'svn diff' and 'svn merge' both implement
> - * their own versions of this table.
> + * diffs from the server. 'svn diff', 'svn merge' and 'svn patch' all
> + * three implement their own versions of this table, though patch's is
> + * very similar to merge's.
I'd kill the word "three" in this sentence; and I'm not sure that it's
useful to mention the similarity here (and if they're so similar, can
the common parts be abstracted out?)
> *
> - * @since New in 1.2.
> + * @since New in 1.5.
> */
> -typedef struct svn_wc_diff_callbacks2_t
> +typedef struct svn_wc_diff_callbacks3_t
> {
> /** A file @a path has changed. If @a tmpfile2 is non-NULL, the
> * contents have changed and those changes can be seen by comparing
> @@ -1152,6 +1156,9 @@
> * @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.)
> *
> + * @a copyfrom_path and @a copyfrom_rev are used when dealing with
> + * copied file. They are respectively the source path and the
> + * source revision from which the file was copied.
Perhaps just steal from the delta editor docstring:
* If @a copyfrom_path is non-@c NULL, this add has history (i.e., is a
* copy), and the origin of the copy may be recorded as
* @a copyfrom_path under @a copyfrom_revision.
(Be specific that it is relevant only if non-NULL.)
I see that you are revving file_added and dir_added. If I understand
correctly, you're only making the 'svn patch' implementation of the
diff callbacks use the new svn_wc_diff. First, are you planning to
actually make the other implementations use the new args? (I'm not
sure if that's easy/advisable.) Second, are you planning to rev all
of the APIs which refer to svn_wc_diff_callbacks2_t? (But then you
have to come up with a way to make svn_wc_diff_callbacks2_t
implementations deal with getting unexpected copyfrom args...)
> */
> svn_error_t *(*file_added)(svn_wc_adm_access_t *adm_access,
> svn_wc_notify_state_t *contentstate,
> @@ -1163,6 +1170,8 @@
> svn_revnum_t rev2,
> const char *mimetype1,
> const char *mimetype2,
> + const char *copyfrom_path,
> + svn_revnum_t copyfrom_rev,
> const apr_array_header_t *propchanges,
> apr_hash_t *originalprops,
> void *diff_baton);
> @@ -1201,11 +1210,17 @@
> *
> * @a adm_access will be an access baton for the directory containing
> * @a path, or @c NULL if the diff editor is not using access batons.
> + *
> + * @a copyfrom_path and @a copyfrom_rev are used when dealing with
> + * copied directory. They are respectively the source path and the
> + * source revision from which the directory was copied.
Similar to above.
> */
> svn_error_t *(*dir_added)(svn_wc_adm_access_t *adm_access,
> svn_wc_notify_state_t *state,
> const char *path,
> svn_revnum_t rev,
> + const char *copyfrom_path,
> + svn_revnum_t copyfrom_rev,
> void *diff_baton);
>
> /** A directory @a path was deleted.
> @@ -1250,6 +1265,80 @@
> apr_hash_t *original_props,
> void *diff_baton);
>
> +} svn_wc_diff_callbacks3_t;
> +
> +/**
> + * Similar to @c svn_wc_diff_callbacks3_t, but without @a copyfrom_path
> + * and @a copyfrom_rev args for both @c file_added and @c dir_added.
> + *
> + * @deprecated Provided for backward compatibility with the 1.2 API.
> + */
> +typedef struct svn_wc_diff_callbacks2_t
> +{
> + /** The same as @c file_changed in @c svn_wc_diff_callbacks3_t. */
> + svn_error_t *(*file_changed)(svn_wc_adm_access_t *adm_access,
> + svn_wc_notify_state_t *contentstate,
> + svn_wc_notify_state_t *propstate,
> + const char *path,
> + const char *tmpfile1,
> + const char *tmpfile2,
> + svn_revnum_t rev1,
> + svn_revnum_t rev2,
> + const char *mimetype1,
> + const char *mimetype2,
> + const apr_array_header_t *propchanges,
> + apr_hash_t *originalprops,
> + void *diff_baton);
> +
> + /** Similar to @c svn_wc_diff_callbacks3_t's @c file_added but without
> + * @a copyfrom_path and @a copyfrom_rev arguments. */
> + svn_error_t *(*file_added)(svn_wc_adm_access_t *adm_access,
> + svn_wc_notify_state_t *contentstate,
> + svn_wc_notify_state_t *propstate,
> + const char *path,
> + const char *tmpfile1,
> + const char *tmpfile2,
> + svn_revnum_t rev1,
> + svn_revnum_t rev2,
> + const char *mimetype1,
> + const char *mimetype2,
> + const apr_array_header_t *propchanges,
> + apr_hash_t *originalprops,
> + void *diff_baton);
> +
> + /** The same as @c file_deleted in @c svn_wc_diff_callbacks3_t. */
> + svn_error_t *(*file_deleted)(svn_wc_adm_access_t *adm_access,
> + svn_wc_notify_state_t *state,
> + const char *path,
> + const char *tmpfile1,
> + const char *tmpfile2,
> + const char *mimetype1,
> + const char *mimetype2,
> + apr_hash_t *originalprops,
> + void *diff_baton);
> +
> + /** Similar to @c svn_wc_diff_callbacks3_t's @c dir_added but without
> + * @a copyfrom_path and @a copyfrom_rev arguments. */
> + svn_error_t *(*dir_added)(svn_wc_adm_access_t *adm_access,
> + svn_wc_notify_state_t *state,
> + const char *path,
> + svn_revnum_t rev,
> + void *diff_baton);
> +
> + /** The same as @c dir_deleted in @c svn_wc_diff_callbacks3_t. */
> + svn_error_t *(*dir_deleted)(svn_wc_adm_access_t *adm_access,
> + svn_wc_notify_state_t *state,
> + const char *path,
> + void *diff_baton);
> +
> + /** The same as @c dir_props_changed in @c svn_wc_diff_callbacks3_t. */
> + svn_error_t *(*dir_props_changed)(svn_wc_adm_access_t *adm_access,
> + svn_wc_notify_state_t *state,
> + const char *path,
> + const apr_array_header_t *propchanges,
> + apr_hash_t *original_props,
> + void *diff_baton);
> +
> } svn_wc_diff_callbacks2_t;
>
> /**
>
> Modified: branches/svnpatch-diff/subversion/libsvn_client/patch.c
> URL: http://svn.collab.net/viewvc/svn/branches/svnpatch-diff/subversion/libsvn_client/patch.c?pathrev=26828&r1=26827&r2=26828
> ==============================================================================
> --- branches/svnpatch-diff/subversion/libsvn_client/patch.c (original)
> +++ branches/svnpatch-diff/subversion/libsvn_client/patch.c Fri Sep 28 07:03:42 2007
> @@ -94,7 +94,7 @@
>
>
>
> -/* A svn_wc_diff_callbacks2_t function. Used for both file and directory
> +/* A svn_wc_diff_callbacks3_t function. Used for both file and directory
> property merges. */
> static svn_error_t *
> merge_props_changed(svn_wc_adm_access_t *adm_access,
> @@ -148,7 +148,7 @@
> return SVN_NO_ERROR;
> }
>
> -/* A svn_wc_diff_callbacks2_t function. */
> +/* A svn_wc_diff_callbacks3_t function. */
> static svn_error_t *
> merge_file_changed(svn_wc_adm_access_t *adm_access,
> svn_wc_notify_state_t *content_state,
> @@ -283,7 +283,7 @@
> return SVN_NO_ERROR;
> }
>
> -/* A svn_wc_diff_callbacks2_t function. */
> +/* A svn_wc_diff_callbacks3_t function. */
> static svn_error_t *
> merge_file_added(svn_wc_adm_access_t *adm_access,
> svn_wc_notify_state_t *content_state,
> @@ -295,16 +295,22 @@
> svn_revnum_t rev2,
> const char *mimetype1,
> const char *mimetype2,
> + const char *copyfrom_path,
> + svn_revnum_t copyfrom_rev,
> const apr_array_header_t *prop_changes,
> apr_hash_t *original_props,
> void *baton)
> {
> struct patch_cmd_baton *patch_b = baton;
> apr_pool_t *subpool = svn_pool_create(patch_b->pool);
> + const char *mine_basename = svn_path_basename(mine, subpool);
> svn_node_kind_t kind;
> int i;
> apr_hash_t *new_props;
>
> + /* This new file can't have any original prop in this offline context. */
> + original_props = apr_hash_make(subpool);
> +
> /* In most cases, we just leave prop_state as unknown, and let the
> content_state what happened, so we set prop_state here to avoid that
> below. */
> @@ -354,23 +360,54 @@
> svn_pool_destroy(subpool);
> return SVN_NO_ERROR;
> }
> +
> if (! patch_b->dry_run)
> {
> - /* Since 'mine' doesn't exist, and this is
> - 'merge_file_added', I hope it's safe to assume that
> - 'older' is empty, and 'yours' is the full file. Merely
> - copying 'yours' to 'mine', isn't enough; we need to get
> - the whole text-base and props installed too, just as if
> - we had called 'svn cp wc wc'. */
> - SVN_ERR(svn_wc_add_repos_file2(mine, adm_access, yours, NULL,
> - new_props, NULL, NULL,
> - SVN_IGNORED_REVNUM, subpool));
> - /* The file 'yours' points to edit_baton's cached empty file. Since
> - * it was just moved, set it to empty so that later get_empty_file()
> - * calls don't get it wrong with the cache, i.e. create another
> - * empty file again. */
> - *(char *)yours = '\0';
> + if (copyfrom_path) /* schedule-add-with-history */
> + {
> + /* Check whether the source we want to copy from exists. */
> + svn_node_kind_t copyfrom_kind;
> + SVN_ERR(svn_io_check_path(copyfrom_path, ©from_kind,
> + subpool));
> +
> + if (copyfrom_kind == svn_node_none)
What if it's a directory?
> + {
> + if (content_state)
> + *content_state = svn_wc_notify_state_source_missing;
> + svn_pool_destroy(subpool);
> + return SVN_NO_ERROR;
> + }
> +
> + SVN_ERR(svn_wc_copy2(copyfrom_path, adm_access, mine_basename,
> + patch_b->ctx->cancel_func,
> + patch_b->ctx->cancel_baton,
> + NULL, NULL, /* no notification */
> + subpool));
> + }
> + else /* schedule-add */
> + {
> + /* Copy the cached empty file and schedule-add it. The
> + * contents will come in either via apply-textdelta
> + * following calls if this is a binary file or with
> + * unidiff for text files. */
> + SVN_ERR(svn_io_copy_file(yours, mine, TRUE, subpool));
> + SVN_ERR(svn_wc_add2(mine, adm_access, NULL, SVN_IGNORED_REVNUM,
> + patch_b->ctx->cancel_func,
> + patch_b->ctx->cancel_baton,
> + NULL, NULL, /* no notification */
> + subpool));
Hmm, is this equivalent to the svn_wc_add_repos_file2?
> + }
> +
> + /* Now regardless of the schedule-add nature, merge properties. */
> + if (prop_changes->nelts > 0)
> + SVN_ERR(merge_props_changed(adm_access, prop_state,
> + mine, prop_changes,
> + original_props, baton));
> + else
> + if (prop_state)
> + *prop_state = svn_wc_notify_state_unchanged;
> }
> +
> if (content_state)
> *content_state = svn_wc_notify_state_changed;
> if (prop_state && apr_hash_count(new_props))
> @@ -436,7 +473,7 @@
> return SVN_NO_ERROR;
> }
>
> -/* A svn_wc_diff_callbacks2_t function. */
> +/* A svn_wc_diff_callbacks3_t function. */
> static svn_error_t *
> merge_file_deleted(svn_wc_adm_access_t *adm_access,
> svn_wc_notify_state_t *state,
> @@ -508,12 +545,14 @@
> return SVN_NO_ERROR;
> }
>
> -/* A svn_wc_diff_callbacks2_t function. */
> +/* A svn_wc_diff_callbacks3_t function. */
> static svn_error_t *
> merge_dir_added(svn_wc_adm_access_t *adm_access,
> svn_wc_notify_state_t *state,
> const char *path,
> svn_revnum_t rev,
> + const char *copyfrom_path,
> + svn_revnum_t copyfrom_rev,
> void *baton)
> {
> struct patch_cmd_baton *patch_b = baton;
> @@ -667,7 +706,7 @@
> (*mdb->ctx->notify_func2)(mdb->ctx->notify_baton2, notify, pool);
> }
>
> -/* A svn_wc_diff_callbacks2_t function. */
> +/* A svn_wc_diff_callbacks3_t function. */
> static svn_error_t *
> merge_dir_deleted(svn_wc_adm_access_t *adm_access,
> svn_wc_notify_state_t *state,
> @@ -744,7 +783,7 @@
> * names as (a) they are pretty much merge operations (b) even if
> * tweaked them to meet 'svn patch' needs, they do pretty much what
> * their real sibblings do. */
> -static const svn_wc_diff_callbacks2_t
> +static const svn_wc_diff_callbacks3_t
> patch_callbacks =
> {
> merge_file_changed,
> @@ -774,7 +813,7 @@
> const char *empty_file;
>
> /* The merge callbacks array and its baton. */
> - const svn_wc_diff_callbacks2_t *diff_callbacks;
> + const svn_wc_diff_callbacks3_t *diff_callbacks;
> void *diff_cmd_baton;
>
> /* If the func is non-null, send notifications of actions. */
> @@ -843,6 +882,12 @@
> /* A cache of any property changes (svn_prop_t) received for this file. */
> apr_array_header_t *propchanges;
>
> + /* The source file's path the file was copied from. */
> + const char *copyfrom_path;
> +
> + /* The source file's revision the file was copied from. */
> + svn_revnum_t copyfrom_rev;
> +
I'm not sure that this is grammatical; "If non-NULL, the path of the
file this file was copied from"?
> /* The pool passed in by add_file or open_file.
> Also, the pool this file_baton is allocated in. */
> apr_pool_t *pool;
> @@ -885,6 +930,8 @@
> svn_boolean_t added,
> void *edit_baton,
> struct dir_baton *parent_baton,
> + const char *copyfrom_path,
> + svn_revnum_t copyfrom_rev,
> apr_pool_t *pool)
> {
> struct file_baton *file_baton = apr_pcalloc(pool, sizeof(*file_baton));
> @@ -898,6 +945,8 @@
> file_baton->propchanges = apr_array_make(pool, 1, sizeof(svn_prop_t));
> file_baton->dir_baton = parent_baton;
> file_baton->is_binary = FALSE;
> + file_baton->copyfrom_path = copyfrom_path;
> + file_baton->copyfrom_rev = copyfrom_rev;
>
> return file_baton;
> }
> @@ -1065,7 +1114,8 @@
> struct file_baton *b;
>
> /* Compare a file being deleted against an empty file */
> - b = make_file_baton(path, FALSE, eb, pb, pool);
> + b = make_file_baton(path, FALSE, eb, pb, NULL,
> + SVN_IGNORED_REVNUM, pool);
>
> SVN_ERR(eb->diff_callbacks->file_deleted
> (adm_access, &state, b->wcpath,
> @@ -1134,8 +1184,6 @@
> svn_wc_notify_state_t state;
> svn_wc_notify_action_t action;
>
> - /* ### TODO: support copyfrom? */
> -
> b = make_dir_baton(path, pb, eb, TRUE, pool);
> *child_baton = b;
>
> @@ -1144,6 +1192,7 @@
>
> SVN_ERR(eb->diff_callbacks->dir_added
> (adm_access, &state, b->wcpath, SVN_IGNORED_REVNUM,
> + copyfrom_path, copyfrom_revision,
> eb->diff_cmd_baton));
>
> if ((state == svn_wc_notify_state_missing)
> @@ -1191,9 +1240,8 @@
> struct dir_baton *pb = parent_baton;
> struct file_baton *b;
>
> - /* ### TODO: support copyfrom? */
> -
> - b = make_file_baton(path, TRUE, pb->edit_baton, pb, pool);
> + b = make_file_baton(path, TRUE, pb->edit_baton, pb,
> + copyfrom_path, copyfrom_revision, pool);
> *file_baton = b;
>
> /* We want to schedule this file for addition. */
> @@ -1213,7 +1261,8 @@
> struct dir_baton *pb = parent_baton;
> struct file_baton *b;
>
> - b = make_file_baton(path, FALSE, pb->edit_baton, pb, pool);
> + b = make_file_baton(path, FALSE, pb->edit_baton, pb,
> + NULL, SVN_IGNORED_REVNUM, pool);
> *file_baton = b;
>
> return SVN_NO_ERROR;
> @@ -1344,6 +1393,7 @@
> SVN_IGNORED_REVNUM,
> SVN_IGNORED_REVNUM,
> NULL, mimetype,
> + b->copyfrom_path, b->copyfrom_rev,
> b->propchanges, NULL,
> b->edit_baton->diff_cmd_baton));
> else
> @@ -1355,13 +1405,14 @@
> SVN_IGNORED_REVNUM,
> SVN_IGNORED_REVNUM,
> NULL, mimetype,
> - b->propchanges, NULL,
> + b->propchanges, NULL, /* use base props */
> b->edit_baton->diff_cmd_baton));
> }
>
>
> if ((content_state == svn_wc_notify_state_missing)
> - || (content_state == svn_wc_notify_state_obstructed))
> + || (content_state == svn_wc_notify_state_obstructed)
> + || (content_state == svn_wc_notify_state_source_missing))
> action = svn_wc_notify_skip;
> else if (b->added)
> action = svn_wc_notify_update_add;
> @@ -1497,7 +1548,7 @@
> make_editor_baton(const char *target,
> svn_wc_adm_access_t *adm_access,
> svn_boolean_t dry_run,
> - const svn_wc_diff_callbacks2_t *callbacks,
> + const svn_wc_diff_callbacks3_t *callbacks,
> void *patch_cmd_baton,
> svn_wc_notify_func2_t notify_func,
> void *notify_baton,
>
> Modified: branches/svnpatch-diff/subversion/libsvn_wc/diff.c
> URL: http://svn.collab.net/viewvc/svn/branches/svnpatch-diff/subversion/libsvn_wc/diff.c?pathrev=26828&r1=26827&r2=26828
> ==============================================================================
> --- branches/svnpatch-diff/subversion/libsvn_wc/diff.c (original)
> +++ branches/svnpatch-diff/subversion/libsvn_wc/diff.c Fri Sep 28 07:03:42 2007
> @@ -1548,7 +1548,7 @@
>
> *dir_baton = NULL;
> if (entry->copyfrom_url)
> - copyfrom_url = svn_path_basename(entry->copyfrom_url, pb->pool);
> + copyfrom_url = svn_path_is_child(entry->repos, entry->copyfrom_url, NULL);
Why is this change necessary? It's to code that isn't
svnpatch-specific, so hopefully it doesn't have adverse effects elsewhere...
>
> /* If the join baton is set we're performing a drive relative to the
> * directory @c join_dir_baton holds. */
>
> Modified: branches/svnpatch-diff/subversion/svn/notify.c
> URL: http://svn.collab.net/viewvc/svn/branches/svnpatch-diff/subversion/svn/notify.c?pathrev=26828&r1=26827&r2=26828
> ==============================================================================
> --- branches/svnpatch-diff/subversion/svn/notify.c (original)
> +++ branches/svnpatch-diff/subversion/svn/notify.c Fri Sep 28 07:03:42 2007
> @@ -73,6 +73,13 @@
> path_local)))
> goto print_error;
> }
> + else if (n->content_state == svn_wc_notify_state_source_missing)
> + {
> + if ((err = svn_cmdline_printf
> + (pool, _("Skipped target: '%s' -- copy-source is missing\n"),
Is "copy-source" terminology we use in svn?
(And would it be useful to say *what* the missing copy source is?)
> + path_local)))
> + goto print_error;
> + }
> else
> {
> if ((err = svn_cmdline_printf
>
> Modified: branches/svnpatch-diff/subversion/tests/cmdline/diff_tests.py
> URL: http://svn.collab.net/viewvc/svn/branches/svnpatch-diff/subversion/tests/cmdline/diff_tests.py?pathrev=26828&r1=26827&r2=26828
> ==============================================================================
> --- branches/svnpatch-diff/subversion/tests/cmdline/diff_tests.py (original)
> +++ branches/svnpatch-diff/subversion/tests/cmdline/diff_tests.py Fri Sep 28 07:03:42 2007
> @@ -2710,7 +2710,7 @@
> '( delete-entry ( 5:A/D/H 2:d6 ) ) ',
> '( close-dir ( 2:d6 ) ) ',
> '( add-dir ( 3:A/T 2:d1 2:d8 ( ) ) ) ',
> - '( add-file ( 8:A/T/mumu 2:d8 2:c9 ( 2:mu ) ) ) ',
> + '( add-file ( 8:A/T/mumu 2:d8 2:c9 ( 4:A/mu ) ) ) ',
> '( change-file-prop ( 2:c9 13:svn:mergeinfo ( 0: ) ) ) ',
> '( close-file ( 2:c9 ( ) ) ) ',
> '( close-dir ( 2:d8 ) ) ',
> @@ -2757,7 +2757,7 @@
>
> # No copy-path is received from the server.
> svnpatch_output_base_head[20] =\
> - svnpatch_output_base_head[20].replace('2:mu ','')
> + svnpatch_output_base_head[20].replace('4:A/mu ','')
>
> expected_svnpatch_base_head = svnpatch_encode(svnpatch_output_base_head)
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: svn-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: svn-help@subversion.tigris.org
>
>
--
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r26828 - in branches/svnpatch-diff/subversion: include libsvn_client libsvn_wc svn tests/cmdline
Posted by Charles Acknin <ch...@gmail.com>.
On 9/28/07, David Glasser <gl...@davidglasser.net> wrote:
> > + /** The source to copy the file from is missing. */
> > + svn_wc_notify_state_source_missing
>
> If this is really copyfrom-specific, perhaps the word "copy" should be
> in the enum name.
I figured something like svn_wc_notify_state_copy_source_missing would
be horribly long. svn_wc_notify_state_cp_src_missing? Hmm, not much
better.
> > /**
> > * A callback vtable invoked by our diff-editors, as they receive
> > - * diffs from the server. 'svn diff' and 'svn merge' both implement
> > - * their own versions of this table.
> > + * diffs from the server. 'svn diff', 'svn merge' and 'svn patch' all
> > + * three implement their own versions of this table, though patch's is
> > + * very similar to merge's.
>
> I'd kill the word "three" in this sentence; and I'm not sure that it's
> useful to mention the similarity here (and if they're so similar, can
> the common parts be abstracted out?)
Fixed in r26897.
> > *
> > - * @since New in 1.2.
> > + * @since New in 1.5.
> > */
> > -typedef struct svn_wc_diff_callbacks2_t
> > +typedef struct svn_wc_diff_callbacks3_t
> > {
> > /** A file @a path has changed. If @a tmpfile2 is non-NULL, the
> > * contents have changed and those changes can be seen by comparing
> > @@ -1152,6 +1156,9 @@
> > * @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.)
> > *
> > + * @a copyfrom_path and @a copyfrom_rev are used when dealing with
> > + * copied file. They are respectively the source path and the
> > + * source revision from which the file was copied.
>
> Perhaps just steal from the delta editor docstring:
>
> * If @a copyfrom_path is non-@c NULL, this add has history (i.e., is a
> * copy), and the origin of the copy may be recorded as
> * @a copyfrom_path under @a copyfrom_revision.
>
> (Be specific that it is relevant only if non-NULL.)
Fixed in r26897 as well.
> I see that you are revving file_added and dir_added. If I understand
> correctly, you're only making the 'svn patch' implementation of the
> diff callbacks use the new svn_wc_diff. First, are you planning to
> actually make the other implementations use the new args? (I'm not
> sure if that's easy/advisable.)
Well because this is svnpatch-diff branch, I'm trying not to go beyond
the scope and stick with 'svn patch'-specific changes here. However,
I've noticed 'svn merge' would need this too (e.g.
subversion/libsvn_client/merge.c:merge_file_added) , and this made it
a good argument to rev-up that vtable.
> Second, are you planning to rev all
> of the APIs which refer to svn_wc_diff_callbacks2_t? (But then you
> have to come up with a way to make svn_wc_diff_callbacks2_t
> implementations deal with getting unexpected copyfrom args...)
Would that be fine if we move svn_wc_diff_callbacks2_t implementations
to svn_wc_diff_callbacks3_t as they need it? Like when it comes to
use the copyfrom stuffs for 'svn merge' we just move to callbacks3
scheme? (As for now, since this is just a vtable it doesn't break
compatibility and no need for compat-wrappers.)
> > + if (copyfrom_path) /* schedule-add-with-history */
> > + {
> > + /* Check whether the source we want to copy from exists. */
> > + svn_node_kind_t copyfrom_kind;
> > + SVN_ERR(svn_io_check_path(copyfrom_path, ©from_kind,
> > + subpool));
> > +
> > + if (copyfrom_kind == svn_node_none)
>
> What if it's a directory?
As the log message says, I still have to work on the directory copy.
> > + else /* schedule-add */
> > + {
> > + /* Copy the cached empty file and schedule-add it. The
> > + * contents will come in either via apply-textdelta
> > + * following calls if this is a binary file or with
> > + * unidiff for text files. */
> > + SVN_ERR(svn_io_copy_file(yours, mine, TRUE, subpool));
> > + SVN_ERR(svn_wc_add2(mine, adm_access, NULL, SVN_IGNORED_REVNUM,
> > + patch_b->ctx->cancel_func,
> > + patch_b->ctx->cancel_baton,
> > + NULL, NULL, /* no notification */
> > + subpool));
>
> Hmm, is this equivalent to the svn_wc_add_repos_file2?
I recall svn_wc_add_repos_file2 would add-with-history. Which is what
we don't want here since I did split the pipe in two to precisely
address scheduled-add-with-history and scheduled-add differently.
> > + /* The source file's path the file was copied from. */
> > + const char *copyfrom_path;
> > +
> > + /* The source file's revision the file was copied from. */
> > + svn_revnum_t copyfrom_rev;
> > +
>
> I'm not sure that this is grammatical; "If non-NULL, the path of the
> file this file was copied from"?
This makes me think, where did I read it is incorrect to place
prepositions (from, to, with, etc.) at the end in english? Like "the
path of the file from which this file was copied" instead of "the path
of the file this file was copied from"? (My use is wrong too then.)
> > --- branches/svnpatch-diff/subversion/svn/notify.c (original)
> > +++ branches/svnpatch-diff/subversion/svn/notify.c Fri Sep 28 07:03:42 2007
> > @@ -73,6 +73,13 @@
> > path_local)))
> > goto print_error;
> > }
> > + else if (n->content_state == svn_wc_notify_state_source_missing)
> > + {
> > + if ((err = svn_cmdline_printf
> > + (pool, _("Skipped target: '%s' -- copy-source is missing\n"),
>
> Is "copy-source" terminology we use in svn?
I don't know but `grep "copy-source"` catches a few. Thinking about
something more appropriate?
> (And would it be useful to say *what* the missing copy source is?)
I thought about it while implementing too. The way to achieve this is
to add a copyfrom_path field to svn_wc_notify_t, which implies a
revving here too. Sounds good?
Cheers,
Charles
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org