Daniel Patterson <danpat@danpat.net> writes:
> kfogel@collab.net wrote:
> > There's a bug, though: when you do an export from a working copy
> > (instead of from a URL), the new -N flag is just ignored. It should
> > have the same effect when the source is a working copy as when the
> > source is a URL.
>
> Hmm, my bad, I've never used that feature, I didn't even know it
> existed.
>
> Updated log message below, updated patch attached. Still no
> test cases, my Python literacy is not great.
Nice work! This passes my hand tests, and passes 'make check', and
I'd like to commit it. But there's one problem: it is inconsistent in
how it treats svn:externals.
Let's take the cvs2svn repository as an example, since it uses
svn:externals. If you do
$ svn co http://svn.collab.net/repos/cvs2svn/trunk/ cvs2svn-wc
[...]
$ svn export -N http://svn.collab.net/repos/cvs2svn/trunk/ URL-exported
[...]
$ svn export -N cvs2svn-wc WC-exported
[...]
then afterwards, 'URL-exported' will contain the 'svntest' subdir
(which comes from the svn:externals property), yet 'WC-exported' will
not.
IMHO, the -N flag should prevent any externals from being exported.
In other words, your patch's from-wc behavior should happen in the
from-url case as well.
Do you agree with this proposal, and if so, would you be willing to
update the patch accordingly? In the meantime, I have filed
http://subversion.tigris.org/issues/show_bug.cgi?id=2228
...as this is getting close enough to being committed that it's nice
to have a place to track the latest version of the patch at all
times. Feel free to comment on the issue, add attachments, etc.
Regarding the patch itself, a few minor comments:
> [[[
> Add --non-recursive parameter to "svn export"
>
> * subversion/libsvn_client/export.c:
> (svn_client_export3): add new "recursive" parameter that
> gets passed through to svn_ra_do_update. Also, if exporting
> from a working copy, pass the recursive flag through to
> copy_versioned_files (which has been updated), see below.
> (svn_client_export2): default to passing TRUE for new
> "recurse" parameter
> (copy_versioned_files): add a recursive parameter and
> only do a recursive copy if it's true.
>
> * subversion/include/svn_client.h:
> (svn_client_export3): update public API to include new
> "recursive" parameter"
>
> * subversion/libsvn_client/externals.c:
> (handle_external_item_change): update call to
> svn_client_export3 to pass TRUE as the recursive value.
> When traversing externals, if we're doing a nonrecursive
> export, we should never get to this point, so TRUE should
> always be the case.
>
> * subversion/clients/cmdlind/export-cmd.c:
> (svn_cl__export): use updated svn_client_export3 function and pass
> inverse of --non-recursive command line through. Still defaults
> to the original behaviour of a recursive export.
>
> * subversion/clients/cmdline/main.c:
> (svn_cl__cmd_table[]): add "N" option to "export" command
>
> ]]]
Excellent log message. Might want to capitalize the first sentence in
each symbol summary, as we traditionally do, but no big deal. Also,
this is really an "option" not a "parameter" being added (at least,
that seems to be common usage among U.S. programmers, not sure about
elsewhere). And do give the "-N" short form too, in case anyone
searches the log messages on that instead of "--non-recursive".
> Index: subversion/include/svn_client.h
> ===================================================================
> --- subversion/include/svn_client.h (revision 13095)
> +++ subversion/include/svn_client.h (working copy)
> @@ -1588,6 +1588,9 @@
> * will use the standard eol marker. Any other value will cause the
> * SVN_ERR_IO_UNKNOWN_EOL error to be returned.
> *
> + * @a recurse passes through to svn_ra_do_update to make exports of
> + * directories recursive if TRUE.
> + *
> * All allocations are done in @a pool.
> */
A more verbose, but precise, description might be:
* If @a recurse is TRUE, then export subdirectories recursively, else
* just export @a from and its immediate non-directory children.
* Also, if @a recurse is TRUE, behave as if @a ignore_externals is
* also TRUE.
(I inserted a description of the proposed externals behavior just for
convenience, in case that's how we end up going.)
> Index: subversion/libsvn_client/export.c
> ===================================================================
> --- subversion/libsvn_client/export.c (revision 13095)
> +++ subversion/libsvn_client/export.c (working copy)
> @@ -852,7 +859,7 @@
> &reporter, &report_baton,
> revnum,
> "", /* no sub-target */
> - TRUE, /* recurse */
> + recurse, /* recurse */
> export_editor, edit_baton, pool));
I think leaving a comment "/* recurse */" next to a variable named
"recurse" qualifies as excessive redundancy :-).
Above minor nits notwithstanding, this was an excellent patch.
Looking forward to the next (and hopefully final) iteration.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Feb 22 04:37:00 2005