Re: [PATCH v2] last-modified: implement faster algorithm

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 22, 2025, 03:48 UTC
Message-ID: <xmqqecqv1trk.fsf@gitster.g>
In-Reply-To: <aPgkwnq87UeusC6v@nand.local>

Taylor Blau <me@ttaylorr.com> writes:

Show 19 quoted lines

> On Tue, Oct 21, 2025 at 10:52:11AM -0700, Junio C Hamano wrote:
>> > +		struct diff_filepair *fp = diff_queued_diff.queue[i];
>> > +		size_t k = path_idx(lm, fp->two->path);
>> > +		if (0 <= k && bitmap_get(active_c, k))
>> > +			bitmap_set(lm->scratch, k);
>> > +	}
>>
>> Earlier path_idx() wanted to signal an error by returning negative,
>> but the type is size_t that is unsigned so it cannot do so.  We
>> instead get
>>
>>     builtin/last-modified.c:307:23: error: comparison of unsigned expression in '>= 0' is always true [-Werror=type-limits]
>>       307 |                 if (0 <= k && bitmap_get(active_c, k))
>>           |                       ^~
>> ...
> Yeah, this is a true positive. I was curious if GitHub's version of the
> code also returned "-1" from a function whose return type is unsigned,
> and in fact our version of this function (called diff2idx()) returns an
> 'int'.

Yes, I think the tool is doing the right thing here, unlike "hey you are comparing int with size_t" we saw earlier, and is giving us a useful diagnosis.

> Practically speaking that's probably OK, since we are unlikely to have
> so many active paths anyway (or if we did, we'd likely have other
> problems to deal with ;-)), but it is gross nonetheless.

The case path_idx() returns -1 is an error case, not "there are too many paths we are following" case. I do not see what relevance the number of active paths has here.