Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> 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).
> 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).
> 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.
> + 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;
}
