Re: "bad revision" fetch error when fetching missing objects from partial clones

[Jeff King <peff@peff.net>]

On Mon, May 17, 2021 at 08:03:22AM +0200, Patrick Steinhardt wrote:

> > If this is supposed to work, I think we need to teach the traversal code
> > to "add back" all of the objects that were explicitly given when a
> > filter is in use (either explicitly, or perhaps just clearing or
> > avoiding the UNINTERESTING flag on user-given objects in the first
> > place). And my earlier patch does that for the bitmap side, but not the
> > regular traversal.
> 
> I think adding these objects back in after computations is the easiest
> thing we can do here. For bitmaps, it should be relatively easy to do.
> The only thing I wonder about in your patch is whether we should really
> do this in a specific filter, or if we can't just do it at the end after
> all filters have been computed.

We definitely should do it at the end for all filters; my patch was just
illustrating the fix for this specific problem. It was actually while
cleaning it up to work on all filters that I dug further and found the
non-bitmap behavior.

> For the traversal-based code, it feels like the current design with the
> `NOT_USER_GIVEN` flag is making this harder than necessary. I really
> wonder whether we should again refactor the code to use a positive
> `USER_GIVEN` flag again: it's much more explicit, there's only one site
> where we need to add it and ultimately it's a lot easier to reason
> about. Furthermore, we can easily avoid marking such an object as
> `UNINTERESTING` if we see that it was explicitly added to the set while
> still marking its referenced objects as `UNINTERESTING` if need be.

That's my gut feeling, too. But I'm a little afraid that it would be
opening up a can of worms that we attempted to fix with 99c9aa9579
(revision: mark non-user-given objects instead, 2018-10-05) in the first
place.

For the purposes of this particular fix, though, we might be able to
skip this question either way with the "add back in" strategy. The
objects found in revs->pending right before prepare_revision_walk() make
up the "user given" list, so we could possibly just make us of that
(possibly stashing it away as necessary).

Thanks for the response. I'll be curious to hear the thoughts of folks
with Jonathan Tan who were involved in the original partial-clone design
(i.e., is this behavior of filters a surprise to them, or just a subtle
corner of the initial design).

-Peff