Re: [PATCH v2 3/5] index-pack: check (and optionally set) hash algo based on input file
- From
brian m. carlson <sandals@crustytoothpaste.net>
- Date
- Feb 24, 2018, 22:56 UTC
- Message-ID
- <20180224225625.GC4620@genre.crustytoothpaste.net>
- In-Reply-To
- <20180224033429.9656-4-pclouds@gmail.com>
On Sat, Feb 24, 2018 at 10:34:27AM +0700, Nguyễn Thái Ngọc Duy wrote:
Show 51 quoted lines
> After 454253f059 (builtin/index-pack: improve hash function abstraction
> - 2018-02-01), index-pack uses the_hash_algo for hashing. If "git
> index-pack" is executed without a repository, we do not know what hash
> algorithm to be used and the_hash_algo in theory could be undefined.
>
> Since there should be some information about the hash algorithm in the
> input pack file, we can initialize the correct hash algorithm with that
> if the_hash_algo is not yet initialized. This assumes that pack files
> with new hash algorithm MUST step up pack version.
>
> While at there, make sure the hash algorithm requested by the pack file
> and configured by the repository (if we're running with a repo) are
> consistent.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> builtin/index-pack.c | 26 +++++++++++++++++++++++++-
> 1 file changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index 7e3e1a461c..1144458140 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -326,10 +326,31 @@ static const char *open_pack_file(const char *pack_name)
> output_fd = -1;
> nothread_data.pack_fd = input_fd;
> }
> - the_hash_algo->init_fn(&input_ctx);
> return pack_name;
> }
>
> +static void prepare_hash_algo(uint32_t pack_version)
> +{
> + const struct git_hash_algo *pack_algo;
> +
> + switch (pack_version) {
> + case 2:
> + case 3:
> + pack_algo = &hash_algos[GIT_HASH_SHA1];
> + break;
> + default:
> + die("BUG: how to determine hash algo for new version?");
> + }
> +
> + if (!the_hash_algo) /* running without repo */
> + the_hash_algo = pack_algo;
> +
> + if (the_hash_algo != pack_algo)
> + die(_("incompatible hash algorithm, "
> + "configured for %s but the pack file needs %s"),
> + the_hash_algo->name, pack_algo->name);I like this. It's a nice improvement and it should be easy for us to pass additional information into the function when our pack format understands multiple algorithms.
I might have done the comparison using the format_id members instead of the pointers themselves, but that's more a personal preference than anything.
-- brian m. carlson / brian with sandals: Houston, Texas, US https://www.crustytoothpaste.net/~bmc | My opinion only OpenPGP: https://keybase.io/bk2204