Paul Burba wrote:
> 1) "Takeover" is no more, the term that is. [...]
Good. All other changes good.
> Sidebar: I noticed that this "repair" behavior is not described in the doc
> string for svn_wc_text_modified_p(), should it be?
Yes, I'd say that is worth mentioning. The way to document it would be to say
that the function *may* repair the time stamp, not that it *does*. If that was
already the case, a separate patch would be ideal but I'd be happy for it to be
included in this patch if you like.
> [[[
[...]
> * subversion/libsvn_client/checkout.c
> (svn_client__checkout_internal): New argument. Update calls to
> svn_client__update_internal.
> (svn_client_checkout3): New.
> (svn_client_checkout): Reimplement as wrapper around
> svn_client_checkout3.
> (svn_client_checkout): Update call to svn_client__checkout_internal.
One of those should say "(svn_client_checkout2)".
> * subversion/libsvn_wc/questions.c
> (svn_wc__text_modified_internal_p): New arg. Update call to
> svn_wc__text_base_path.
> (svn_wc_text_modified_p2): New. Update comment.
> (svn_wc_text_modified_p): Reimplement as wrapper around
> svn_wc_text_modified_p2
I'm not keen on adding a boolean parameter to any API. Calls with multiple
booleans quickly become unreadable. But it's not a big deal here compared with
some of our other APIs.
Thinking out loud... I do wonder whether providing operations on the temporary
text base is something we really want to be exposing in this way. Are we
providing the option to access the "tmp" text base in all other libsvn_wc APIs
that access the text base? (I haven't looked.)
> * subversion/libsvn_wc/wc.h
> (svn_wc__text_modified_internal_p): New arg.
It would be helpful if the log indicated why or what the new arg is, like you
have done with other items, so I could get a feel for the change without going
to the code.
> Index: subversion/include/svn_wc.h
> ===================================================================
[...]
> +/* Similar to svn_wc_text_modified_p2() but with the use_tmp_base
Start Doxygen comments with "/**". Doxygen says:
subversion/include/svn_wc.h:1125: Warning: Member svn_wc_text_modified_p... is
not documented.
subversion/include/svn_wc.h:2537: Warning: Member svn_wc_get_update_editor2...
is not documented.
subversion/include/svn_wc.h:2638: Warning: Member svn_wc_get_switch_editor2...
is not documented.
> checkout (co): Check out a working copy from a repository.
> usage: checkout URL[@REV]... [PATH]
[...]
> If --force is used, unversioned obstructing paths in the working
> copy destination do not automatically cause the check out to fail.
> If the obstructing path is the same type (file or directory) as the
> corresponding path in the repository it will be left 'as-is' in the
> working copy. For directories this simply means the obstruction is
"simply ... the obstruction is tolerated": this implies to me that nothing will
be done to the unversioned directory. However, "svn checkout" converts it into
a versioned directory and then proceeds inside it. Perhaps you were thinking
of the implementation step that tries to create the directory tolerating its
existence; however, from the user's point of view of the whole operation, it is
not simply tolerated but is converted.
> tolerated. For files, any content differences between the
> obstruction and the repository are treated like a local modification
> to the working copy. All properties from the repository and
> applicable auto-props are applied to the obstructing path.
Properties from the repository AND those that would have been given to it by
"avn add"? As in the two sets of properties are merged? I think it would make
sense to give the working file the properties that "svn add" would have given
it, and then any differences between that set and the pristine base set from
the repository will show up as local modifications. To me that would be the
equivalent for properties of what we do with the base text, and therefore the
right thing to do.
(I may have missed some design decisions, or maybe it just wasn't clearly
resolved.)
> For each item checked out a line will start with a character
> reporting the action taken. For unforced checkouts this character
> is always 'A' for 'Added'. Forced checkouts report obstructed paths
> with 'E' for 'Existed' and use 'A' otherwise.
This paragraph is talking about files Added by the checkout, but doesn't say
so, incorrectly implying that no other codes than 'E' and 'A' can result from a
checkout.
> update (up): Bring changes from the repository into the working copy.
> usage: update [PATH...]
[...]
> For each updated item a line will start with a character reporting the
> action taken. These characters have the following meaning:
>
> A Added
> D Deleted
> U Updated
> C Conflict
> G Merged
>
> A character in the first column signifies an update to the actual file,
[...]
>
> If --force is used, unversioned obstructing paths in the working
[...]
> auto-props are applied to the obstructing path. Obstructing paths are
> reported in the first column with:
>
> E Existed
I would suggest this notification code belongs in the list above, and just
ending this last sentence with "with the code 'E'."
> switch (sw): Update the working copy to a different URL.
> usage: 1. switch URL [PATH]
[...]
> auto-props are applied to the obstructing path. Obstructing paths are
> reported in the first column with:
>
> E Existed
This last sentence doesn't make so much sense in the "switch" command help
because columns and the codes used in them haven't been mentioned earlier on in
this help like they are in "update" help. It is probably best to omit this
sentence entirely, and let the user find the full list and partial description
of the notifications in the help for "update".
Otherwise, the log looks good. I don't have time to check the code thoroughly,
but I'm building it so I can try it out. I have recently been wanting this at
work so I'm glad you're doing this.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Jun 23 01:12:34 2006