Re: [PATCH 1/4] branch: add branch_checked_out() helper

From: Derrick Stolee <derrickstolee@github.com>
Date: Jun 14, 2022, 13:48 UTC
Message-ID: <526fd647-4d8c-7773-06e3-011db173fe28@github.com>
In-Reply-To: <42ada1a3-adc2-2c43-9aa1-7854858865ae@gmail.com>

On 6/14/2022 6:09 AM, Phillip Wood wrote:

> Hi Stolee
> 
> On 08/06/2022 21:08, Derrick Stolee via GitGitGadget wrote:
>> From: Derrick Stolee <derrickstolee@github.com>

Show 6 quoted lines

>> +        if (wt->head_ref)
>> +            strmap_put(&current_checked_out_branches,
>> +                   wt->head_ref,
>> +                   xstrdup(wt->path));
> 
> STRMAP_INIT sets .strdup_strings = 1, so the xstrdup() is unnecessary.

That .strdup_strings is only for the key, not the value (since the value could be anything, not necessarily a string).

Show 30 quoted lines

>> +    }
>> +
>> +    free_worktrees(worktrees);
>> +}
>> +
>> +int branch_checked_out(const char *refname, char **path)
>> +{
>> +    const char *path_in_set;
>> +    prepare_checked_out_branches();
>> +
>> +    path_in_set = strmap_get(&current_checked_out_branches, refname);
>> +    if (path_in_set && path)
>> +        *path = xstrdup(path_in_set);
>> +
>> +    return !!path_in_set;
>> +}
> 
> I like the idea of having a specific function to see if a branch is
> checkout out rather than Ævar's suggestion of forcing all callers to do
>     strmap_get(get_worktree_refs_strmap(), ref)
> which will quickly get tiresome. I do wonder though if we'd be better
> off with a thin wrapper around strmap_get() such as
> 
> const char* branch_checked_out(const char *refname)
> {
>     prepare_checked_out_branches();
>     return strmap_get(&current_checked_out_branches, refname);
> }
> 
> so that the user can choose whether to copy the path or not.

This is an interesting suggestion. It changes callers a bit, but not too much. The pattern currently is:

	if (branch_checked_out(name, &path)) {
		/* do something */
		free(path);
	}

but would become

	if ((path = branch_checked_out(name))) {
		/* do something */
	}

Sounds good to me.

Thanks, -Stolee