Re: [PATCH v3 2/4] branch: fix die_if_checked_out() when ignore_current_worktree

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Feb 7, 2023, 12:58 UTC
Message-ID: <230207.86wn4t37k6.gmgdl@evledraar.gmail.com>
In-Reply-To: <1da499d6-3de3-ba34-1ca0-6f39e528b80b@dunelm.org.uk>

On Tue, Feb 07 2023, Phillip Wood wrote:

Show 17 quoted lines

> 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.