From: Phillip Wood <phillip.wood123@gmail.com>
Date: Tue, 07 Feb 2023 10:50:12 GMT
Subject: Re: [PATCH v3 2/4] branch: fix die_if_checked_out() when ignore_current_worktree
Message-ID: <1da499d6-3de3-ba34-1ca0-6f39e528b80b@dunelm.org.uk>
In-Reply-To: <230206.861qn27mt9.gmgdl@evledraar.gmail.com>

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. 
The only concern would be that there are more than INT_MAX which seems 
unlikely.

Best Wishes

Phillip

>> @@ -435,10 +435,9 @@ const struct worktree *find_shared_symref(struct worktree **worktrees,
>>   					  const char *target)
>>   {
>>   
>> -	for (int i = 0; worktrees[i]; i++) {
>> +	for (int i = 0; worktrees[i]; i++)
>>   		if (is_shared_symref(worktrees[i], symref, target))
>>   			return worktrees[i];
>> -	}
> 
> You added this function in the last commit, let's just skip adding the
> braces to begin with, rather than this style-fix after the fact.