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

[Junio C Hamano <gitster@pobox.com>]

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:

> 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.

> +	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.