From: Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com>
Date: Wed, 21 Jan 2026 10:28:34 GMT
Subject: Re: [PATCH 1/3] show-index: implement automatic hash detection
Message-ID: <20260121102956.759871-1-shreyanshpaliwalcmsmn@gmail.com>
In-Reply-To: <xmqqzf68yx75.fsf@gitster.g>

> 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 "(".
> 

Got it, I will keep this in mind.

> 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?
> 

I believe there is no explicit check for
irregular files in the current implementation.

> > +		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.
> 

Yes this is surely a much better approach than before,
I will implement this as a seperate helper function in v2
if we go ahead with the detection logic.