Re: [PATCH v3 6/8] commit-reach: implement ahead_behind() logic

From: Derrick Stolee <derrickstolee@github.com>
Date: Mar 17, 2023, 18:44 UTC
Message-ID: <9bd7943c-a8a3-5022-bd7e-f95be8dc2ab5@github.com>
In-Reply-To: <20230315232835.173584-1-jonathantanmy@google.com>

On 3/15/2023 7:28 PM, Jonathan Tan wrote:

Show 6 quoted lines

> First of all, thanks to Taylor and Stolee for this algorithm and code
> - it is straightforwardly written and looks correct to me. I have some
> commit message and code comment suggestions that if taken, would have
> helped me on my first reading, but these are subjective so feel free
> to ignore them if you think they would add unnecessary detail (I did
> understand the algorithm in the end, after all).

I appreciate your comments here. I've done some reworking of the message based on what you say here, as well as the verbal feedback from review club.

Show 8 quoted lines

>> +void ahead_behind(struct repository *r,
>> +		  struct commit **commits, size_t commits_nr,
>> +		  struct ahead_behind_count *counts, size_t counts_nr)
>> +{
>> +	struct prio_queue queue = { .compare = compare_commits_by_gen_then_commit_date };
>> +	size_t width = (commits_nr + BITS_IN_EWORD - 1) / BITS_IN_EWORD;
> 
> As we discussed in our Review Club, DIV_ROUND_UP can be used for this.

Got it!

Show 9 quoted lines

>> +			if (bitmap_popcount(bitmap_p) == commits_nr)
>> +				p->item->object.flags |= STALE;
> 
> Might be worth adding a comment above the STALE line: this parent commit
> and all its ancestors can be reached by every commit in the commits
> list and thus can never be "ahead" or "behind" in any pair; mark this
> STALE so that, as an optimization, we can stop iteration if only STALE
> commits remain (since further iteration would never change any "ahead"
> or "behind" value).

This is a helpful thing to point out, so a comment is appropriate.

Overall, maybe algorithms like this should have more inline comments than we typically expect in the Git codebase. We want to make sure that these things are readable in the future, hopefully without digging too far in the history to find the lengthy commit message about it.

I'll delay sending v4 until giving a little time to hear back on this point. My default is to not add the comments, but I'd be happy to, if we think this is an appropriate time to deviate from the standard.

Thanks, -Stolee