git/streaming.c, branch v2.37.2

object-file.c: stop dying in parse_loose_header()

2021-10-01T22:06:00Z

Make parse_loose_header() return error codes and data instead of invoking die() by itself. For now we'll move the relevant die() call to loose_object_info() and read_loose_object() to keep this change smaller. In a subsequent commit we'll make read_loose_object() return an error code instead of dying. We should also address the "allow_unknown" case (should be moved to builtin/cat-file.c), but for now I'll be leaving it. For making parse_loose_header() not die() change its prototype to accept a "struct object_info *" instead of the "unsigned long *sizep" it accepted before. Its callers can now check the populated populated "oi->typep". Because of this we don't need to pass in the "unsigned int flags" which we used for OBJECT_INFO_ALLOW_UNKNOWN_TYPE, we can instead do that check in loose_object_info(). This also refactors some confusing control flow around the "status" variable. In some cases we set it to the return value of "error()", i.e. -1, and later checked if "status < 0" was true. Since 93cff9a978e (sha1_loose_object_info: return error for corrupted objects, 2017-04-01) the return value of loose_object_info() (then named sha1_loose_object_info()) had been a "status" variable that be any negative value, as we were expecting to return the "enum object_type". The only negative type happens to be OBJ_BAD, but the code still assumed that more might be added. This was then used later in e.g. c84a1f3ed4d (sha1_file: refactor read_object, 2017-06-21). Now that parse_loose_header() will return 0 on success instead of the type (which it'll stick into the "struct object_info") we don't need to conflate these two cases in its callers. Since parse_loose_header() doesn't need to return an arbitrary "status" we only need to treat its "ret < 0" specially, but can idiomatically overwrite it with our own error() return. This along with having made unpack_loose_header() return an "enum unpack_loose_header_result" in an earlier commit means that we can move the previously nested if/else cases mostly into the "ULHR_OK" branch of the "switch" statement. We should be less silent if we reach that "status = -1" branch, which happens if we've got trailing garbage in loose objects, see f6371f92104 (sha1_file: add read_loose_object() function, 2017-01-13) for a better way to handle it. For now let's punt on it, a subsequent commit will address that edge case. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano

object-file.c: return ULHR_TOO_LONG on "header too long"

2021-10-01T22:06:00Z

Split up the return code for "header too long" from the generic negative return value unpack_loose_header() returns, and report via error() if we exceed MAX_HEADER_LEN. As a test added earlier in this series in t1006-cat-file.sh shows we'll correctly emit zlib errors from zlib.c already in this case, so we have no need to carry those return codes further down the stack. Let's instead just return ULHR_TOO_LONG saying we ran into the MAX_HEADER_LEN limit, or other negative values for "unable to unpack header". Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano

object-file.c: use "enum" return type for unpack_loose_header()

2021-10-01T22:06:00Z

In a preceding commit we changed and documented unpack_loose_header() from its previous behavior of returning any negative value or zero, to only -1 or 0. Let's add an "enum unpack_loose_header_result" type and use it for these return values, and have the compiler assert that we're exhaustively covering all of them. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano

object-file.c: simplify unpack_loose_short_header()

2021-10-01T22:06:00Z

Combine the unpack_loose_short_header(), unpack_loose_header_to_strbuf() and unpack_loose_header() functions into one. The unpack_loose_header_to_strbuf() function was added in 46f034483eb (sha1_file: support reading from a loose object of unknown type, 2015-05-03). Its code was mostly copy/pasted between it and both of unpack_loose_header() and unpack_loose_short_header(). We now have a single unpack_loose_header() function which accepts an optional "struct strbuf *" instead. I think the remaining unpack_loose_header() function could be further simplified, we're carrying some complexity just to be able to emit a garbage type longer than MAX_HEADER_LEN, we could alternatively just say "we found a garbage type ..." instead. But let's leave the current behavior in place for now. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano

object-file.c: make parse_loose_header_extended() public

2021-10-01T22:06:00Z

Make the parse_loose_header_extended() function public and remove the parse_loose_header() wrapper. The only direct user of it outside of object-file.c itself was in streaming.c, that caller can simply pass the required "struct object-info *" instead. This change is being done in preparation for teaching read_loose_object() to accept a flag to pass to parse_loose_header(). It isn't strictly necessary for that change, we could simply use parse_loose_header_extended() there, but will leave the API in a better end state. It would be a better end-state to have already moved the declaration of these functions to object-store.h to avoid the forward declaration of "struct object_info" in cache.h, but let's leave that cleanup for some other time. 1. https://lore.kernel.org/git/patch-v6-09.22-5b9278e7bb4-20210907T104559Z-avarab@gmail.com/ Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano

streaming.c: move {open,close,read} from vtable to "struct git_istream"

2021-05-06T03:56:10Z

Move the definition of the structure around the open/close/read functions introduced in 46bf043807c (streaming: a new API to read from the object store, 2011-05-11) to instead populate "close" and "read" members in the "struct git_istream". This gets us rid of an extra pointer deference, and I think makes more sense. The "close" and "read" functions are the primary interface to the stream itself. Let's also populate a "open" callback in the same struct. That's now used by open_istream() after istream_source() decides what "open" function should be used. This isn't needed to get rid of the "stream_vtbl" variables, but makes sense for consistency. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano

streaming.c: stop passing around "object_info *" to open()

2021-05-06T03:56:09Z

Change the streaming interface to stop passing around the "struct object_info" the open() functions. As seen in 7ef2d9a2604 (streaming: read non-delta incrementally from a pack, 2011-05-13) which introduced the "st->u.in_pack" assignments being changed here only the open_istream_pack_non_delta() path need these. So let's instead do this when preparing the selected callback in the istream_source() function. This might also allow the compiler to reduce the lifetime of the "oi" variable, as we've moved it from "git_istream()" to "istream_source()". Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano

streaming.c: remove {open,close,read}_method_decl() macros

2021-05-06T03:56:06Z

Remove the {open,close,read}_method_decl() macros added in 46bf043807c (streaming: a new API to read from the object store, 2011-05-11) in favor of inlining the definition of the arguments of these functions. Since we'll end up using them via the "{open,close,read}_istream_fn" types we don't gain anything in the way of compiler checking by using these macros, and as of preceding commits we no longer need to declare these argument lists twice. So declaring them at a distance just serves to make the code less readable. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano

streaming.c: remove enum/function/vtbl indirection

2021-05-06T03:56:04Z

Remove the indirection of discovering a function pointer to use via an enum and virtual table. This refactors code added in 46bf043807c (streaming: a new API to read from the object store, 2011-05-11). We can instead simply return an "open_istream_fn" for use from the "istream_source()" selector function directly. This allows us to get rid of the "incore", "loose" and "pack_non_delta" enum variables. We'll return the functions instead. The "stream_error" variable in that enum can likewise go in favor of returning NULL, which is what the open_istream() was doing when it got that value anyway. We can thus remove the entire enum, and the "open_istream_tbl" virtual table that (indirectly) referenced it. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano

streaming.c: avoid forward declarations

2021-05-06T03:56:02Z

Change code added in 46bf043807c (streaming: a new API to read from the object store, 2011-05-11) to avoid forward declarations of the functions it uses. We can instead move this code to the bottom of the file, and thus avoid the open_method_decl() calls. Aside from the addition of the "static helpers[...]" comment being added here, and the removal of the forward declarations this is a move-only change. The style of the added "static helpers[...]" comment isn't in line with our usual coding style, but is consistent with several other comments used in this file, so let's use that style consistently here. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano