Re: [PATCH v2 3/5] index-pack: check (and optionally set) hash algo based on input file

[brian m. carlson]

On Sat, Feb 24, 2018 at 10:34:27AM +0700, Nguyễn Thái Ngọc Duy wrote:
> 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 
> ---
>  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(_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 = _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


signature.asc
Description: PGP signature

[PATCH v2 3/5] index-pack: check (and optionally set) hash algo based on input file

[Nguyễn Thái Ngọc Duy]

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 
---
 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(_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 = _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);
+}
+
 static void parse_pack_header(void)
 {
struct pack_header *hdr = fill(sizeof(struct pack_header));
@@ -341,6 +362,9 @@ static void parse_pack_header(void)
die(_("pack version %"PRIu32" unsupported"),
ntohl(hdr->hdr_version));
 
+   prepare_hash_algo(ntohl(hdr->hdr_version));
+   the_hash_algo->init_fn(_ctx);
+
nr_objects = ntohl(hdr->hdr_entries);
use(sizeof(struct pack_header));
 }
-- 
2.16.1.435.g8f24da2e1a