Re: [PATCH v1] builtin/pack-objects.c: ignore missing links with --stdin-packs

Taylor Blau [off-list ref] writes:

When 'git pack-objects --stdin-packs' encounters a commit in a pack, it
marks it as a starting point of a best-effort reachability traversal
that is used to populate the name-hash of the objects listed in the
given packs.

The traversal expects that it should be able to walk the ancestors of
all commits in a pack without issue. Ordinarily this is the case, but it
is possible to having missing parents from an unreachable part of the
repository. In that case, we'd consider any missing objects in the
unreachable portion of the graph to be junk.

Ah, OK.  So a pack that is being consolidated, or more likely a
loose commit that is rolled into the smallest geometric group) may
contain an unreachable commit, whose tree or blob has already been
pruned, which is an expected state (i.e. tree or blob may have been
older than the commit whose message may have been amended recently
before the entire commit got abandoned).  And we do not want to alarm
users by warning.

This should be handled gracefully: since the traversal is best-effort
(i.e., we don't strictly need to fill in all of the name-hash fields),
we should simply ignore any missing links.

Or the entire set of objects that refer them can be omitted from the
resulting set of objects (iow, consider a commit that lacks a tree
or a blob to be checked out stale and prunable without downsides,
and prune it and its remaining trees and blobs by leaving them out
of the resulting pack), but I suspect that is a lot more involved
change.

This patch does that (by setting the 'ignore_missing_links' bit on the
rev_info struct), and ensures we don't regress in the future by adding a
test which demonstrates this case.

It is a little over-eager, since it will also ignore missing links in
reachable parts of the packs (which would indicate a corrupted
repository), but '--stdin-packs' is explicitly *not* about reachability.
So this step isn't making anything worse for a repository which contains
packs missing reachable objects (since we never drop objects with
'--stdin-packs').

Yup.  I find the reasoning quite sensible.

Thanks, will queue.

On Fri, Mar 19, 2021 at 11:19:11AM -0700, Junio C Hamano wrote:

quoted

The traversal expects that it should be able to walk the ancestors of
all commits in a pack without issue. Ordinarily this is the case, but it
is possible to having missing parents from an unreachable part of the
repository. In that case, we'd consider any missing objects in the
unreachable portion of the graph to be junk.

Ah, OK.  So a pack that is being consolidated, or more likely a
loose commit that is rolled into the smallest geometric group) may
contain an unreachable commit, whose tree or blob has already been
pruned, which is an expected state (i.e. tree or blob may have been
older than the commit whose message may have been amended recently
before the entire commit got abandoned).  And we do not want to alarm
users by warning.

Yes, though it is not just warning, but rather that before this patch
we'd abort the whole repack.

quoted

This should be handled gracefully: since the traversal is best-effort
(i.e., we don't strictly need to fill in all of the name-hash fields),
we should simply ignore any missing links.

Or the entire set of objects that refer them can be omitted from the
resulting set of objects (iow, consider a commit that lacks a tree
or a blob to be checked out stale and prunable without downsides,
and prune it and its remaining trees and blobs by leaving them out
of the resulting pack), but I suspect that is a lot more involved
change.

It is safe to omit the whole set from the name-hash traversal, which is
purely an optimization. But it would generally not be a good idea to
leave them out of the resulting pack, since that would mean deleting
them entirely from the repository (because we'll remove the old packs
they were in after pack-objects completes).

If they are truly unreachable, then it is not strictly wrong to delete
them (i.e., we are not corrupting a repository unless it was already
corrupted), but:

  - if the repository _is_ already corrupted, we are definitely making
    things worse

  - we generally try to keep even unreachable parts of the graph
    complete, doing things like keeping unreachable-but-old objects that
    are reachable from unreachable-but-recent. Again, we know here that
    the object graph is incomplete, so anybody pointing a ref at a
    descendant of our broken commit is already corrupting the
    repository. But it probably makes sense to follow the existing rules
    as much as possible, and not make such a situation worse.

quoted

It is a little over-eager, since it will also ignore missing links in
reachable parts of the packs (which would indicate a corrupted
repository), but '--stdin-packs' is explicitly *not* about reachability.
So this step isn't making anything worse for a repository which contains
packs missing reachable objects (since we never drop objects with
'--stdin-packs').

Yup.  I find the reasoning quite sensible.

Thanks, will queue.

I had seen and discussed the patch before it hit the list, but just to
make it explicit: it also looks good to me. ;)

-Peff