Re: [PATCH 1/3] show-index: implement automatic hash detection

[Junio C Hamano <gitster@pobox.com>]

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

If we turn what is inide of this block into a separate helper
function, it would allow us to structure the logic better.

	/* Returns GIT_HASH_* constants, or GIT_HASH_UNKNOWN */
	static int auto_detect_hash_function(int fd)

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.

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?

Thanks.

> +
> +	/* 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)];