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
