RFC patch, 3 partsshow-index: modernize and implement auto-detection of hash algorithm

> @@ -71,6 +60,40 @@ int cmd_show_index(int argc,
>  			die("corrupt index file");
>  		nr = n;
>  	}
> +
> +	/* detection of hash algorithm
> +	Only works for small files, i.e without large offsets */
> +	if(!the_hash_algo && version == 2) {

> +		struct stat st;
> +		size_t file_base_size;
> +		size_t table_size;
> +		size_t size_rem;
> +		size_t hash_size;
> +
> +		if(fstat(0, &st) || !S_ISREG(st.st_mode))
> +			die(_("unable to detect hash from non-regular file"));

> +		file_base_size = 8 + (256 * 4);
> +		table_size = file_base_size + (nr * 4 * 4);
> +		size_rem = st.st_size - table_size;
> +		hash_size = size_rem / (nr + 2);
> +
> +		if(hash_size == GIT_SHA1_RAWSZ) {
> +			repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
> +		} else if(hash_size == GIT_SHA256_RAWSZ) {
> +			repo_set_hash_algo(the_repository, GIT_HASH_SHA256);

> +
> +	/* Final fallback to SHA1 */
> +	if(!the_hash_algo)
> +		repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
> +
> +	hashsz = the_hash_algo->rawsz;
> +
>  	if (version == 1) {
>  		for (i = 0; i < nr; i++) {
>  			unsigned int offset, entry[(GIT_MAX_RAWSZ + 4) / sizeof(unsigned int)];

> On Tue, Jan 20, 2026 at 10:07:42AM -0800, Junio C Hamano wrote:
> > Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com> writes:
> > > @@ -71,6 +60,40 @@ int cmd_show_index(int argc,
> [snip]
> > By the way, what happens if we find SHA-256 also broken and end up
> > choosing another hash function that is 256-bit wide in the next hash
> > revamp?
> 
> Yeah, agreed. The index unfortunately does not carry sufficient info to
> clearly identify the hash function that is in use, and second-guessing
> via the hash length doesn't really seem like a sensible solution to me.
> If we cannot tell for sure what the hash is, then we should rather ask
> the user to specify the object format. And in fact we already do that,
> as we have the `--object-format=` option for git-show-index(1).

> > On Tue, Jan 20, 2026 at 10:07:42AM -0800, Junio C Hamano wrote:
> > > Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com> writes:
> > > > @@ -71,6 +60,40 @@ int cmd_show_index(int argc,
> > [snip]
> > > By the way, what happens if we find SHA-256 also broken and end up
> > > choosing another hash function that is 256-bit wide in the next hash
> > > revamp?
> > 
> > Yeah, agreed. The index unfortunately does not carry sufficient info to
> > clearly identify the hash function that is in use, and second-guessing
> > via the hash length doesn't really seem like a sensible solution to me.
> > If we cannot tell for sure what the hash is, then we should rather ask
> > the user to specify the object format. And in fact we already do that,
> > as we have the `--object-format=` option for git-show-index(1).
> 
> Yes this is exactly why I was peculiar about this patch and the
> TODO comment, also why I sent it out as an RFC.
> 
> I initially assumed that in the near future we’re unlikely to move away
> from SHA-256 to another hash, but I agree that relying
> on hash length is still a heuristic that won't be a good approach
> in the long term as well as it creates ambiguity in the large files
> containing 64-bit offsets.
> 
> So should we drop this thought entirely and just make sure
> that if git show-index is run outside a repo,
> it should throw an error asking the the user
> to use --object-format option rather than silently
> falling back to SHA-1 which is the current approach.

> Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com> writes:
> 
> > @@ -71,6 +60,40 @@ int cmd_show_index(int argc,
> >			die("corrupt index file");
> >		nr = n;
> >	}
> > +
> > +	/* detection of hash algorithm
> > +	Only works for small files, i.e without large offsets */
> > +	if(!the_hash_algo && version == 2) {
> 
> We have one SP between "if" (and other syntactic elements like
> "while") and the open parenthesis "(".  End-user controlled function
> names lack this SP between <word> and "(".
> 

> For example, ...
> 
> > +		struct stat st;
> > +		size_t file_base_size;
> > +		size_t table_size;
> > +		size_t size_rem;
> > +		size_t hash_size;
> > +
> > +		if(fstat(0, &st) || !S_ISREG(st.st_mode))
> > +			die(_("unable to detect hash from non-regular file"));
> 
> ... this "die()" does not have to be here.  We can just return
> GIT_HASH_UNKNOWN and let the caller fallback.  Does the existing
> code correctly complain when the filestream is opened for a
> non-regular file, or it just gets totally confused?
> 

> > +		file_base_size = 8 + (256 * 4);
> > +		table_size = file_base_size + (nr * 4 * 4);
> > +		size_rem = st.st_size - table_size;
> > +		hash_size = size_rem / (nr + 2);
> > +
> > +		if(hash_size == GIT_SHA1_RAWSZ) {
> > +			repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
> > +		} else if(hash_size == GIT_SHA256_RAWSZ) {
> > +			repo_set_hash_algo(the_repository, GIT_HASH_SHA256);
> 
> And instead of calling repo_set_hash_algo(), just return the
> constants so that the caller can handle it.  And 
> 
> > +		} else {
> > +			die(_("unable to detect hash algorithm, "
> > +					"use --object-format option"));
> 
> ... this also can return GIT_HASH_UNKNOWN, without complaining
> anything.
> 
> > +		}
> > +	}
> 
> So, instead of inserting all of the above lines in cmd_show_index(),
> we'd have something like the following ...
> 
> 	hash_func = auto_detect_hash_function(0);
> 	if (hash_func == GIT_HASH_UNKNOWN) {
> 		warning(_("assuming SHA-1; use --object-format to override"));
> 		hash_func = GIT_HASH_SHA1;
> 	}
> 	repo_set_hash_algo(the_repository, hash_func);
>         hashsz = the_hash_algo->rawsz;
> 
> ... there.
> 

> As Git is in the process of removing global state,
> this function still relies on the global variables,
> the_repository and the_hash_algo.
> 
> Remove the associated macro and the UNUSED attribute from
> the repo parameter, and replace all uses of the_repository and
> the_hash_algo with repo and repo->hash_algo, respectively.

> This modernizes git show-index and makes it more compatible.
> 
> Signed-off-by: Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com>
> ---
>   builtin/show-index.c | 19 +++++++++----------
>   1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/builtin/show-index.c b/builtin/show-index.c
> index a9c2f18b73..96adae14c0 100644
> --- a/builtin/show-index.c
> +++ b/builtin/show-index.c
> @@ -1,4 +1,3 @@
> -#define USE_THE_REPOSITORY_VARIABLE
>   #define DISABLE_SIGN_COMPARE_WARNINGS
>   
>   #include "builtin.h"
> @@ -16,7 +15,7 @@ static const char *const show_index_usage[] = {
>   int cmd_show_index(int argc,
>   		   const char **argv,
>   		   const char *prefix,
> -		   struct repository *repo UNUSED)
> +		   struct repository *repo)
>   {
>   	int i;
>   	unsigned nr;
> @@ -37,7 +36,7 @@ int cmd_show_index(int argc,
>   		hash_algo = hash_algo_by_name(hash_name);
>   		if (hash_algo == GIT_HASH_UNKNOWN)
>   			die(_("Unknown hash algorithm"));
> -		repo_set_hash_algo(the_repository, hash_algo);
> +		repo_set_hash_algo(repo, hash_algo);
>   	}
>   
>   	if (fread(top_index, 2 * 4, 1, stdin) != 1)
> @@ -63,7 +62,7 @@ int cmd_show_index(int argc,
>   
>   	/* detection of hash algorithm
>   	Only works for small files, i.e without large offsets */
> -	if(!the_hash_algo && version == 2) {
> +	if(!repo->hash_algo && version == 2) {
>   		struct stat st;
>   		size_t file_base_size;
>   		size_t table_size;
> @@ -79,9 +78,9 @@ int cmd_show_index(int argc,
>   		hash_size = size_rem / (nr + 2);
>   
>   		if(hash_size == GIT_SHA1_RAWSZ) {
> -			repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
> +			repo_set_hash_algo(repo, GIT_HASH_SHA1);
>   		} else if(hash_size == GIT_SHA256_RAWSZ) {
> -			repo_set_hash_algo(the_repository, GIT_HASH_SHA256);
> +			repo_set_hash_algo(repo, GIT_HASH_SHA256);
>   		} else {
>   			die(_("unable to detect hash algorithm, "
>   					"use --object-format option"));
> @@ -89,10 +88,10 @@ int cmd_show_index(int argc,
>   	}
>   
>   	/* Final fallback to SHA1 */
> -	if(!the_hash_algo)
> -		repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
> +	if(!repo->hash_algo)
> +		repo_set_hash_algo(repo, GIT_HASH_SHA1);
>   
> -	hashsz = the_hash_algo->rawsz;
> +	hashsz = repo->hash_algo->rawsz;
>   
>   	if (version == 1) {
>   		for (i = 0; i < nr; i++) {
> @@ -114,7 +113,7 @@ int cmd_show_index(int argc,
>   		for (i = 0; i < nr; i++) {
>   			if (fread(entries[i].oid.hash, hashsz, 1, stdin) != 1)
>   				die(_("unable to read sha1 %u/%u"), i, nr);
> -			entries[i].oid.algo = hash_algo_by_ptr(the_hash_algo);
> +			entries[i].oid.algo = hash_algo_by_ptr(repo->hash_algo);
>   		}
>   		for (i = 0; i < nr; i++)
>   			if (fread(&entries[i].crc, 4, 1, stdin) != 1)

> On 20/01/2026 14:05, Shreyansh Paliwal wrote:
> > As Git is in the process of removing global state,
> > this function still relies on the global variables,
> > the_repository and the_hash_algo.
> > 
> > Remove the associated macro and the UNUSED attribute from
> > the repo parameter, and replace all uses of the_repository and
> > the_hash_algo with repo and repo->hash_algo, respectively.
> 
> I don't think that is a good idea because repo will be NULL outside of a 
> repository. For a lot of commands that does not matter because they 
> require a repository to run but judging from the first patch in this 
> series this command is supposed to be able to run outside a repository.
> 
> I'm increasingly of the opinion that adding a repository argument to the 
> builtin commands was a mistake as they all just use a single repository 
> so using "the_repository" seems perfectly reasonable. It leads to 
> problems like the segfault in this patch and takes attention away from 
> the much more useful task of moving our library code away from using 

> "the_repository". If you're interested in contributing to that effort 
> then there are a number of instances of "the_repository" in wt-status.c 
> that can be trivially replaced by the repository instance in "struct 
> wt_status" or the repository passed to the function. I'm not sure how 
> easy it is to remove them all - you might need to change the code to 
> pass a repository instance down the call chain in a few cases but there 
> are certainly quite a few that can be easily and usefully cleaned up.

> I'm increasingly of the opinion that adding a repository argument to the 
> builtin commands was a mistake as they all just use a single repository 
> so using "the_repository" seems perfectly reasonable. It leads to 
> problems like the segfault in this patch and takes attention away from 
> the much more useful task of moving our library code away from using 
> "the_repository".

> When 'git show-index' is run outside of a
> repository and no hashing algorithm is
> specified via --object-format, it silently
> falls back to SHA-1, relying on the
> historical default.
>
> This works for existing SHA-1 based
> index files, but the behavior can be ambiguous
> and confusing when the input index file uses a
> different hash algorithm, such as SHA-256.
>
> Add a warning when this fallback happens
> to make the assumption explicit and to
> guide users toward using --object-format
> when needed.

> -	if (!the_hash_algo)
> +	if (!the_hash_algo) {
> +		warning(_("assuming SHA-1; use --object-format to override"));
>  		repo_set_hash_algo(the_repository, GIT_HASH_DEFAULT);
> +	}

> > When 'git show-index' is run outside of a
> > repository and no hashing algorithm is
> > specified via --object-format, it silently
> > falls back to SHA-1, relying on the
> > historical default.
> >
> > This works for existing SHA-1 based
> > index files, but the behavior can be ambiguous
> > and confusing when the input index file uses a
> > different hash algorithm, such as SHA-256.
> >
> > Add a warning when this fallback happens
> > to make the assumption explicit and to
> > guide users toward using --object-format
> > when needed.
> 
> Line wrapping at 50 columns certainly makes the lines narrower than
> 80 column limit, but let's not go to the extreme.  We recommend that
> the lines are still less than 80-columns after being quoted a few
> times in e-mail exchange (as you can see, I lost 2 columns by
> quoting once in the above), which means that around ~70 columns is
> the practical fill-column.

> > Additionally, wrap user-facing die() messages
> > with _() so they can be translated via gettext.
> 
> It is somewhat distracting that such "while at it" changes dominate
> this ~100-line patch, whose "primary change" is a mere three lines
> we can see here: 
> 
> > -	if (!the_hash_algo)
> > +	if (!the_hash_algo) {
> > +		warning(_("assuming SHA-1; use --object-format to override"));
> >		repo_set_hash_algo(the_repository, GIT_HASH_DEFAULT);
> > +	}
> 
> 
> Can we push the "while at it" message changes to a separate patch, a
> preparatory clean-up, on top of which another primary patch adds the
> above warning?  Alternatively, have the primary patch that adds the
> above warning and does nothing else, followed by a post clean-up patch
> to tweak the existing error messages?

>  	/*
>  	 * 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.
> +	 * TODO: If a future implementation of index file version encodes the hash
> +	 *       algorithm in its header, enable show-index to infer it from the
> +	 *       header rather than relying on repository context or a default fallback.
>  	 */
> -	if (!the_hash_algo)
> +	if (!the_hash_algo) {
> +		warning(_("assuming SHA-1; use --object-format to override"));
>  		repo_set_hash_algo(the_repository, GIT_HASH_DEFAULT);

> On 2026-01-29 at 15:36:55, Shreyansh Paliwal wrote:
> >	/*
> >	 * 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.
> > +	 * TODO: If a future implementation of index file version encodes the hash
> > +	 *       algorithm in its header, enable show-index to infer it from the
> > +	 *       header rather than relying on repository context or a default fallback.
> >	 */
> > -	if (!the_hash_algo)
> > +	if (!the_hash_algo) {
> > +		warning(_("assuming SHA-1; use --object-format to override"));
> >		repo_set_hash_algo(the_repository, GIT_HASH_DEFAULT);
> 
> If we're going to start producing a warning, can we also learn a
> `--quiet` option to silence it?

> > On 2026-01-29 at 15:36:55, Shreyansh Paliwal wrote:
> > >	/*
> > >	 * 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.
> > > +	 * TODO: If a future implementation of index file version encodes the hash
> > > +	 *       algorithm in its header, enable show-index to infer it from the
> > > +	 *       header rather than relying on repository context or a default fallback.
> > >	 */
> > > -	if (!the_hash_algo)
> > > +	if (!the_hash_algo) {
> > > +		warning(_("assuming SHA-1; use --object-format to override"));
> > >		repo_set_hash_algo(the_repository, GIT_HASH_DEFAULT);
> > 
> > If we're going to start producing a warning, can we also learn a
> > `--quiet` option to silence it?
> 
> Hi,
> 
> That is a good point.
> However, the warning is only shown for a particular usecase: when 
> 'git show-index' is run outside of a repository and --object-format
> is not specified. Given that narrow scope, I’m wondering whether
> adding a dedicated --quiet option for only this warning would be worthwhile.
> 
> Let me know what you think :)

> I also wonder whether "--quiet" might be a bit _too_ generic in this
> context. I would rather want to use this flag for something that you
> actually have a good reason to silence, instead of only for a warning.
>
> In theory, the user already has the ability to silence the warning: they
> can simply pass "--object-format=sha256". If you think that's not enough
> I'd buid on top of our `advice_if_enabled()` infra, so that the warning
> can be globally disabled by setting a config option.