Abhijeet Sonar <abhijeet.nkt@xxxxxxxxx> 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.
"their own default".
> 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
"outside a repository".
> 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@xxxxxxxxx>
> ---
> builtin/show-index.c | 6 ++++++
> rm | 3 +++
Huh?
> 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.
> 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 $@
Please don't.
> 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.
