Re: [PATCH v2 2/5] commit-reach: use one walk in remove_redundant()

From: Derrick Stolee <stolee@gmail.com>
Date: Feb 1, 2021, 16:31 UTC
Message-ID: <5236b427-d10c-acaf-e8c4-97011bc68c49@gmail.com>
In-Reply-To: <419ddcca-8534-8dc5-bcd8-1e244414bcf1@web.de>

On 2/1/2021 11:12 AM, René Scharfe. wrote:

Show 11 quoted lines

> Am 01.02.21 um 13:47 schrieb Derrick Stolee via GitGitGadget:
>> @@ -210,12 +204,110 @@ static int remove_redundant(struct repository *r, struct commit **array, int cnt
>>  	for (i = filled = 0; i < cnt; i++)
>>  		if (!redundant[i])
>>  			array[filled++] = work[i];
>> +	for (j = filled, i = 0; i < cnt; i++)
>> +		if (redundant[i])
>> +			array[j++] = work[i];
> 
> This puts the loop back in that you removed in the previous commit.
> Intentionally?

Not intentional. Thanks for noticing.

Show 7 quoted lines

>> +	/* rearrange array */
>> +	for (i = count_non_stale = 0; i < cnt; i++) {
>> +		if (!(array[i]->object.flags & STALE))
> 
> Here I would have added another condition, count_non_stale != i, to
> avoid self-assignment (array[x] = array[x]).  The code works without
> it, though.  Not sure if there is a performance benefit to be had --> branch vs. pointer copy.  Probably not worth it..

You are correct, but I'm going to go on the side of not worth it.

Show 8 quoted lines

>> +			array[count_non_stale++] = array[i];
>> +	}
>> +
>> +	/* clear marks */
>> +	for (i = 0; i < walk_start_nr; i++)
>> +		clear_commit_marks(walk_start[i], STALE);
> 
> You can replace this loop with a call to clear_commit_marks_many().

Right! Earlier I was using a 'struct commit_list *' which would not work, but this 'struct commit ** walk_start' does work. Thanks.

-Stolee