From: Taylor Blau <me@ttaylorr.com>
Date: Mon, 27 Oct 2025 19:22:30 GMT
Subject: Re: [PATCH v2] last-modified: implement faster algorithm
Message-ID: <aP/Gdl0kGJWklZdO@nand.local>
In-Reply-To: <xmqqtszpqgmy.fsf@gitster.g>

On Thu, Oct 23, 2025 at 05:37:25PM -0700, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > On Tue, Oct 21, 2025 at 08:48:31PM -0700, Junio C Hamano wrote:
> >> > 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.
> >
> > I just meant that we are unlikely to ever have so many active paths at
> > once that (size_t)-1 would actually have a valid entry, or IOW that
> > active_paths_nr is smaller than 2^32-1.
>
> So?  If path_idx() needed to signal an error, it will return (size_t)-1,
> but as the compiler correctly caught, the code as written, i.e.
>
> 	k = path_idx(...);
> 	if (0 <= k) {
> 		/* did not error so we can safely use k */
> 		...
> 	}
>
> is outright buggy. I do not see why it is "practically speaking
> that's probably OK".  It certainly does not matter if the number we
> will receive in 'k' in the success case is expected to be
> small---the problem is only for an error case.

I am not saying that we should continue to write "if (0 <= k)", since we
will clearly never take the else branch. I am trying to say that
path_idx() *could* return (size_t)-1, and callers would be able to write
"if (k == (size_t)-1)" to check for that error condition.

My observation was that there are unlikely to be so many active paths at
any one time such that we'd ever want to return (size_t)-1 as a valid
index, and could always use it as an error sentinel.

Thanks,
Taylor