Re: [PATCH v4 1/6] revision: add mark_tree_uninteresting_sparse
- From
Derrick Stolee <stolee@gmail.com>
- Date
- Jan 11, 2019, 22:05 UTC
- Message-ID
- <5a25813e-32f6-34a4-9f24-b65415b75dda@gmail.com>
- In-Reply-To
- <xmqqva2vx7p2.fsf@gitster-ct.c.googlers.com>
On 1/11/2019 3:25 PM, Junio C Hamano wrote:
Show 36 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
> So, I assumed that the implementation was wrong, but it is the other
> way around. You do mean to pick only already uninteresting trees
> out of "set" and mark its reachables.
>
> One thing that would make me worried is what help the callers of
> this function will get (or they may have to devise the way
> themselves) to avoid having to traverse the same tree number of
> times. A tree can be made uninteresting after a traversal of
> another tree that contains it, but the logic in this function
>
>> + if (tree->object.flags & UNINTERESTING) {
>> + /*
>> + * Remove the flag so the next call
>> + * is not a no-op. The flag is added
>> + * in mark_tree_unintersting().
>> + */
>> + tree->object.flags ^= UNINTERESTING;
>> + mark_tree_uninteresting(r, tree);
>> + }
> ignores the fact that it is already UNINTERESTING (in fact, in a
> sense it is even worse---it cannot be used to make a not-yet
> UNINTERESTING one into UNINTERESTING), drops the UNINTERESING bit
> and forces the traversal of that tree. The only thing I see that
> would work as a saving grace is that mark_tree_uninteresting()
> itself would honor existing UNINTERESING bit and refuses to recurse
> into its subtrees, but that means blobs at the top-level of such a
> tree would be marked UNINTERESING while those in its subtrees can be
> left not-UNINTERESING, which sounds like inviting a mess.
>
> It does *not* immediately mean this function is misdesigned. It
> just means that the caller needs to carefully follow whatever
> calling convention this series will establish in the later patches
> (which we haven't seen yet at this point).
>I'm sorry that this implementation is particularly confusing. It is created only as a placeholder so we can wire up the --sparse option to use this method without being "wrong" but is then removed entirely in PATCH 4/6:
...
void mark_trees_uninteresting_sparse(struct repository *r,
struct oidset *set)
{
+ unsigned has_interesting = 0, has_uninteresting = 0;
+ struct hashmap map;
+ struct hashmap_iter map_iter;
+ struct path_and_oids_entry *entry;
struct object_id *oid;
struct oidset_iter iter;
oidset_iter_init(set, &iter);
- while ((oid = oidset_iter_next(&iter))) {
+ while ((!has_interesting || !has_uninteresting) &&
+ (oid = oidset_iter_next(&iter))) {
struct tree *tree = lookup_tree(r, oid);
if (!tree)
continue;
- if (tree->object.flags & UNINTERESTING) {
- /*
- * Remove the flag so the next call
- * is not a no-op. The flag is added
- * in mark_tree_unintersting().
- */
- tree->object.flags ^= UNINTERESTING;
- mark_tree_uninteresting(r, tree);
- }
+ if (tree->object.flags & UNINTERESTING)
+ has_uninteresting = 1;
+ else
+ has_interesting = 1;
+ }
...You are definitely correct that "set" is not a valuable variable name. It could instead be "tree_oids" to be slightly more informative.
Thanks, -Stolee