Re: [PATCH v4 03/10] commit-graph: compute generation numbers

[Jakub Narebski <jnareb@gmail.com>]

Derrick Stolee <stolee@gmail.com> writes:
> On 4/29/2018 5:08 AM, Jakub Narebski wrote:
>> Derrick Stolee <dstolee@microsoft.com> writes:

[...]
>> It is a bit strange to me that the code uses get_be32 for reading, but
>> htonl for writing.  Is Git tested on non little-endian machines, like
>> big-endian ppc64 or s390x, or on mixed-endian machines (or
>> selectable-endian machines with data endianness set to non
>> little-endian, like ia64)?  If not, could we use for example openSUSE
>> Build Service (https://build.opensuse.org/) for this?
>
> Since we are packing two values into 64 bits, I am using htonl() here
> to arrange the 30-bit generation number alongside the 34-bit commit
> date value, then writing with hashwrite(). The other 32-bit integers
> are written with hashwrite_be32() to avoid translating this data
> in-memory.

O.K., so you are using what is more effective and easier to use.
Nice to know, thanks for the information.

[...]
>>>   +static void compute_generation_numbers(struct commit** commits,
>>> +				       int nr_commits)
>>> +{
[...]
>>> +	for (i = 0; i < nr_commits; i++) {
>>> +		if (commits[i]->generation != GENERATION_NUMBER_INFINITY &&
>>> +		    commits[i]->generation != GENERATION_NUMBER_ZERO)
>>> +			continue;

[...]

>>>   +	compute_generation_numbers(commits.list, commits.nr);
>>> +
>> Nice and simple.  All right.
>>
>> I guess that we do not pass "struct packed_commit_list commits" as
>> argument to compute_generation_numbers instead of "struct commit**
>> commits.list" and "int commits.nr" to compute_generation_numbers() to
>> keep the latter nice and generic?
>
> Good catch. There is no reason to not use packed_commit_list here.

Actually, now that v5 shows how using packed_commit_list looks like, in
my opinion it looks uglier.  And it might be easier to make mistake.

Also, depending on how compiler is able to optimize it, the version
passing packed_commit_list as an argument has one more indirection
(following two pointers) in the loop.

Best,
-- 
Jakub Narębski