You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Bert Huijben <rh...@sharpsvn.net> on 2009/06/11 15:00:50 UTC
RE: svn commit: r37988 - in trunk/subversion: include
include/private libsvn_subr svn tests/cmdline
> -----Original Message-----
> From: Stefan Sperling [mailto:stsp@elego.de]
> Sent: donderdag 11 juni 2009 16:46
> To: svn@subversion.tigris.org
> Subject: svn commit: r37988 - in trunk/subversion: include include/private
> libsvn_subr svn tests/cmdline
>
> Author: stsp
> Date: Thu Jun 11 07:45:47 2009
> New Revision: 37988
>
> Log:
> Fix issue #3416, "Cannot add or commit 'dir/@file.txt'".
>
> Make every subcommand parse peg revisions which previously didn't
> parse them. The Book documents this behaviour as follows:
>
> After all, how does svn know whether news@11 is the name of a
> directory in my tree or just a syntax for “revision 11 of news”?
> Thankfully, while svn will always assume the latter, [...]
>
> Regrettably, svn didn't always assume the latter.
>
> We only ever parsed peg revisions for commands where they make sense.
> Escaping '@' characters in filenames was impossible for subcommands
> which didn't parse peg revisions. Subversion would always interpret
> 'abc@abc' as a complete filename for those commands, instead of
> as the nonsense 'revision abc of abc' it is.
>
> Even our unit tests didn't agree with The Book, and were merrily
> adding and committing 'abc@abc' without escaping.
>
> To add insult to injury, when splitting peg revisions from target
> paths we never ensured that the target path still had a non-zero
> length after the split. The code then went on to form a canonicalised
> target path with attached peg revision by canonicalising the empty
> path and then concatenating the empty canonicalisation result and
> the peg revision.
>
> This led to confusing behaviour were 'svn add @file' ('@file' is an
> actual filename) was accidentally working, but 'svn add @file@' was
> not -- it resulted in "svn add: Warning: '@file@' not found"
>
> In the former situation we now throw an error, because as far as svn
> can tell only a peg revision was specified, but no target path.
> 'svn add @file@' is the correct way to add a file called @file, and
> because 'svn add' will now parse and discard the peg revision specifier,
> this now works.
>
> This problem didn't just affect add, but every subcommand which
> didn't interpret peg revisions (commit, lock, etc.)
>
> * subversion/libsvn_subr/opt.c
> (svn_opt__split_arg_at_peg_revision): Require target path to have
> a non-zero length after splitting a peg revision from it.
> Allow callers to pass NULL for the parsed peg revision, so they can
> easily discard the parsed peg revision if they don't need it.
> (svn_opt_eat_peg_revisions): New function. Returns a copy of a list
> of targets with all peg revision specifiers removed.
>
> * subversion/tests/cmdline/basic_tests.py
> (basic_peg_revision): Consistently escape '@' characters in filenames,
> and check for regression into issue #3416.
>
> * subversion/svn/patch-cmd.c, subversion/svn/propdel-cmd.c,
> subversion/svn/mkdir-cmd.c, subversion/svn/move-cmd.c,
> subversion/svn/revert-cmd.c, subversion/svn/changelist-cmd.c,
> subversion/svn/update-cmd.c, subversion/svn/resolved-cmd.c,
> subversion/svn/upgrade-cmd.c, subversion/svn/cleanup-cmd.c,
> subversion/svn/commit-cmd.c, subversion/svn/add-cmd.c,
> subversion/svn/propset-cmd.c, subversion/svn/delete-cmd.c,
> subversion/svn/resolve-cmd.c, subversion/svn/status-cmd.c,
> subversion/svn/propedit-cmd.c, subversion/svn/lock-cmd.c,
> subversion/svn/unlock-cmd.c
> (svn_cl__patch, svn_cl__propdel, svn_cl__mkdir, svn_cl__move,
> svn_cl__revert, svn_cl__changelist, svn_cl__update, svn_cl__resolved,
> svn_cl__upgrade, svn_cl__cleanup, svn_cl__commit, svn_cl__add,
> svn_cl__propset, svn_cl__delete, svn_cl__resolve, svn_cl__status,
> svn_cl__propedit, svn_cl__lock, svn_cl__unlock): Remove peg revision
> specifiers from all targets before passing them to the client library,
> using the new function svn_opt_eat_peg_revisions().
>
> * subversion/include/private/svn_opt_private.h
> (svn_opt__split_arg_at_peg_revision): Document changes in behaviour
> described above (see entry for subversion/libsvn_subr/opt.c).
>
> * subversion/include/svn_opt.h
> (svn_opt_parse_path): Document a few additional examples of how peg
> revisions are parsed, all pertaining to issue #3416.
>
> Modified:
> trunk/subversion/include/private/svn_opt_private.h
> trunk/subversion/include/svn_opt.h
> trunk/subversion/libsvn_subr/opt.c
> trunk/subversion/svn/add-cmd.c
> trunk/subversion/svn/changelist-cmd.c
> trunk/subversion/svn/cleanup-cmd.c
> trunk/subversion/svn/commit-cmd.c
> trunk/subversion/svn/delete-cmd.c
> trunk/subversion/svn/lock-cmd.c
> trunk/subversion/svn/mkdir-cmd.c
> trunk/subversion/svn/move-cmd.c
> trunk/subversion/svn/patch-cmd.c
> trunk/subversion/svn/propdel-cmd.c
> trunk/subversion/svn/propedit-cmd.c
> trunk/subversion/svn/propset-cmd.c
> trunk/subversion/svn/resolve-cmd.c
> trunk/subversion/svn/resolved-cmd.c
> trunk/subversion/svn/revert-cmd.c
> trunk/subversion/svn/status-cmd.c
> trunk/subversion/svn/unlock-cmd.c
> trunk/subversion/svn/update-cmd.c
> trunk/subversion/svn/upgrade-cmd.c
> trunk/subversion/tests/cmdline/basic_tests.py
>
> Modified: trunk/subversion/include/private/svn_opt_private.h
> URL:
> http://svn.collab.net/viewvc/svn/trunk/subversion/include/private/svn_opt_priv
> ate.h?pathrev=37988&r1=37987&r2=37988
> ==============================================================================
> --- trunk/subversion/include/private/svn_opt_private.h Thu Jun 11 05:58:46
> 2009 (r37987)
> +++ trunk/subversion/include/private/svn_opt_private.h Thu Jun 11 07:45:47
> 2009 (r37988)
> @@ -32,15 +32,23 @@
> extern "C" {
> #endif /* __cplusplus */
>
> -/* Extract the peg revision, if any, from UTF8_TARGET. Return the peg
> - * revision in *PEG_REVISION and the true target portion in *TRUE_TARGET.
> +/* Extract the peg revision, if any, from UTF8_TARGET.
> + *
> + * If PEG_REVISION is not NULL, return the peg revision in *PEG_REVISION.
> * *PEG_REVISION will be an empty string if no peg revision is found.
> + * Return the true target portion in *TRUE_TARGET.
> *
> * UTF8_TARGET need not be canonical. *TRUE_TARGET will not be canonical
> * unless UTF8_TARGET is.
> *
> + * It is an error if *TRUE_TARGET results in the empty string after the
> + * split, which happens in case UTF8_TARGET has a leading '@' character
> + * with no additional '@' characters to escape the first '@'.
> + *
> * Note that *PEG_REVISION will still contain the '@' symbol as the first
> - * character if a peg revision was found.
> + * character if a peg revision was found. If a trailing '@' symbol was
> + * used to escape other '@' characters in UTF8_TARGET, *PEG_REVISION will
> + * point to the string "@", containing only a single character.
> *
> * All allocations are done in POOL.
> */
>
> Modified: trunk/subversion/include/svn_opt.h
> URL:
> http://svn.collab.net/viewvc/svn/trunk/subversion/include/svn_opt.h?pathrev=37
> 988&r1=37987&r2=37988
> ==============================================================================
> --- trunk/subversion/include/svn_opt.h Thu Jun 11 05:58:46 2009 (r37987)
> +++ trunk/subversion/include/svn_opt.h Thu Jun 11 07:45:47 2009 (r37988)
> @@ -621,11 +621,14 @@ svn_opt_parse_all_args(apr_array_header_
> * "foo/bar@1:2" -> error
> * "foo/bar@baz" -> error
> * "foo/bar@" -> "foo/bar", (base)
> + * "foo/@bar@" -> "foo/@bar", (base)
> * "foo/bar/@13" -> "foo/bar/", (number, 13)
> * "foo/bar@@13" -> "foo/bar@", (number, 13)
> * "foo/@bar@HEAD" -> "foo/@bar", (head)
> * "foo@/bar" -> "foo@/bar", (unspecified)
> * "foo@HEAD/bar" -> "foo@HEAD/bar", (unspecified)
> + * "@foo/bar" -> error
> + * "@foo/bar@" -> "@foo/bar", (unspecified)
> *
> * [*] Syntactically valid but probably not semantically useful.
> *
> @@ -734,6 +737,27 @@ svn_opt_print_help(apr_getopt_t *os,
> const char *footer,
> apr_pool_t *pool);
>
> +/* Return, in @a *true_targets_p, a copy of @a targets with peg revision
> + * specifiers snipped off the end of each element.
> + *
> + * This function is useful for subcommands for which peg revisions
> + * do not make any sense. Such subcommands still need to allow peg
> + * revisions to be specified on the command line so that users of
> + * the command line client can consistently escape '@' characters
> + * in filenames by appending an '@' character, regardless of the
> + * subcommand being used.
> + *
> + * If a peg revision is present but cannot be parsed, an error is thrown.
> + * The user has likely forgotten to escape an '@' character in a filename.
> + *
> + * It is safe to pass the address of @a targets as @a true_targets_p.
> + *
> + * Do all allocations in @a pool. */
> +svn_error_t *
> +svn_opt_eat_peg_revisions(apr_array_header_t **true_targets_p,
> + apr_array_header_t *targets,
> + apr_pool_t *pool);
> +
This new function needs a @since doxygen comment.
> #ifdef __cplusplus
> }
> #endif /* __cplusplus */
Bert
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2361316
RE: svn commit: r37988 - in trunk/subversion: include
include/private libsvn_subr svn tests/cmdline
Posted by Bert Huijben <rh...@sharpsvn.net>.
> -----Original Message-----
> From: Stefan Sperling [mailto:stsp@elego.de]
> Sent: donderdag 11 juni 2009 17:17
> To: Bert Huijben
> Cc: dev@subversion.tigris.org
> Subject: Re: svn commit: r37988 - in trunk/subversion: include
include/private
> libsvn_subr svn tests/cmdline
>
> On Thu, Jun 11, 2009 at 05:00:50PM +0200, Bert Huijben wrote:
> > > +/* Return, in @a *true_targets_p, a copy of @a targets with peg
revision
> > > + * specifiers snipped off the end of each element.
> > > + *
> > > + * This function is useful for subcommands for which peg revisions
> > > + * do not make any sense. Such subcommands still need to allow peg
> > > + * revisions to be specified on the command line so that users of
> > > + * the command line client can consistently escape '@' characters
> > > + * in filenames by appending an '@' character, regardless of the
> > > + * subcommand being used.
> > > + *
> > > + * If a peg revision is present but cannot be parsed, an error is
thrown.
> > > + * The user has likely forgotten to escape an '@' character in a
> filename.
> > > + *
> > > + * It is safe to pass the address of @a targets as @a true_targets_p.
> > > + *
> > > + * Do all allocations in @a pool. */
> > > +svn_error_t *
> > > +svn_opt_eat_peg_revisions(apr_array_header_t **true_targets_p,
> > > + apr_array_header_t *targets,
> > > + apr_pool_t *pool);
> > > +
> >
> > This new function needs a @since doxygen comment.
>
> What do we set it to? I'd like to backport it to 1.6.
>
> Should I set it to 1.7 for now, and make a note in STATUS that
> we should manually adjust the @since tag on trunk and 1.6.x
> if it gets backported?
We can't backport new public functions to 1.6 per our compatibility
guidelines. The usual method to work around this is to mark it as new in the
next version and backport a private version of the new function for use by
subversion only.
Bert
>
> Stefan
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2361326
Re: svn commit: r37988 - in trunk/subversion: include
include/private libsvn_subr svn tests/cmdline
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Jun 11, 2009 at 05:00:50PM +0200, Bert Huijben wrote:
> > +/* Return, in @a *true_targets_p, a copy of @a targets with peg revision
> > + * specifiers snipped off the end of each element.
> > + *
> > + * This function is useful for subcommands for which peg revisions
> > + * do not make any sense. Such subcommands still need to allow peg
> > + * revisions to be specified on the command line so that users of
> > + * the command line client can consistently escape '@' characters
> > + * in filenames by appending an '@' character, regardless of the
> > + * subcommand being used.
> > + *
> > + * If a peg revision is present but cannot be parsed, an error is thrown.
> > + * The user has likely forgotten to escape an '@' character in a filename.
> > + *
> > + * It is safe to pass the address of @a targets as @a true_targets_p.
> > + *
> > + * Do all allocations in @a pool. */
> > +svn_error_t *
> > +svn_opt_eat_peg_revisions(apr_array_header_t **true_targets_p,
> > + apr_array_header_t *targets,
> > + apr_pool_t *pool);
> > +
>
> This new function needs a @since doxygen comment.
What do we set it to? I'd like to backport it to 1.6.
Should I set it to 1.7 for now, and make a note in STATUS that
we should manually adjust the @since tag on trunk and 1.6.x
if it gets backported?
Stefan