Re: [PATCH v4 3/8] worktree: refactor infer_backlink return

[Caleb White <cdwhite3@pm.me>]

On Tue Nov 19, 2024 at 9:08 AM CST, Phillip Wood wrote:
> Hi Caleb
>
> Thanks for splitting this out into a separate patch
>
> On 01/11/2024 04:38, Caleb White wrote:
>> -static int infer_backlink(const char *gitfile, struct strbuf *inferred)
>> +static ssize_t infer_backlink(const char *gitfile, struct strbuf *inferred)
>>   {
>>   	struct strbuf actual = STRBUF_INIT;
>>   	const char *id;
>>
>> +	strbuf_reset(inferred);
>
> I think the code was clearer when we reset the buf just before using it.
> That way it is easy to see that we add the path to an empty buffer.

I moved it up to the top of the function to make it more clear that the
buffer is reset before being used. But I can move it back.

>>   	if (strbuf_read_file(&actual, gitfile, 0) < 0)
>>   		goto error;
>>   	if (!starts_with(actual.buf, "gitdir:"))
>> @@ -741,18 +744,16 @@ static int infer_backlink(const char *gitfile, struct strbuf *inferred)
>>   	id++; /* advance past '/' to point at <id> */
>>   	if (!*id)
>>   		goto error;
>> -	strbuf_reset(inferred);
>>   	strbuf_git_common_path(inferred, the_repository, "worktrees/%s", id);
>>   	if (!is_directory(inferred->buf))
>>   		goto error;
>>
>>   	strbuf_release(&actual);
>> -	return 1;
>> -
>> +	return inferred->len;
>>   error:
>>   	strbuf_release(&actual);
>>   	strbuf_reset(inferred); /* clear invalid path */
>> -	return 0;
>> +	return -1;
>
> Why don't we need to update the callers of this function to account for
> the new return value?

Originally this function was called inside an `if` statement, however,
another topic extracted the call to a separate line and so this return
was no longer used. I decided to keep the return anyway in case it was
useful in the future.


Best,

Caleb