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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jun 14, 2022, 00:05 UTC
Message-ID: <220614.86h74oyuc8.gmgdl@evledraar.gmail.com>
In-Reply-To: <4540dbeed385341f8c5b45134e1a65dc48c75b0c.1654718942.git.gitgitgadget@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...

Show 24 quoted lines

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