Re: [PATCH v2 3/5] rev-list: handle missing tree objects properly

I don't understand about the ">= 0". What should I replace it with?
Maybe you mean the return is never positive so I can change:

parse_tree_gently(tree, 1) >= 0

to:
!parse_tree_gently(tree, 1)

?

Sorry for the lack of clarity - that is what I meant.

quoted

The missing mechanism (for error, allow-any, print) should work without
needing to consult whether an object is a promisor object or not - it
should just print whatever is missing, so the "if
(!is_promisor_object..." line looks out of place.

Done. I considered that a missing object which is not a promisor is a
serious error, so I had it die here.

It is a serious error, but as far as I can tell, that is what the
--missing flags are supposed to help diagnose (so we can't die since we
need the diagnoses to be printed). See, for example, 'rev-list W/
--missing=print' in t6112 - the "r1" repository does not have partial
clone enabled (which I verified by inserting a test_pause then cat-ting
r1/.git/config), but nothing dies.

But now that I've added the
do_not_die_on_missing_tree flag, it's more natural to keep the
previous promisor check as-is.

OK, I'll take a look once you send out v4.

Also, is_promisor_object is an
expensive check, and it would be better to skip it during the common
execution path (which should be when exclude_promisor_objects, an
internal-use-only flag, is *not* set, which means we never call
is_promisor_object.

That's true.

quoted

In my original review [1], I suggested that we always show a tree if we
have its hash - if we don't have the object, we just recurse into it.
This would be the same as your patch, except that the 'die("bad tree
object...' is totally removed instead of merely moved. I still think
this solution has some merit - all the tests still pass (except that we
need to check for "unable to read" instead of "bad tree object" in error
messages), but I just realized that it might still be backwards
incompatible in that a basic "rev-list --objects" would now succeed
instead of fail if a tree was missing (I haven't tested this though).

The presence of the die if !is_promisor_object is what justified the
changing of the parse_tree_gently to always be gently, since it is
what showed the OID. Can we really remove both? Maybe in a different
patch set, since I'm no longer touching that line?

That's true - the idea of removing both needs more thought, and if we
were to do so, we definitely can do it in a different patch set.

quoted

We might need a flag called "do_not_die_on_missing_tree" (much like your
original idea of "show_missing_trees") so that callers that are prepared
to deal with missing trees can set this. Sorry for the churn. You can
document it as such:

Added it, but not with a command-line flag, only in rev-info.h. We can
always  add a flag later if people have been relying on the existing
behavior of git rev-list to balk at missing trees. (That seems
unlikely though, considering there is no filter to enable that before
this patchset).

By flag, I indeed meant in rev-info.h - sorry for the confusion. That
sounds good.