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

From: Derrick Stolee <derrickstolee@github.com>
Date: Jun 14, 2022, 13:32 UTC
Message-ID: <4fc234ab-de06-9e96-daf6-7b3496132541@github.com>
In-Reply-To: <220614.86leu0yur8.gmgdl@evledraar.gmail.com>

On 6/13/2022 7:59 PM, Ævar Arnfjörð Bjarmason wrote:

Show 14 quoted lines

> 
> On Wed, Jun 08 2022, Derrick Stolee via GitGitGadget wrote:
> 
>> From: Derrick Stolee <derrickstolee@github.com>
>> [...]
>> +	path_in_set = strmap_get(&current_checked_out_branches, refname);
>> +	if (path_in_set && path)
>> +		*path = xstrdup(path_in_set);
> 
> Looking at the end state of this series there is nothing that passes a
> null "path" to this function, i.e. it's all &some_variable.
> 
> So just dropping that && seems better from a code understanding &
> analysis perspective.

I don't see the value in making this method more prone to error in
the future, or less flexible to a possible caller that doesn't want
to give a 'path'.

Show 9 quoted lines

> More generally, can't we just expose an API that gives us the strmap
> itself, so that callers don't need to keep any special-behavior in mind?
> I.e. just "make me a strmap of checked out branches".
> 
> Then you can just use strmap_get(), or xstrdup(strmap_get()), and if the
> pattern of "get me the item but duped" is found elsewhere maybe we
> should eventually have a strmap_get_dup() or whatever.
> 
> I.e. just making it: xstrdup(strmap_get(checked_out_branches_strmap(), refname)).

This seems unnecessarily complicated. It also risks someone inserting key-value pairs in an improper place instead of being contained to prepare_checked_out_branches().

If your concern is about creating the static current_checked_out_branches, keep in mind that the callers that call branch_checked_out() in a loop would need to keep track of that strmap across several other methods. That additional complexity is much worse than asking for a simple answer through the black box of branch_checked_out().

Thanks, -Stolee