Re: [PATCH 36/44] builtin/index-pack: add option to specify hash algorithm

From: Martin Ågren <martin.agren@gmail.com>
Date: May 16, 2020, 11:18 UTC
Message-ID: <CAN0heSqpqAyKyaz+Er-SppR8k5W=zfw31rLR=Z8yZzqu=BCnTA@mail.gmail.com>
In-Reply-To: <20200513005424.81369-37-sandals@crustytoothpaste.net>

On Wed, 13 May 2020 at 02:56, brian m. carlson <sandals@crustytoothpaste.net> wrote:

Show 25 quoted lines

>
> git index-pack is usually run in a repository, but need not be. Since
> packs don't contains information on the algorithm in use, instead
> relying on context, add an option to index-pack to tell it which one
> we're using in case someone runs it outside of a repository.
>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
>  builtin/index-pack.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index 7bea1fba52..89f4962a00 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -1760,6 +1760,11 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
>                                         die(_("bad %s"), arg);
>                         } else if (skip_prefix(arg, "--max-input-size=", &arg)) {
>                                 max_input_size = strtoumax(arg, NULL, 10);
> +                       } else if (skip_prefix(arg, "--object-format=", &arg)) {
> +                               int hash_algo = hash_algo_by_name(arg);
> +                               if (hash_algo == GIT_HASH_UNKNOWN)
> +                                       die(_("unknown hash algorithm '%s'"), arg);
> +                               repo_set_hash_algo(the_repository, hash_algo);
>                         } else

Patch 27 added `--hash` to `git show-index` and I almost commented on "hash" vs "object-format". In the end I figured the object format was a more technical (protocol) term. But now I wonder. Should we try to align such options from the start? Or is there perhaps a reason for those different approaches?

Similar to an earlier patch where we modify `the_hash_algo` like this, I feel a bit nervous. What happens if you pass in a "wrong" algo here, i.e., SHA-1 in a SHA-256 repo? Or, given the motivation in the commit message, should this only be allowed if we really *are* outside a repo?

Martin