Re: [PATCH 3/4] fetch: use new branch_checked_out() and add tests

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Wed, Jun 08 2022, Derrick Stolee via GitGitGadget wrote:

> From: Derrick Stolee <derrickstolee@github.com>

re a comment on 1/4...

>  	struct commit *current = NULL, *updated;
> -	const struct worktree *wt;
> +	char *path = NULL;

Init'd to NULL here...

>  	const char *pretty_ref = prettify_refname(ref->name);
>  	int fast_forward = 0;
>  
> @@ -900,17 +900,17 @@ static int update_local_ref(struct ref *ref,
>  	}
>  
>  	if (!update_head_ok &&
> -	    (wt = find_shared_symref(worktrees, "HEAD", ref->name)) &&
> -	    !wt->is_bare && !is_null_oid(&ref->old_oid)) {
> +	    !is_null_oid(&ref->old_oid) &&
> +	    branch_checked_out(ref->name, &path)) {
>  		/*
>  		 * If this is the head, and it's not okay to update
>  		 * the head, and the old value of the head isn't empty...
>  		 */
>  		format_display(display, '!', _("[rejected]"),
> -			       wt->is_current ?
> -				       _("can't fetch in current branch") :
> -				       _("checked out in another worktree"),
> +			       path ? _("can't fetch in current branch") :
> +				      _("checked out in another worktree"),
>  			       remote, pretty_ref, summary_width);
> +		free(path);
>  		return 1;

..but only used if branch_checkoed_out() returns non-zero, but (and I've
only eyeballed this, not run it) isn't it impossible for that function
not to return successfully and have "path" be non-NULL?

This seems to be a defense against the underlying strmap_get() starting
to return nonsensical results, but if we instead just trust it...

Also re the "can't we just expose the strmap?" comment on 1/4, here we
strmap_get() it, and xstrdup() and free() it, but we never needed our
own copy, or even cared about the string at all, we're just checking
"was it in the set?".

Which is what strmap_contains() would give us, if we just exposed it at
that level. So:

	struct strmap *map = make_my_branchs_map_thing();

        [...]
        if ([...] && strmap_contains(map, ref->name)) {
	        ...

Seems more straightforward, no?

Looking at the other callers it seems none of them need the xstrdup() at
all, if looking at the end state. This one is a strmp_contains(), then
an error() which could use strmap_get() without the xstrdup(), as we
format it immediately.

The remaining two are calling die() right afterwards, which ditto can
use a plain strmap_get() without xstrdup().

It's not that I care about the extra xstrdup(), which is obviously
trivial, it's just harder to read & reason about APIs which e.g. "&&
path", and strdup(), only to find that apparently no user in-tree cared
about those...

Maybe they'll be needed, but let's just add them then, and in the
meantime aim for simpler? :)