On 6/13/2022 7:59 PM, Ævar Arnfjörð Bjarmason wrote: > > On Wed, Jun 08 2022, Derrick Stolee via GitGitGadget wrote: > >> From: Derrick Stolee <derrickstolee@xxxxxxxxxx> >> [...] >> + path_in_set = strmap_get(¤t_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'. > 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
