Re: [PATCH v4] show-index: fix uninitialized hash function
Abhijeet Sonar <abhijeet.nkt@gmail.com> writes:
> In c8aed5e8da (repository: stop setting SHA1 as the default object
> hash), we got rid of the default hash algorithm for the_repository.
> Due to this change, it is now the responsibility of the callers to set
> thier own default when this is not present.
> As stated in the docs, show-index should use SHA1 as the default hash
> algorithm when ran outsize of a repository. Make sure this promise is
Show 8 quoted lines
> met by falling back to SHA1 when the_hash_algo is not present (i.e.
> when the command is ran outside of a repository). Also add a test that
> verifies this behaviour.
>
> Signed-off-by: Abhijeet Sonar <abhijeet.nkt@gmail.com>
> ---
> builtin/show-index.c | 6 ++++++
> rm | 3 +++
Show 15 quoted lines
> t/t5300-pack-object.sh | 4 ++++
> 3 files changed, 13 insertions(+)
> create mode 100755 rm
>
> diff --git a/builtin/show-index.c b/builtin/show-index.c
> index f164c01bbe..645c2548fb 100644
> --- a/builtin/show-index.c
> +++ b/builtin/show-index.c
> @@ -38,6 +38,12 @@ int cmd_show_index(int argc,
> repo_set_hash_algo(the_repository, hash_algo);
> }
>
> + // Fallback to SHA1 if we are running outside of a repository.
> + // TODO: Figure out and implement a way to detect the hash algorithm in use by the
> + // the index file passed in and use that instead.
/*
* A multi-line comment in our codebase looks
* like this; slash-asterisk and asterisk-slash
* are placed on their own lines. We do not do
* double-slash comments.
*/
> + if (!the_hash_algo)
> + repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
OK. This is in line with how the command is documented to behave.
Having said that, I am not sure if it was an omission by mistake when 8e42eb0e (doc: sha256 is no longer experimental, 2023-07-31) marked SHA-256 as non-experimental, or it was deliberate. It would have been an equally plausible, if not more sensible, position to take to say that, since SHA-1 and SHA-256 are now on equal footing, we won't "default" to SHA-1 anymore, when 8e42eb0e declared that SHA-256 is no longer a second-class citizen.
In any case, we can further remedy that, if we really wanted to, by tweaking the documentation to require the option outside a repository without any default, for example, and then change this to die().
Of course, we may want to use the hash that is used in the index file we are reading, if we can, as your comment said.
These incremental improvements can be left outside the scope of this change.
Show 9 quoted lines
> diff --git a/rm b/rm
> new file mode 100755
> index 0000000000..2237506bf2
> --- /dev/null
> +++ b/rm
> @@ -0,0 +1,3 @@
> +#!/bin/sh
> +
> +echo rm $@
Show 11 quoted lines
> diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
> index 3b9dae331a..51fed26cc4 100755
> --- a/t/t5300-pack-object.sh
> +++ b/t/t5300-pack-object.sh
> @@ -523,6 +523,10 @@ test_expect_success 'index-pack --strict <pack> works in non-repo' '
> test_path_is_file foo.idx
> '
>
> +test_expect_success SHA1 'show-index works OK outside a repository' '
> + nongit git show-index <foo.idx
> +'
If we are not using a hash that is not SHA-1, we should then be able to do the same check with
nongit git show-index --object-format=<hash> <foo.idx
i.e., with an explicit argument. I do not think we have any hits in the t/ directory from
$ git grep -e 'show-index .*--object-format' t/
so such a test might be worth adding, either as a part of this
change or as a separate patch.
> test_expect_success !PTHREADS,!FAIL_PREREQS \
> 'index-pack --threads=N or pack.threads=N warns when no pthreads' '
> test_must_fail git index-pack --threads=2 2>err &&
Except for these minor nits, everything else looks great.
Thanks.
Abhijeet Sonar
