Re: revision: propagate flag bits from tags to pointees

[Junio C Hamano <gitster@pobox.com>]

Junio C Hamano <gitster@pobox.com> writes:

> But I have a suspicion that my patch may break if any codepath looks
> at the current flag on the object and decides "ah, it already is
> marked" and punts.
>
> It indeed looks like mark_tree_uninteresting() does have that
> property.  When an uninteresting tag directly points at a tree, if
> we propagate the UNINTERESTING bit to the pointee while peeling,
> wouldn't we end up calling mark_tree_uninteresting() on a tree,
> whose flags already have UNINTERESTING bit set, causing it not to
> recurse?

Extending that line of thought further, what should this do?

    git rev-list --objects ^HEAD^^{tree} HEAD^{tree} |
    git pack-object --stdin pack

It says "I am interested in the objects that is used in the tree of
HEAD, but I do not need those that already appear in HEAD^".

With the current code (with or without the fix under discussion, or
even without the faulty "do not peel tags used in range notation"),
the tree of the HEAD^ is marked in handle_revision_arg() as
UNINTERESTING when it is placed in revs->pending.objects[], and the
handle_commit() --- we should rename it to handle_pending_object()
or something, by the way --- will call mark_tree_uninteresting() on
that tree, which then would say "It is already uninteresting" and
return without marking the objects common to these two trees
uninteresting, no?

I think that is a related but separate bug that dates back to
prehistoric times, and the asymmetry between handle_commit() deals
with commits and trees should have been a clear clue that tells us
something is fishy.  It calls "mark PARENTS uninteresting", leaving
the responsibility of marking the commit itself to the caller, but
it calls mark_tree_uninteresting() whose caller is not supposed to
mark the tree itself.

Which suggest me that a right fix for this separate bug would be to
introduce mark_tree_contents_uninteresting() or something, which has
the parallel semantics to mark_parents_uninteresting().  Then
mark_blob_uninteresting() call in the function can clearly go.  Such
a change will make it clear that handle_commit() is responsible for
handling the flags for the given object, and any helper functions
called by it should not peek and stop the flag of the object itself
when deciding to recurse into the objects linked to it.