Re: [PATCH v4 4/6] rev-list: handle missing tree objects properly

quoted

quoted

@@ -373,6 +375,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
      init_revisions(&revs, prefix);
      revs.abbrev = DEFAULT_ABBREV;
      revs.commit_format = CMIT_FMT_UNSPECIFIED;
+     revs.do_not_die_on_missing_tree = 1;

Is this correct? I would have expected this to be set only if --missing
was set.

If --missing is not set, then we want to fetch missing objects
automatically, and then die if we fail to do that, which is what
happens for blobs.

This is true, and should already be handled. Pay attention to when
fetch_if_missing is set in builtin/rev-list.c.

do_not_die_on_missing_tree should probably be set to 1 whenever
fetch_if_missing is set to 0, I think.

(I acknowledge that the usage of this global variable is confusing, but
I couldn't think of a better way to implement this when I did. Perhaps
when the object store refactoring is done, this can be a store-specific
setting instead of a global variable.)

So we don't want to die in list-objects.c. If we
fail to fetch, then we will die on line 213 in rev-list.c.

Why don't we want to die in list-objects.c? When --missing=error is
passed, fetch_if_missing retains its default value of 1, so
parse_tree_gently() will attempt to fetch it - and if it fails, I think
it's appropriate to die in list-objects.c (and this should be the
current behavior). On other values, e.g. --missing=allow-any, there is
no autofetch (since fetch_if_missing is 0), so it is correct not to die
in list-objects.c.

quoted

As for --missing=allow-promisor, I don't see them being tested anywhere
:-( so feel free to make a suggestion. I would put them in t6112 for
easy comparison with the other --missing tests.

Kept my allow-promisor test in t0410 since it requires partial clone
to be turned on in the config, and because it is pretty similar to
--exclude-promisor-objects.

OK, sounds good to me.