From: Phillip Wood <phillip.wood123@gmail.com>
Date: Tue, 19 Nov 2024 15:08:11 GMT
Subject: Re: [PATCH v4 3/8] worktree: refactor infer_backlink return
Message-ID: <977c1e75-1fef-4827-91ae-cc1a6ce7508e@gmail.com>
In-Reply-To: <20241031-wt_relative_options-v4-3-07a3dc0f02a3@pm.me>

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.

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

Best Wishes

Phillip