[08/23] fsck: require an actual buffer for non-blobs

The fsck_object() function takes in a buffer, but also a "struct
object". The rules for using these vary between types:

  - for a commit, we'll use the provided buffer; if it's NULL, we'll
    fall back to get_commit_buffer(), which loads from either an
    in-memory cache or from disk. If the latter fails, we'd die(), which
    is non-ideal for fsck.

  - for a tag, a NULL buffer will fall back to loading the object from
    disk (and failure would lead to an fsck error)

  - for a tree, we _never_ look at the provided buffer, and always use
    tree->buffer

  - for a blob, we usually don't look at the buffer at all, unless it
    has been marked as a .gitmodule file. In that case we check the
    buffer given to us, or assume a NULL buffer is a very large blob
    (and complain about it)

This is much more complex than it needs to be. It turns out that nobody
ever feeds a NULL buffer that isn't a blob:

  - git-fsck calls fsck_object() only from fsck_obj(). That in turn is
    called by one of:

      - fsck_obj_buffer(), which is a callback to verify_pack(), which
	unpacks everything except large blobs into a buffer (see
	pack-check.c, lines 131-141).

      - fsck_loose(), which hits a BUG() on non-blobs with a NULL buffer
	(builtin/fsck.c, lines 639-640)

    And in either case, we'll have just called parse_object_buffer()
    anyway, which would segfault on a NULL buffer for commits or tags
    (not for trees, but it would install a NULL tree->buffer which would
    later cause a segfault)

  - git-index-pack asserts that the buffer is non-NULL unless the object
    is a blob (see builtin/index-pack.c, line 832)

  - git-unpack-objects always writes a non-NULL buffer into its
    obj_buffer hash, which is then fed to fsck_object(). (There is
    actually a funny thing here where it does not store blob buffers at
    all, nor does it call fsck on them; it does check any needed blobs
    via fsck_finish() though).

Let's make the rules simpler, which reduces the amount of code and gives
us more flexibility in refactoring the fsck code. The new rules are:

  - only blobs are allowed to pass a NULL buffer

  - we always use the provided buffer, never pulling information from
    the object struct

We don't have to adjust any callers, because they were already adhering
to these. Note that we do drop a few fsck identifiers for missing tags,
but that was all dead code (because nobody passed a NULL tag buffer).

Signed-off-by: Jeff King <peff@peff.net>
---
 fsck.c | 51 +++++++++------------------------------------------
 fsck.h |  6 +++++-
 2 files changed, 14 insertions(+), 43 deletions(-)

Message ID	20191018045412.GH17879@sigill.intra.peff.net (mailing list archive)
State	New, archived
Headers	show Return-Path: <SRS0=pBOz=YL=vger.kernel.org=git-owner@kernel.org> Date: Fri, 18 Oct 2019 00:54:12 -0400 From: Jeff King <peff@peff.net> To: git@vger.kernel.org Subject: [PATCH 08/23] fsck: require an actual buffer for non-blobs Message-ID: <20191018045412.GH17879@sigill.intra.peff.net> References: <20191018044103.GA17625@sigill.intra.peff.net> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20191018044103.GA17625@sigill.intra.peff.net> Sender: git-owner@vger.kernel.org Precedence: bulk
Series	parsing and fsck cleanups \| expand [0/23] parsing and fsck cleanups [01/23] parse_commit_buffer(): treat lookup_commit() failure as parse error [02/23] parse_commit_buffer(): treat lookup_tree() failure as parse error [03/23] parse_tag_buffer(): treat NULL tag pointer as parse error [04/23] remember commit/tag parse failures [05/23] fsck: stop checking commit->tree value [06/23] fsck: stop checking commit->parent counts [07/23] fsck: stop checking tag->tagged [08/23] fsck: require an actual buffer for non-blobs [09/23] fsck: unify object-name code [10/23] fsck_describe_object(): build on our get_object_name() primitive [11/23] fsck: use oids rather than objects for object_name API [12/23] fsck: don't require object structs for display functions [13/23] fsck: only provide oid/type in fsck_error callback [14/23] fsck: only require an oid for skiplist functions [15/23] fsck: don't require an object struct for report() [16/23] fsck: accept an oid instead of a "struct blob" for fsck_blob() [17/23] fsck: drop blob struct from fsck_finish() [18/23] fsck: don't require an object struct for fsck_ident() [19/23] fsck: don't require an object struct in verify_headers() [20/23] fsck: rename vague "oid" local variables [21/23] fsck: accept an oid instead of a "struct tag" for fsck_tag() [22/23] fsck: accept an oid instead of a "struct commit" for fsck_commit() [23/23] fsck: accept an oid instead of a "struct tree" for fsck_tree()

[08/23] fsck: require an actual buffer for non-blobs

Commit Message

Patch