Hyrum K. Wright wrote:
> On Feb 12, 2010, at 8:10 AM, Greg Stein wrote:
>
>> On Fri, Feb 12, 2010 at 10:39, Matthew Bentham <mjb67_at_artvps.com> wrote:
>>> 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'.
>
That's worth quite a bit actually, I haven't been able to get it to pass
'make check' after implementing Greg's suggestions. It fails one of the
tests in lock_tests.py. How do you guys generally run an individual
test or test program? It's implied in the documentation that the test
programs support it, but does 'make check' have any options?
Matthew
Received on 2010-02-15 11:28:47 CET