Junio C Hamano <gitster@xxxxxxxxx> 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html