Junio C Hamano <gitster@xxxxxxxxx> writes:
> Karthik Nayak via B4 Relay
> <devnull+karthik.188.gmail.com@xxxxxxxxxx> writes:
>
>> From: Karthik Nayak <karthik.188@xxxxxxxxx>
>>
>> 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.
>
Agreed, Patrick also pointed out the same.
> 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.
>
Will fix it in the next version, but in short, it was because the
function is also part of 'pack-write.c' and we're focusing only on
cleanup of 'pack-write.c'.
Attachment:
signature.asc
Description: PGP signature
