Re: [PATCH] fix possible bug in handling of peg revisions in 'svn copy'

Stefan Sperling wrote:
> On Sun, Nov 04, 2007 at 02:39:23PM +0000, Barry Scott wrote:
>> I'm reading svn_cl__copy() in attempt to understand how to call
>> svn_client_copy4().
>>
>> Isn't it a bug that the peg_revision is not allocated from the pool?
>> The value of peg_revision is overwritten each time around the loop.
>
> Looks very strange indeed.
>
> You mean this bit, right?
>
> /* Get the src list and associated peg revs */
> sources = apr_array_make(pool, targets->nelts - 1,
> sizeof(svn_client_copy_source_t *));
> for (i = 0; i < (targets->nelts - 1); i++)
> {
> const char *target = ((const char **) (targets->elts))[i];
> svn_client_copy_source_t *source = apr_palloc(pool, sizeof(*source));
> const char *src;
> svn_opt_revision_t peg_revision;
>
> SVN_ERR(svn_opt_parse_path(&peg_revision, &src, target, pool));
> source->path = src;
> source->revision = &(opt_state->start_revision);
> source->peg_revision = &peg_revision;
>
> APR_ARRAY_PUSH(sources, svn_client_copy_source_t *) = source;
> }
>
> As I read it when this loop is done all sources will have the same
> peg revision, namely the one of the last source in the list.
>
> This does not look as if it was intended, especially given
> that the comment talks about multiple peg revisions.
>
> I think this is the diff Barry has expressed in plain English:
>
> Index: subversion/svn/copy-cmd.c
> ===================================================================
> --- subversion/svn/copy-cmd.c (revision 27585)
> +++ subversion/svn/copy-cmd.c (working copy)
> @@ -60,12 +60,12 @@
> const char *target = APR_ARRAY_IDX(targets, i, const char *);
> svn_client_copy_source_t *source = apr_palloc(pool, sizeof(*source));
> const char *src;
> - svn_opt_revision_t peg_revision;
> + svn_opt_revision_t *peg_revision = apr_palloc(pool, sizeof(*peg_revision));
>
> - SVN_ERR(svn_opt_parse_path(&peg_revision, &src, target, pool));
> + SVN_ERR(svn_opt_parse_path(peg_revision, &src, target, pool));
> source->path = src;
> source->revision = &(opt_state->start_revision);
> - source->peg_revision = &peg_revision;
> + source->peg_revision = peg_revision;
>
> APR_ARRAY_PUSH(sources, svn_client_copy_source_t *) = source;
> }

Barry, thanks for catching this, and Stefan, thanks for the patch!

I wrote a log message and committed this r27593. In the future, it
would be helpful to have a log message accompanying the patch when it
gets sent to the list. (Not a big deal this time, just reminding you
for the future.)

Thanks again,
-Hyrum