Re: [PATCH v5 1/4] fetch: Protect branches checked out in all worktrees

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 10, 2021, 23:46 UTC
Message-ID: <xmqq8rxvwp4b.fsf@gitster.g>
In-Reply-To: <nycvar.QRO.7.76.6.2111101315330.21127@tvgsbejvaqbjf.bet>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> ... (apart from the slightly iffy assumption that `buf->ref`
> starts with `refs/heads/` and therefore `buf->ref + strlen("refs/heads/")`
> would not overrun, but I _think_ the current code enforces that prefix
> somewhere along the lines)

I think that is in 4/4, where the existing code does this:

Show 13 quoted lines

> diff --git a/branch.c b/branch.c
> index 7a88a4861e..1aaf694b39 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -199,18 +199,20 @@ int validate_branchname(const char *name, struct strbuf *ref)
>   */
>  int validate_new_branchname(const char *name, struct strbuf *ref, int force)
>  {
> -	const char *head;
> +	const struct worktree *wt;
>  
>  	if (!validate_branchname(name, ref))
>  		return 0;

This takes a bare branch name in "name" (or a shorthand like @{-1}), expand that into a full refname into "ref". Before passing the ref into check_refname_format(), "refs/heads/" is unconditionally added at the beginning. So we know ref begins with "refs/heads/" after this point.

>  	if (!force)
>  		die(_("A branch named '%s' already exists."),
>  		    ref->buf + strlen("refs/heads/"));

And we already assume ref->buf has "refs/heads/" as its prefix. It may be nice to use skip_prefix(), but it probably is not worth it.

Show 5 quoted lines

> +	wt = find_shared_symref("HEAD", ref->buf);
> +	if (wt && !wt->is_bare)
> +		die(_("Cannot force update the branch '%s'"
> +		      "checked out at '%s'."),
> +		    ref->buf + strlen("refs/heads/"), wt->path);

And this new use just reuses what we assume to be valid.

So, correctness-wise, I do not think there is much to tweak further on top of this round. I've always queued this round more or less as-is.

In preparation for the next development cycle, however, it might make sense to add a preparatory clean-up step to downcase the first word of "die()" messages in the files that are involved in this series (not necessarily the ones that are touched by the patches, but all of them) and then apply these four patches (with matching adjustments, like "Cannot force update" -> "cannot force update") on top. In another review message, I also noticed some inefficient code that is due to insufficient support from the worktree.c API, but that is not about correctness and can be left out of the series to get these fixes early in the next cycle.

Thanks.