Eating peg revisions [was: [PATCH] Fix issue #3651 - "svn copy does not eat peg revision within copy target path"]

[Julian Foad <ju...@wandisco.com>]

On Wed, 2010-06-09 at 17:05 +0530, Senthil Kumaran S wrote:
> Hi,
> 
> I am attaching a patch to fix issue #3651, just to check whether this is what
> stsp intended to say via
> http://subversion.tigris.org/issues/show_bug.cgi?id=3651#desc2
> 
> Thank You.
> plain text document attachment (3651.patch.txt)
> [[[
> Fix issue #3651.

Hi Senthil.

Please could you quote the issue's Subject when you quote its issue
number in a log message or in an email subject line.  That would make
life easier for me.  Thanks.


> * subversion/svn/copy-cmd.c
>   (svn_cl__copy): Eat peg revisions in copy target paths.
> ]]]
> 
> Index: subversion/svn/copy-cmd.c
> ===================================================================
> --- subversion/svn/copy-cmd.c	(revision 952908)
> +++ subversion/svn/copy-cmd.c	(working copy)
> @@ -77,6 +77,8 @@
>        APR_ARRAY_PUSH(sources, svn_client_copy_source_t *) = source;
>      }
>  
> +  SVN_ERR(svn_opt_eat_peg_revisions(&targets, targets, pool));
> +

This fixes one missed case of the fix that was applied for all other
commands in r878062 for issue #3416 "Cannot add or commit
'dir/@file.txt'".

This reminds me that there is more to do.

Use of svn_opt_eat_peg_revisions() does indeed fix the cases where
"@HEAD" or "@@" is present on the command line, which is good.  However,
if the user specifies a non-head revision:

  svn copy X $repos/trunk/oldfile@5

that should be an error (unless, perhaps, the latest revision of oldfile
is r5) but this patch hides the error.

The API svn_opt_eat_peg_revisions() checks that any supplied revision
peg specifier is syntactically valid, but it never gives the caller the
opportunity to check that the peg makes sense semantically.

For this reason, we should change all callers of
svn_opt_eat_peg_revisions() to parse and ignore "@@" endings (like
eat_peg_revisions() does) but also to throw an error if any non-empty
peg revision is specified (except perhaps HEAD, where that makes sense
for the particular command).  For finer grained control, the caller
could use svn_opt_parse_path() instead, and should then verify that the
peg rev is either unspecified or a semantically acceptable value.

Then, the public name "svn_opt_eat_peg_revisions()" can be removed, as
it is still "new in 1.7", and the private name
"svn_opt_eat_peg_revisions()" can be deprecated or perhaps removed,
depending on 1.6-compatibility requirements.

- Julian


>    /* Figure out which type of trace editor to use.
>       If the src_paths are not homogeneous, setup_copy will return an error. */
>    src_path = APR_ARRAY_IDX(targets, 0, const char *);

Re: Eating peg revisions [was: [PATCH] Fix issue #3651 - "svn copy does not eat peg revision within copy target path"]

[Senthil Kumaran S <se...@collab.net>]

Hi Julian,

Julian Foad wrote:
>> [[[
>> Fix issue #3651.
> 
> Hi Senthil.
> 
> Please could you quote the issue's Subject when you quote its issue
> number in a log message or in an email subject line.  That would make
> life easier for me.  Thanks.

I did that in the original commit log message. Will make a note of it in future
to include issue Subject in all messages.

Thank You.
-- 
Senthil Kumaran S
http://www.stylesen.org/