On Feb 12, 2010, at 8:10 AM, Greg Stein wrote:
> On Fri, Feb 12, 2010 at 10:39, Matthew Bentham <mj...@artvps.com> wrote:
>> I understand from thread "WC-NG: How can we help with 1.7-readiness?" that
>> this sort of thing is helpful. I am a massive newbie, please forgive me if
>> I do something obviously wrong.
>
> Can't learn without trying! Thanks for jumping in :-)
>
>> [[[
>> wc-ng: work towards eliminating svn_wc_entry_t
>>
>> * subversion/libsvn_client/commit_util.c
>> (add_lock_token): Replace a use of svn_wc__maybe_get_entry with
>> use of svn_wc__node_get_*
>> ]]]
>
> For log messages, we also like to provide attribution, so at the end
> of the above message you would have:
>
> Patch by: Matthew Bentham <mjb67{_AT_}artvps.com>
>
>> Index: subversion/libsvn_client/commit_util.c
>> ===================================================================
>> --- subversion/libsvn_client/commit_util.c (revision 909397)
>> +++ subversion/libsvn_client/commit_util.c (working copy)
>> @@ -195,19 +195,21 @@
>> {
>> struct add_lock_token_baton *altb = walk_baton;
>> apr_pool_t *token_pool = apr_hash_pool_get(altb->lock_tokens);
>> - const svn_wc_entry_t *entry;
>> + const char* lock_token = NULL;
>> + const char* url = NULL;
>
> No need to initialize these to NULL. The node functions will always
> set the value, unless it returns an error (in which case, you don't
> care about the values).
>
>> + SVN_ERR(svn_wc__node_get_url(&url, altb->wc_ctx, local_abspath,
>> + scratch_pool, scratch_pool));
>> + SVN_ERR(svn_wc__node_get_lock_token(&lock_token, altb->wc_ctx,
>> + local_abspath, scratch_pool, scratch_pool));
>>
>> - SVN_ERR(svn_wc__maybe_get_entry(&entry, altb->wc_ctx, local_abspath,
>> - svn_node_unknown, FALSE, FALSE,
>> - scratch_pool, scratch_pool));
>> -
>> /* I want every lock-token I can get my dirty hands on!
>> If this entry is switched, so what. We will send an irrelevant lock
>> token. */
>> - if (entry && entry->url && entry->lock_token)
>> - apr_hash_set(altb->lock_tokens, apr_pstrdup(token_pool, entry->url),
>> + if (url && lock_token)
>> + apr_hash_set(altb->lock_tokens, apr_pstrdup(token_pool, url),
>> APR_HASH_KEY_STRING,
>> - apr_pstrdup(token_pool, entry->lock_token));
>> + apr_pstrdup(token_pool, lock_token));
>
> Rather than dup'ing into the token_pool, you could pass that pool as
> the result_pool to the node functions.
>
> However, I'd go ahead and suggest leaving it as above so that
> token_pool won't grow if only one of the values is present.
>
> Hmm. I would also rejigger things a bit while you're at it: fetch the
> lock token first, and if NULL, then early-exit from the function. The
> URL can usually be derived, so will be present quite often. The lock
> token is the more obvious discriminator. Also, when you fetch the URL,
> you'll be able to use token_pool for the call's result_pool. Does that
> all make sense?
FWIW, the original patch passes 'make check'.
-Hyrum
