Re: [PATCH v2 4/4] sha1_file, fsck: add missing blob support
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 15, 2017, 18:34 UTC
- Message-ID
- <xmqqy3st14my.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <aa3904dbe16050188a6b70a209bfcbfa86ea3213.1497387714.git.jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
Show 24 quoted lines
> diff --git a/sha1_file.c b/sha1_file.c
> index 98086e21e..75fe2174d 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -27,6 +27,9 @@
> #include "list.h"
> #include "mergesort.h"
> #include "quote.h"
> +#include "iterator.h"
> +#include "dir-iterator.h"
> +#include "sha1-lookup.h"
>
> #define SZ_FMT PRIuMAX
> static inline uintmax_t sz_fmt(size_t s) { return s; }
> @@ -1624,6 +1627,72 @@ static const struct packed_git *has_packed_and_bad(const unsigned char *sha1)
> return NULL;
> }
>
> +struct missing_blob_manifest {
> + struct missing_blob_manifest *next;
> + const char *data;
> +};
> +struct missing_blob_manifest *missing_blobs;
> +int missing_blobs_initialized;I do not think you meant to make these non-static. The type of the former is not even visible to the outside world, and the latter is something that could be made into static to prepare_missing_blobs() function (unless and until you start allowing the missing-blobs manifest to be re-initialized). Your ensure_configured() below seems to do the "static" right, on the other hand ;-).
Do we expect that we will have only a handful of these missing blob manifests? Each manifest seems to be efficiently looked-up with a binary search, but it makes me wonder if it is a good idea to consolidate these manifests into a single list of object names to eliminate the outer loop in has_missing_blob(). Unlike pack .idx files that must stay one-to-one with .pack files, it appears to me that there is no reason why we need to keep multiple ones separate for extended period of time (e.g. whenever we learn that we receieved an incomplete pack from the other side with a list of newly missing blobs, we could incorporate that into existing missing blob list).
> +int has_missing_blob(const unsigned char *sha1, unsigned long *size)
> +{This function that answers "is it expected to be missing?" is confusingly named. Is it missing, or does it exist?
Show 18 quoted lines
> @@ -2981,11 +3050,55 @@ static int sha1_loose_object_info(const unsigned char *sha1,
> return (status < 0) ? status : 0;
> }
>
> +static char *missing_blob_command;
> +static int sha1_file_config(const char *conf_key, const char *value, void *cb)
> +{
> + if (!strcmp(conf_key, "core.missingblobcommand")) {
> + missing_blob_command = xstrdup(value);
> + }
> + return 0;
> +}
> +
> +static int configured;
> +static void ensure_configured(void)
> +{
> + if (configured)
> + return;Do not be selfish and pretend that this is the _only_ kind of configuration that needs to be done inside sha1_file.c. Call the function ensure_<something>_is_configured() and rename the run-once guard to match.
The run-once guard can be made static to the "ensure" function, and if you do so, then its name can stay to be "configured", as at that point it is clear what it is guarding.
Show 13 quoted lines
> diff --git a/t/t3907-missing-blob.sh b/t/t3907-missing-blob.sh
> new file mode 100755
> index 000000000..e0ce0942d
> --- /dev/null
> +++ b/t/t3907-missing-blob.sh
> @@ -0,0 +1,69 @@
> +#!/bin/sh
> +
> +test_description='core.missingblobcommand option'
> +
> +. ./test-lib.sh
> +
> +pack() {Style: "pack () {"> + perl -e '$/ = undef; $input = <>; print pack("H*", $input)'high-nybble first to match ntohll() done in has_missing_blob()? OK.
Show 24 quoted lines
> +} > + > +test_expect_success 'sha1_object_info_extended and read_sha1_file (through git cat-file -p)' ' > + rm -rf server client && > + > + git init server && > + test_commit -C server 1 && > + test_config -C server uploadpack.allowanysha1inwant 1 && > + HASH=$(git hash-object server/1.t) && > + > + git init client && > + test_config -C client core.missingblobcommand \ > + "git -C \"$(pwd)/server\" pack-objects --stdout | git unpack-objects" && > + > + # does not work if missing blob is not registered > + test_must_fail git -C client cat-file -p "$HASH" && > + > + mkdir -p client/.git/objects/missing && > + printf "%016x%s%016x" 1 "$HASH" "$(wc -c <server/1.t)" | > + pack >client/.git/objects/missing/x && > + > + # works when missing blob is registered > + git -C client cat-file -p "$HASH" > +'
OK, by passing printf '%016x', implementations of "$(wc -c)" that gives extra whitespace around its output can still work correctly. Good.