Karl Fogel wrote:
> vgeorgescu@tigris.org writes:
>> Log:
>> Add depth support to svn_client_proplist3.
>>
>> [...]
>
> Yay!
>
> A few comments below...
>
> [...]
>> + * If @a depth is @c svn_depth_empty, list only the properties of
>> + * @a path_or_url itself. If @a depth is @c svn_depth_files, and
>> + * @a path_or_url is a directory, list the properties of @a path_or_url
>> + * and its file entries. If @c svn_depth_immediates, list the properties
>> + * of its immediate file and directory entries. If @c svn_depth_infinity,
>> + * list the properties of its file entries and recurse (with
>> + * @c svn_depth_infinity) on directory entries.
>> *
>> * If @a target is not found, return the error @c SVN_ERR_ENTRY_NOT_FOUND.
>> *
>
> Should we also document the behavior for svn_depth_unknown? Or at
> least document that the behavior for depths not mentioned here is
> undefined? I think we haven't been consistent about doing that
> elsewhere, but that might just mean those other places should be fixed
> too :-).
I documented the meaning of svn_depth_unknown in r25033, and that all
other depths produce undefined behavior.
> [...]
>> ==============================================================================
>> --- trunk/subversion/libsvn_client/prop_commands.c (original)
>> +++ trunk/subversion/libsvn_client/prop_commands.c Sun May 13 10:10:40 2007
>> @@ -931,8 +932,9 @@
>>
>> if (kind == svn_node_dir)
>> {
>> - SVN_ERR(svn_ra_get_dir2(ra_session, (recurse ? &dirents : NULL), NULL,
>> - &prop_hash, target_relative, revnum,
>> + SVN_ERR(svn_ra_get_dir2(ra_session,
>> + (depth > svn_depth_empty) ? &dirents : NULL,
>> + NULL, &prop_hash, target_relative, revnum,
>> SVN_DIRENT_KIND, scratchpool));
>> }
>> else if (kind == svn_node_file)
>
> I'm not sure we have any other code that depends on numeric comparison
> of two depths; it encodes assumptions about the depths' values that
> may not hold in the future. We can't predict what new depths we might
> add, or what their interpretations might be relative to existing
> depths.
Hmm, I really like comparing them directly. It's much more readable and
less error-prone than using multiple equality comparisons. Lieven even
documented that the numbering is stable so that this trick could be used.
> (We could also make a macro if this idiom occurs often enough, but I'm
> not sure it does.)
>
We could, but how likely are we to introduce new depths? I don't think
it makes sense to introduce any new depths between 'empty' and 'files'
or between 'files' and 'immediates', and if you're worried about
introducing new depths between 'immediates' and 'infinity', then we can
just set svn_depth_infinity to a large value now, to prevent any
problems in the future.
> [...]
>> @@ -1104,16 +1119,22 @@
>>
>> SVN_ERR(remote_proplist(url, "",
>> kind, revnum, ra_session,
>> - recurse, receiver, receiver_baton,
>> + depth, receiver, receiver_baton,
>> pool, subpool));
>> svn_pool_destroy(subpool);
>> }
>> else /* working copy path */
>> {
>> svn_boolean_t pristine;
>> + int adm_depth = -1;
>> +
>> + if (depth == svn_depth_empty || depth == svn_depth_files)
>> + adm_depth = 0;
>> + else if (depth == svn_depth_immediates)
>> + adm_depth = 1;
>
> It might be good to have a comment explaining that "int adm_depth = -1;"
> is the correct initialization for svn_depth_infinity. I had to look
> twice before I realized what was going on.
Done in r25033.
> [...]
>> + apr_hash_t *entries;
>> + apr_hash_index_t *hi;
>> + apr_pool_t *iterpool = svn_pool_create(pool);
>> +
>> + SVN_ERR(svn_wc_entries_read(&entries, adm_access, FALSE, pool));
>> + for (hi = apr_hash_first(pool, entries); hi; hi = apr_hash_next(hi))
>> + {
>> + const void *key;
>> + void *val;
>> + const svn_wc_entry_t *current_entry;
>> + const char *path;
>> +
>> + apr_hash_this(hi, &key, NULL, &val);
>> + current_entry = val;
>> +
>> + /* We already handled THIS_DIR. */
>> + if (strcmp(key, SVN_WC_ENTRY_THIS_DIR) == 0)
>> + continue;
>> +
>> + path = svn_path_join(target, key, iterpool);
>> +
>> + if (current_entry->kind == svn_node_file
>> + || depth == svn_depth_immediates)
>> + {
>> + /* Ignore the entry if it does not exist at the time
>> + of interest. */
>> + if (current_entry->schedule
>> + == (pristine ? svn_wc_schedule_add
>> + : svn_wc_schedule_delete))
>> + continue;
>> +
>> + SVN_ERR(pristine_or_working_props(&hash, path,
>> + adm_access,
>> + pristine, iterpool));
>> + SVN_ERR(call_receiver(path, hash, receiver,
>> + receiver_baton, iterpool));
>> + }
>> + }
>> + }
>> }
>
> You declared an 'iterpool', but never clear or destroy it.
I already took care of this in r25008-9.
>
>> SVN_ERR(svn_wc_adm_close(adm_access));
>> @@ -1210,7 +1275,8 @@
>> pl_baton.props = *props;
>> pl_baton.pool = pool;
>>
>> - SVN_ERR(svn_client_proplist3(target, peg_revision, revision, recurse,
>> + SVN_ERR(svn_client_proplist3(target, peg_revision, revision,
>> + recurse ? svn_depth_infinity : svn_depth_empty,
>> proplist_receiver_cb, &pl_baton, ctx, pool));
>
> Ah, right, we can't use the SVN_DEPTH_FROM_RECURSE() macro here,
> because proplist3() promises a slightly different behavior than most
> things. Makes sense. Might be worth a comment, though.
Also done in r25033.
> Bravo for taking care of this API, Vlad! I hope the above comments help.
>
Thanks for the review!
--
Vlad
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue May 15 23:06:49 2007