Subject: Re: [PATCH 3/5] pack-write: pass hash_algo to `write_idx_file()`

From: Junio C Hamano <[email-masked]>
Date: Thu, 16 Jan 2025 11:12:49 -0800

Karthik Nayak via B4 Relay
<[email-masked]> writes:

> From: Karthik Nayak <[email-masked]>
>
> The `write_idx_file()` function uses the global `the_hash_algo` variable
> to access the repository's hash function. To avoid global variable
> usage, pass the hash function from the layers above.
>
> Altough the layers above could have access to the hash function
> internally, simply pass in `the_hash_algo`. This avoids any
> compatibility issues and bubbles up global variable usage to upper
> layers which can be eventually resolved.
> ...
> -void stage_tmp_packfiles(struct strbuf *name_buffer,
> +void stage_tmp_packfiles(const struct git_hash_algo *hash_algo,
> +			 struct strbuf *name_buffer,
>  			 const char *pack_tmp_name,
>  			 struct pack_idx_entry **written_list,
>  			 uint32_t nr_written,
> @@ -561,8 +563,8 @@ void stage_tmp_packfiles(struct strbuf *name_buffer,
>  	if (adjust_shared_perm(pack_tmp_name))
>  		die_errno("unable to make temporary pack file readable");
>  
> -	*idx_tmp_name = (char *)write_idx_file(NULL, written_list, nr_written,
> -					       pack_idx_opts, hash);
> +	*idx_tmp_name = (char *)write_idx_file(hash_algo, NULL, written_list,
> +					       nr_written, pack_idx_opts, hash);

The proposed log message should mention the reason why this
stage_tmp_packfiles() function needs to be singled out among many
other direct callers of write_idx_file() function.  

In other words, ...

> @@ -798,8 +798,8 @@ static const char *create_index(void)
>  	if (c != last)
>  		die("internal consistency error creating the index");
>  
> -	tmpfile = write_idx_file(NULL, idx, object_count, &pack_idx_opts,
> -				 pack_data->hash);
> +	tmpfile = write_idx_file(the_hash_algo, NULL, idx, object_count,
> +				 &pack_idx_opts, pack_data->hash);
>  	free(idx);
>  	return tmpfile;
>  }

... this hunk could have made create_index() to take a git_hash_algo
object and pass it down to write_idx_file(), while changing all the
callers of create_index() pass the_hash_algo, but we did not do so.

But stage_tmp_packfiles() got that treatment.  Please tell your
readers in the proposed log message what makes it special.

Thanks.