You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by Hyrum K Wright <hy...@wandisco.com> on 2012/05/01 03:37:43 UTC
Re: svn commit: r1331652 - /subversion/trunk/subversion/libsvn_delta/compat.c
On Fri, Apr 27, 2012 at 8:50 PM, <gs...@apache.org> wrote:
> Author: gstein
> Date: Sat Apr 28 01:50:35 2012
> New Revision: 1331652
>
> URL: http://svn.apache.org/viewvc?rev=1331652&view=rev
> Log:
> For the Ev2 shims:
>
> Revamp the Ev2/Ev1 shims by using struct change_node rather than the
> funky "operation" records and the build() function. Each Ev2 callback
> simply inserts a new change_node structure with some values in
> it. Later, we use the path_driver to apply these change_node
> structures to all modified paths.
>
> This obsoletes huge chunks of code; to be removed in a future revision.
>
> Test failures: log 38, merge 105.
>
> * subversion/libsvn_delta/compat.c:
> (process_actions): remove an incorrect assertion
> (struct editor_baton): add a CHANGES hash
> (insert_change): new helper function, similar to locate_change()
> (add_directory_cb, add_file_cb, add_absent_cb, delete_cb): add code
> blocks for setting up change records
> (add_symlink_cb): #if this whole thing out, and add SVN__NOT_IMPLEMENTED()
> (alter_directory_cb, alter_file_cb): set up change records, noting
> that the record could have been created by an earlier copy/move.
> (copy_cb): set up change records. use the FETCH_KIND_FUNC callback
> to fetch kind information about the source (so we know what Ev1
> function to invoke for the destination)
> (move_cb): set up a change record for the source/deletion side, and
> the destination/copy side. use the FETCH_KIND_FUNC callback.
> (drive_ev1_props): helpful utility function to drive a series of
> single property changes (adds, modifies, deletes) to the Ev1
> editor. this helper also manages the special "unlock" mechanism
> (apply_change): apply all changes described in a change_node
> structure to a specified node
> (drive_changes): if any changes have been made (eg. start_edit_func
> was called an the root baton opened), then prepare a list of paths
> for the path_driver, and then run the sucker to make all the
> necessary edits. we have some special sneakiness to ensure the
> path_driver doesn't call open_root() a second time.
> (complete_cb, abort_cb): use drive_changes() rather than drive_root()
> (start_edit_func): leave a note about early-open of the root
> (do_unlock): record UNLOCK in the change_node. this will be handled
> by drive_ev1_props() later.
> (svn_delta__editor_from_delta): create the new CHANGES hash
>
> Modified:
> subversion/trunk/subversion/libsvn_delta/compat.c
>
> Modified: subversion/trunk/subversion/libsvn_delta/compat.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_delta/compat.c?rev=1331652&r1=1331651&r2=1331652&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_delta/compat.c (original)
> +++ subversion/trunk/subversion/libsvn_delta/compat.c Sat Apr 28 01:50:35 2012
> @@ -434,8 +434,12 @@ process_actions(struct ev2_edit_baton *e
> if (props || contents)
> {
> /* Changes to properties or content should have indicated the revision
> - it was intending to change. */
> + it was intending to change.
> +
> + Oop. Not true. The node may be locally-added. */
> +#if 0
> SVN_ERR_ASSERT(SVN_IS_VALID_REVNUM(change->changing));
> +#endif
>
> if (kind == svn_kind_dir)
> SVN_ERR(svn_editor_alter_directory(eb->editor, repos_relpath,
> @@ -1042,10 +1046,38 @@ struct editor_baton
> const char *repos_root;
> const char *base_relpath;
>
> + /* REPOS_RELPATH -> struct change_node * */
> + apr_hash_t *changes;
> +
> apr_pool_t *edit_pool;
> };
>
>
> +/* Insert a new change for RELPATH, or return an existing one. */
> +static struct change_node *
> +insert_change(const char *relpath,
> + apr_hash_t *changes)
> +{
> + apr_pool_t *result_pool;
> + struct change_node *change;
> +
> + change = apr_hash_get(changes, relpath, APR_HASH_KEY_STRING);
> + if (change != NULL)
> + return change;
> +
> + result_pool = apr_hash_pool_get(changes);
> +
> + /* Return an empty change. Callers will tweak as needed. */
> + change = apr_pcalloc(result_pool, sizeof(*change));
> + change->changing = SVN_INVALID_REVNUM;
> + change->deleting = SVN_INVALID_REVNUM;
> +
> + apr_hash_set(changes, relpath, APR_HASH_KEY_STRING, change);
As the key of the hash, RELPATH should be duplicated into a pool with
sufficient lifetime.
>...
-Hyrum
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com/
Re: svn commit: r1331652 - /subversion/trunk/subversion/libsvn_delta/compat.c
Posted by Greg Stein <gs...@gmail.com>.
On Mon, Apr 30, 2012 at 23:31, Hyrum K Wright <hy...@wandisco.com> wrote:
> On Mon, Apr 30, 2012 at 10:02 PM, Greg Stein <gs...@gmail.com> wrote:
>>
>> On Apr 30, 2012 9:38 PM, "Hyrum K Wright" <hy...@wandisco.com> wrote:
>>>...
>>> As the key of the hash, RELPATH should be duplicated into a pool with
>>> sufficient lifetime.
>>
>> Good catch! Fixed in r1332508.
>
> Thanks. For the same reason, you probably want to
> s/apr_hash_copy/svn_prop_hash_dup/ over compat.c. The former only
> duplicates the hash, not the keys or the contents, while the latter
> will ensure the prophash has the same lifetime as the change it
> belongs to.
Oop! You're right. Will do after I commit my work-in-progress.
Thx,
-g
Re: svn commit: r1331652 - /subversion/trunk/subversion/libsvn_delta/compat.c
Posted by Hyrum K Wright <hy...@wandisco.com>.
On Mon, Apr 30, 2012 at 10:02 PM, Greg Stein <gs...@gmail.com> wrote:
>
> On Apr 30, 2012 9:38 PM, "Hyrum K Wright" <hy...@wandisco.com> wrote:
>>...
>
>
>> > +/* Insert a new change for RELPATH, or return an existing one. */
>> > +static struct change_node *
>> > +insert_change(const char *relpath,
>> > + apr_hash_t *changes)
>> > +{
>> > + apr_pool_t *result_pool;
>> > + struct change_node *change;
>> > +
>> > + change = apr_hash_get(changes, relpath, APR_HASH_KEY_STRING);
>> > + if (change != NULL)
>> > + return change;
>> > +
>> > + result_pool = apr_hash_pool_get(changes);
>> > +
>> > + /* Return an empty change. Callers will tweak as needed. */
>> > + change = apr_pcalloc(result_pool, sizeof(*change));
>> > + change->changing = SVN_INVALID_REVNUM;
>> > + change->deleting = SVN_INVALID_REVNUM;
>> > +
>> > + apr_hash_set(changes, relpath, APR_HASH_KEY_STRING, change);
>>
>> As the key of the hash, RELPATH should be duplicated into a pool with
>> sufficient lifetime.
>
> Good catch! Fixed in r1332508.
Thanks. For the same reason, you probably want to
s/apr_hash_copy/svn_prop_hash_dup/ over compat.c. The former only
duplicates the hash, not the keys or the contents, while the latter
will ensure the prophash has the same lifetime as the change it
belongs to.
-Hyrum
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com/
Re: svn commit: r1331652 - /subversion/trunk/subversion/libsvn_delta/compat.c
Posted by Hyrum K Wright <hy...@wandisco.com>.
On Mon, Apr 30, 2012 at 10:02 PM, Greg Stein <gs...@gmail.com> wrote:
>
> On Apr 30, 2012 9:38 PM, "Hyrum K Wright" <hy...@wandisco.com> wrote:
>>...
>
>
>> > +/* Insert a new change for RELPATH, or return an existing one. */
>> > +static struct change_node *
>> > +insert_change(const char *relpath,
>> > + apr_hash_t *changes)
>> > +{
>> > + apr_pool_t *result_pool;
>> > + struct change_node *change;
>> > +
>> > + change = apr_hash_get(changes, relpath, APR_HASH_KEY_STRING);
>> > + if (change != NULL)
>> > + return change;
>> > +
>> > + result_pool = apr_hash_pool_get(changes);
>> > +
>> > + /* Return an empty change. Callers will tweak as needed. */
>> > + change = apr_pcalloc(result_pool, sizeof(*change));
>> > + change->changing = SVN_INVALID_REVNUM;
>> > + change->deleting = SVN_INVALID_REVNUM;
>> > +
>> > + apr_hash_set(changes, relpath, APR_HASH_KEY_STRING, change);
>>
>> As the key of the hash, RELPATH should be duplicated into a pool with
>> sufficient lifetime.
>
> Good catch! Fixed in r1332508.
Thanks. For the same reason, you probably want to
s/apr_hash_copy/svn_prop_hash_dup/ over compat.c. The former only
duplicates the hash, not the keys or the contents, while the latter
will ensure the prophash has the same lifetime as the change it
belongs to.
-Hyrum
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com/
Re: svn commit: r1331652 - /subversion/trunk/subversion/libsvn_delta/compat.c
Posted by Greg Stein <gs...@gmail.com>.
On Apr 30, 2012 9:38 PM, "Hyrum K Wright" <hy...@wandisco.com> wrote:
>...
> > +/* Insert a new change for RELPATH, or return an existing one. */
> > +static struct change_node *
> > +insert_change(const char *relpath,
> > + apr_hash_t *changes)
> > +{
> > + apr_pool_t *result_pool;
> > + struct change_node *change;
> > +
> > + change = apr_hash_get(changes, relpath, APR_HASH_KEY_STRING);
> > + if (change != NULL)
> > + return change;
> > +
> > + result_pool = apr_hash_pool_get(changes);
> > +
> > + /* Return an empty change. Callers will tweak as needed. */
> > + change = apr_pcalloc(result_pool, sizeof(*change));
> > + change->changing = SVN_INVALID_REVNUM;
> > + change->deleting = SVN_INVALID_REVNUM;
> > +
> > + apr_hash_set(changes, relpath, APR_HASH_KEY_STRING, change);
>
> As the key of the hash, RELPATH should be duplicated into a pool with
> sufficient lifetime.
Good catch! Fixed in r1332508.
Re: svn commit: r1331652 - /subversion/trunk/subversion/libsvn_delta/compat.c
Posted by Greg Stein <gs...@gmail.com>.
On Apr 30, 2012 9:38 PM, "Hyrum K Wright" <hy...@wandisco.com> wrote:
>...
> > +/* Insert a new change for RELPATH, or return an existing one. */
> > +static struct change_node *
> > +insert_change(const char *relpath,
> > + apr_hash_t *changes)
> > +{
> > + apr_pool_t *result_pool;
> > + struct change_node *change;
> > +
> > + change = apr_hash_get(changes, relpath, APR_HASH_KEY_STRING);
> > + if (change != NULL)
> > + return change;
> > +
> > + result_pool = apr_hash_pool_get(changes);
> > +
> > + /* Return an empty change. Callers will tweak as needed. */
> > + change = apr_pcalloc(result_pool, sizeof(*change));
> > + change->changing = SVN_INVALID_REVNUM;
> > + change->deleting = SVN_INVALID_REVNUM;
> > +
> > + apr_hash_set(changes, relpath, APR_HASH_KEY_STRING, change);
>
> As the key of the hash, RELPATH should be duplicated into a pool with
> sufficient lifetime.
Good catch! Fixed in r1332508.