Re: [PATCH v2 3/4] Integrate hash algorithm support with repo setup

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Oct 29, 2017, 01:44 UTC
Message-ID: <CAPig+cTrAxWczJ5qX2qE-hqoo6hA2QCom25HYB7v48rVnPgTow@mail.gmail.com>
In-Reply-To: <20171028181239.59458-4-sandals@crustytoothpaste.net>

On Sat, Oct 28, 2017 at 2:12 PM, brian m. carlson <sandals@crustytoothpaste.net> wrote:

Show 14 quoted lines

> In future versions of Git, we plan to support an additional hash
> algorithm.  Integrate the enumeration of hash algorithms with repository
> setup, and store a pointer to the enumerated data in struct repository.
> Of course, we currently only support SHA-1, so hard-code this value in
> read_repository_format.  In the future, we'll enumerate this value from
> the configuration.
> [...]
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
> diff --git a/cache.h b/cache.h
> @@ -132,6 +133,8 @@ struct git_hash_algo {
>  extern const struct git_hash_algo hash_algos[GIT_HASH_NALGOS];
>
> +#define current_hash the_repository->hash_algo

The all-lowercase name "current_hash" seems likely to conflict with a variable name some day; the fact that it is also a #define makes such a collision even more worrisome. Although it is retrieving the "hash algorithm", when reading the terse name "current_hash", one may instead intuitively think it is referring to a hash _value_ (not an algorithm).

Show 15 quoted lines

> diff --git a/repository.c b/repository.c
> @@ -64,6 +64,11 @@ void repo_set_gitdir(struct repository *repo, const char *path)
> +void repo_set_hash_algo(struct repository *repo, int hash_algo)
> +{
> +       repo->hash_algo = &hash_algos[hash_algo];
> +}
> +
>  /*
>   * Attempt to resolve and set the provided 'gitdir' for repository 'repo'.
>   * Return 0 upon success and a non-zero value upon failure.
> @@ -136,6 +141,8 @@ int repo_init(struct repository *repo, const char *gitdir, const char *worktree)
>         if (read_and_verify_repository_format(&format, repo->commondir))
>                 goto error;
>
> +       repo->hash_algo = &hash_algos[format.hash_algo];

Should this be instead invoking repo_set_hash_algo()?

>         if (worktree)
>                 repo_set_worktree(repo, worktree);