On 9/28/07, David Glasser <glasser@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
Received on Tue Oct 2 23:57:52 2007