On Tue, Feb 07 2023, Phillip Wood wrote:
> On 06/02/2023 16:56, Ævar Arnfjörð Bjarmason wrote:
>> On Sun, Feb 05 2023, Rubén Justo wrote:
>>
>>> - wt = find_shared_symref(worktrees, "HEAD", branch);
>>> - if (wt && (!ignore_current_worktree || !wt->is_current)) {
>>> - skip_prefix(branch, "refs/heads/", &branch);
>>> - die(_("'%s' is already checked out at '%s'"), branch, wt->path);
>>> + for (int i = 0; worktrees[i]; i++) {
>> I see that there are existing "int i" for counting worktrees in
>> worktree.c, FWIW for new code I wouldn't mind if it's "size_t i"
>> instead, to make it future proof (and to eventually get rid of cast
>> warnings as we move more things from "int" to "size_t").
>
> This seems to be different from the usual worries about int/size_t
> comparisons/truncation. worktrees is NULL terminated so there is no
> signed/unsigned comparison here as we're not comparing it to
> anything.
Having looked at this again I think using "int" for now is the right
thing, sorry about the noise.
> The only concern would be that there are more than INT_MAX
> which seems unlikely.
My general concern isn't just with the code that we can prove doesn't
overflow in such cases, but that by having different types for such
things (which isn't the case here, I thought our "struct worktrees **"
would be alloc'd with a "size_t") we end up with coercion warnings.
Those warnings are so prevalent in this codebase that we've had to
suppress them, and consequently we make it harder to spot the real
issues.
