Re: [PATCH v5 1/1] worktree add: sanitize worktree names

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 11, 2019, 06:36 UTC
Message-ID: <xmqqtvg9kjdb.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20190308092834.12549-2-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Update 'worktree add' code to remove special characters to follow
> these rules. In the future the user will be able to specify the
> worktree name by themselves if they're not happy with this dumb
> character substitution.

This replaces both of the two patches in v4, and applies to a more recent tip of 'master' (post 7d0c1f4556).

Show 8 quoted lines

> diff --git a/refs.c b/refs.c
> index 142888a40a..e9f83018f0 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -72,30 +72,57 @@ static unsigned char refname_disposition[256] = {
>   * - it ends with ".lock", or
>   * - it contains a "@{" portion
>   */

The comment above needs modernizing (see attached at the end).

Show 6 quoted lines

>  		case 2:
> -			if (last == '.')
> -				return -1; /* Refname contains "..". */
> +			if (last == '.') { /* Refname contains "..". */
> +				if (sanitized)
> +					sanitized->len--; /* collapse ".." to single "." */

As Eric points out, this needs to be fixed.

I'll use the strbuf_setlen() version suggested by Eric in the meantime, but "sanitized->buf[sanitized->len-1] = '-'" as done to everything else in the function may be a better idea, especially since they'll be able to name the worktree themselves in the future anyway.

> +
> +	if (refname[0] == '.') { /* Component starts with '.'. */
> +		if (sanitized)
> +			sanitized->buf[component_start] = '-';

... and a dot turns into a dash in some cases anyway.

Show 13 quoted lines

> +		else
> +			return -1;
> +	}
>  	if (cp - refname >= LOCK_SUFFIX_LEN &&
> -	    !memcmp(cp - LOCK_SUFFIX_LEN, LOCK_SUFFIX, LOCK_SUFFIX_LEN))
> -		return -1; /* Refname ends with ".lock". */
> +	    !memcmp(cp - LOCK_SUFFIX_LEN, LOCK_SUFFIX, LOCK_SUFFIX_LEN)) {
> +		if (!sanitized)
> +			return -1;
> +		/* Refname ends with ".lock". */
> +		while (strbuf_strip_suffix(sanitized, LOCK_SUFFIX)) {
> +			/* try again in case we have .lock.lock */
> +		}

No need for {}; just have an empty statment

		while (...) 
			; /* try again ... */

This "strip all .lock repeatedly" made me stop and think a bit; this will never make the component empty, as the only way for this loop to make it empty is if we have a string that match "^\(.lock)\*$" in it, but the first dot would have already been turned into a dash, so we'll end up with "-lock", which is not empty.

> +	}
>  	return cp - refname;
>  }

See below for a possible further polishment.

 * The first hunk is not about this patch but a long-standing issue
   after the comment was given to this function for a single level
   (I do not know or care how it happened--perhaps we had a single
   function that verifies multiple levels which later was split into
   a caller that loops and this function that checks a single level,
   and the comment for the multi-level function was left stale).

 * check_refname_component() can now try to sanitize; document it.

 * The last hunk is from Eric's comment.

 refs.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/refs.c b/refs.c
index e9f83018f0..3a1b2a8c3c 100644
--- a/refs.c
+++ b/refs.c
@@ -63,7 +63,7 @@ static unsigned char refname_disposition[256] = {
  * not legal.  It is legal if it is something reasonable to have under
  * ".git/refs/"; We do not like it if:
  *
- * - any path component of it begins with ".", or
+ * - it begins with ".", or
  * - it has double dots "..", or
  * - it has ASCII control characters, or
  * - it has ":", "?", "[", "\", "^", "~", SP, or TAB anywhere, or
@@ -71,6 +71,10 @@ static unsigned char refname_disposition[256] = {
  * - it ends with a "/", or
  * - it ends with ".lock", or
  * - it contains a "@{" portion
+ *
+ * When sanitized is not NULL, instead of rejecting the input refname
+ * as an error, try to come up with a usable replacement for the input
+ * refname in it.
  */
 static int check_refname_component(const char *refname, int *flags,
 				   struct strbuf *sanitized)
@@ -95,7 +99,8 @@ static int check_refname_component(const char *refname, int *flags,
 		case 2:
 			if (last == '.') { /* Refname contains "..". */
 				if (sanitized)
-					sanitized->len--; /* collapse ".." to single "." */
+					/* collapse ".." to single "." */
+					strbuf_setlen(sanitized, sanitized->len - 1);
 				else
 					return -1;
 			}