You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <da...@elego.de> on 2012/05/15 21:50:08 UTC
Re: svn commit: r1338810 -
/subversion/trunk/subversion/libsvn_repos/replay.c
cmpilato@apache.org wrote on Tue, May 15, 2012 at 17:57:36 -0000:
> Author: cmpilato
> Date: Tue May 15 17:57:36 2012
> New Revision: 1338810
>
> URL: http://svn.apache.org/viewvc?rev=1338810&view=rev
> Log:
> Fix issue #4184 ("partial svnsync drops properties when converting
> copies to adds"). Again.
>
> If we downgrade a copy to a plain add, forcibly dump the full proplist
> of the added thing across the wire, too.
>
> * subversion/libsvn_repos/replay.c
> (path_driver_cb_func): Transmit props when the CHANGE indicates we
> should *and* when we've downgraded a copy to a raw add.
>
> Modified:
> subversion/trunk/subversion/libsvn_repos/replay.c
>
> Modified: subversion/trunk/subversion/libsvn_repos/replay.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/replay.c?rev=1338810&r1=1338809&r2=1338810&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_repos/replay.c (original)
> +++ subversion/trunk/subversion/libsvn_repos/replay.c Tue May 15 17:57:36 2012
> @@ -698,10 +698,15 @@ path_driver_cb_func(void **dir_baton,
> }
> }
>
> - /* Handle property modifications. */
> if (! do_delete || do_add)
> {
> - if (change->prop_mod)
> + /* Handle property modifications.
> +
> + Note that this needs to happen in the "copy from a file or
> + directory we aren't allowed to see" case since otherwise the
> + caller will have no way to actually get those properties
> + which they are apparently allowed to see. */
> + if (change->prop_mod || (change->copyfrom_path && ! copyfrom_path))
Why is change->copyfrom_path initialized?
You don't check change->copyfrom_known, and fill_copyfrom() is called in
the 'do_add' case but not in the '! do_delete && ! do_add' case.
And, BTW, did you revert the docstring patch to fill_copyfrom() part of
r1338803? I think it should stay.
> {
> apr_array_header_t *prop_diffs;
> apr_hash_t *old_props;
>
>
Re: svn commit: r1338810 - /subversion/trunk/subversion/libsvn_repos/replay.c
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 05/15/2012 03:50 PM, Daniel Shahaf wrote:
>> }
>> }
>>
>> - /* Handle property modifications. */
>> if (! do_delete || do_add)
>> {
>> - if (change->prop_mod)
>> + /* Handle property modifications.
>> +
>> + Note that this needs to happen in the "copy from a file or
>> + directory we aren't allowed to see" case since otherwise the
>> + caller will have no way to actually get those properties
>> + which they are apparently allowed to see. */
>> + if (change->prop_mod || (change->copyfrom_path && ! copyfrom_path))
>
> Why is change->copyfrom_path initialized?
>
> You don't check change->copyfrom_known, and fill_copyfrom() is called in
> the 'do_add' case but not in the '! do_delete && ! do_add' case.
I copied code from the /* Handle textual modifications */ block just below
this. If there's a problem with the copyfrom initialization here, it
predates my edits.
My guess is that this code has worked fine because while
change->copyfrom_path may not be "valid" (in the semantic,
information-carrying sense) it is also not filled with random junk. FSFS
always fills it with valid stuff; BDB initializes the whole CHANGE structure
with zeroes. Still, we clearly need to locally ensure that the bits are
valid. I'll do so in a subsequent commit.
> And, BTW, did you revert the docstring patch to fill_copyfrom() part of
> r1338803? I think it should stay.
I re-added it in r1339154. Thanks for pointing that out.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development
Re: svn commit: r1338810 - /subversion/trunk/subversion/libsvn_repos/replay.c
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 05/15/2012 03:50 PM, Daniel Shahaf wrote:
>> }
>> }
>>
>> - /* Handle property modifications. */
>> if (! do_delete || do_add)
>> {
>> - if (change->prop_mod)
>> + /* Handle property modifications.
>> +
>> + Note that this needs to happen in the "copy from a file or
>> + directory we aren't allowed to see" case since otherwise the
>> + caller will have no way to actually get those properties
>> + which they are apparently allowed to see. */
>> + if (change->prop_mod || (change->copyfrom_path && ! copyfrom_path))
>
> Why is change->copyfrom_path initialized?
>
> You don't check change->copyfrom_known, and fill_copyfrom() is called in
> the 'do_add' case but not in the '! do_delete && ! do_add' case.
I copied code from the /* Handle textual modifications */ block just below
this. If there's a problem with the copyfrom initialization here, it
predates my edits.
My guess is that this code has worked fine because while
change->copyfrom_path may not be "valid" (in the semantic,
information-carrying sense) it is also not filled with random junk. FSFS
always fills it with valid stuff; BDB initializes the whole CHANGE structure
with zeroes. Still, we clearly need to locally ensure that the bits are
valid. I'll do so in a subsequent commit.
> And, BTW, did you revert the docstring patch to fill_copyfrom() part of
> r1338803? I think it should stay.
I re-added it in r1339154. Thanks for pointing that out.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development