On Tue, Sep 29, 2009 at 8:57 AM, Stefan Sperling <stsp_at_elego.de> wrote:
> Dave,
>
> thanks for the patch. Some comments below.
>
> On Fri, Sep 25, 2009 at 11:22:25AM -0700, Dave Brown wrote:
>> The added/deleted accessors are not perfect, they don't go through
>> all the logic that entries.c does to populate entry->schedule. I
>> kept wanting to look at svn_wc__de_status_t values. But the accessors
>> suffice for export.c use.
>
> I think the "skip export" logic is too simplistic. E.g. it does not
> consider schedule-replace nodes. However this problem has existed
> before your patch, and can be fixed in another patch.
> We should list all possible states a node can be in and define skipping
> behaviour for each case, then update your skip_export_node() function.
>
>> [[[
>> Remove wc_entry_t usage from export.c. Make a couple of accessor api's
>> in svn_wc_private.h to fetch from wc_db the equivalent of the old test
>> entry->schedule == (schdule_add,schedule_delete).
>
> s/schdule/schedule/
>
>> * include/private/svn_wc_private.h:
>> (svn_wc__node_is_delete, svn_wc__node_is_added): New functions
>>
>> * libsvn_wc/node.c:
>> (svn_wc__node_is_delete, svn_wc__node_is_added): New functions
>>
>> * libsvn_client/export.c:
>> (skip_export_node): add static helper function to decide if a node
>> should be exported or not
>
> You can indent additional lines in each (...) entry by one or two spaces.
>
>> (copy_one_versioned_file,copy_versioned_files): Remove wc_entry_t
>> usage. Use svn_wc_private.h api's to fetch specific information:
>> url,depth,added or deleted status.
>> ]]]
>>
>> ------------------------------------------------------
>> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2400350
>
>> Index: subversion/include/private/svn_wc_private.h
>> ===================================================================
>> --- subversion/include/private/svn_wc_private.h (revision 39608)
>> +++ subversion/include/private/svn_wc_private.h (working copy)
>> @@ -434,6 +434,17 @@
>> void *cancel_baton,
>> apr_pool_t *scratch_pool);
>>
>> +/** Equivalent to the old notion of "entry->schedule == schedule_delete" */
>> +svn_error_t *
>> +svn_wc__node_is_delete(svn_boolean_t *deleted, svn_wc_context_t *wc_ctx,
>> + const char *abspath, apr_pool_t *scratch_pool);
>
> This should be called "svn_wc__node_is_deleted" (past tense).
> There is a function with that name in entries.c already.
> The existing function expects a wc_db rather than a wc_ctx.
> and has a comment saying that it wants to be updated to wc_db.
>
> Looks like your new function is almost a drop-in replacement.
Hi Stefan,
libsvn_wc/entries.c:svn_wc__node_is_deleted() is actually asking about
the 'deleted' member of svn_wc_entry_t not the 'schedule' member.
entry->deleted and entry->schedule == svn_wc_schedule_delete are
different beasts:
/** The directory containing this entry had a versioned child of this
* name, but this entry represents a different revision or a switched
* path at which no item exists in the repository. This typically
* arises from committing or updating to a deletion of this entry
* without committing or updating the parent directory.
*
* The schedule can be 'normal' or 'add'. */
svn_boolean_t deleted;
/** scheduling (add, delete, replace ...) */
svn_wc_schedule_t schedule;
Paul
> If we cannot remove the existing function because it has too many users
> internal to libsvn_wc relying on the wc_db parameter, we should update
> the existing function with your code and provide a thin wrapper that takes
> a wc_ctx for use by libsvn_client, with a different name (but more different
> than just a single character).
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2402191
Received on 2009-09-30 19:26:08 CEST