| Message ID | 20190308092834.12549-2-pclouds@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | worktree add: sanitize worktree names | expand |
On Fri, Mar 8, 2019 at 4:28 AM Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote: > Worktree names are based on $(basename $GIT_WORK_TREE). They aren't > significant until 3a3b9d8cde (refs: new ref types to make per-worktree > refs visible to all worktrees - 2018-10-21), where worktree name could > be part of a refname and must follow refname rules. > > 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. > > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> > --- > diff --git a/refs.c b/refs.c > @@ -72,30 +72,57 @@ static unsigned char refname_disposition[256] = { > +static int check_refname_component(const char *refname, int *flags, > + struct strbuf *sanitized) > { > for (cp = refname; ; cp++) { > unsigned char disp = refname_disposition[ch]; > + if (sanitized && disp != 1) > + strbuf_addch(sanitized, ch); > + > switch (disp) { > case 1: > goto out; > case 2: > + if (last == '.') { /* Refname contains "..". */ > + if (sanitized) > + sanitized->len--; /* collapse ".." to single "." */ I think this needs to be: strbuf_setlen(sanitized, sanitized->len - 1); to ensure that NUL-terminator ends up in the correct place if this "." is the very last character in 'refname'. (Otherwise, the NUL will remain after the second ".", thus ".." won't be collapsed to "." at all.) > + else > + return -1; > + } > break;
Eric Sunshine <sunshine@sunshineco.com> writes: >> case 2: >> + if (last == '.') { /* Refname contains "..". */ >> + if (sanitized) >> + sanitized->len--; /* collapse ".." to single "." */ > > I think this needs to be: > > strbuf_setlen(sanitized, sanitized->len - 1); > > to ensure that NUL-terminator ends up in the correct place if this "." > is the very last character in 'refname'. (Otherwise, the NUL will > remain after the second ".", thus ".." won't be collapsed to "." at > all.) True. Why doesn't it do the similar "replace with -" it does for other unfortunate characters, though?
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). > 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; }
On Mon, Mar 11, 2019 at 1:20 PM Junio C Hamano <gitster@pobox.com> wrote: > > Eric Sunshine <sunshine@sunshineco.com> writes: > > >> case 2: > >> + if (last == '.') { /* Refname contains "..". */ > >> + if (sanitized) > >> + sanitized->len--; /* collapse ".." to single "." */ > > > > I think this needs to be: > > > > strbuf_setlen(sanitized, sanitized->len - 1); > > > > to ensure that NUL-terminator ends up in the correct place if this "." > > is the very last character in 'refname'. (Otherwise, the NUL will > > remain after the second ".", thus ".." won't be collapsed to "." at > > all.) > > True. Why doesn't it do the similar "replace with -" it does for > other unfortunate characters, though? > I think Jeff saw an opportunity to keep it cleaner ("." looks better than ".-") and took it.
On Mon, Mar 11, 2019 at 1:36 PM Junio C Hamano <gitster@pobox.com> wrote: > 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. Yep. I added a BUG() check anyway in worktree.c just in case something slips through in the future. > > + } > > 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. Thanks.
Hi Duy, On Fri, 8 Mar 2019, Nguyễn Thái Ngọc Duy wrote: > 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 > */ > -static int check_refname_component(const char *refname, int *flags) > +static int check_refname_component(const char *refname, int *flags, > + struct strbuf *sanitized) > { > const char *cp; > char last = '\0'; > + size_t component_start; This variable is uninitialized. It is then... > + > + if (sanitized) > + component_start = sanitized->len; ... initialized only when `sanitized` is not `NULL`, and subsequently... > > for (cp = refname; ; cp++) { > int ch = *cp & 255; > unsigned char disp = refname_disposition[ch]; > + > + if (sanitized && disp != 1) > + strbuf_addch(sanitized, ch); > + > switch (disp) { > case 1: > goto out; > case 2: > - if (last == '.') > - return -1; /* Refname contains "..". */ > + if (last == '.') { /* Refname contains "..". */ > + if (sanitized) > + sanitized->len--; /* collapse ".." to single "." */ > + else > + return -1; > + } > break; > case 3: > - if (last == '@') > - return -1; /* Refname contains "@{". */ > + if (last == '@') { /* Refname contains "@{". */ > + if (sanitized) > + sanitized->buf[sanitized->len-1] = '-'; > + else > + return -1; > + } > break; > case 4: > - return -1; > + /* forbidden char */ > + if (sanitized) > + sanitized->buf[sanitized->len-1] = '-'; > + else > + return -1; > + break; > case 5: > - if (!(*flags & REFNAME_REFSPEC_PATTERN)) > - return -1; /* refspec can't be a pattern */ > + if (!(*flags & REFNAME_REFSPEC_PATTERN)) { > + /* refspec can't be a pattern */ > + if (sanitized) > + sanitized->buf[sanitized->len-1] = '-'; > + else > + return -1; > + } > > /* > * Unset the pattern flag so that we only accept > @@ -109,26 +136,48 @@ static int check_refname_component(const char *refname, int *flags) > out: > if (cp == refname) > return 0; /* Component has zero length. */ > - if (refname[0] == '.') > - return -1; /* Component starts with '.'. */ > + > + if (refname[0] == '.') { /* Component starts with '.'. */ > + if (sanitized) > + sanitized->buf[component_start] = '-'; ... used a loooooooong time after that, also only if `sanitized` is not `NULL`. Apparently for some GCC versions, this is too cute, and it complains that this variable might be used uninitialized: https://dev.azure.com/gitgitgadget/git/_build/results?buildId=4352&view=logs And quite honestly, even for mere humans it is not all *that* clear that `sanitized` cannot be changed from `NULL` to non-`NULL` in the code in between, *in particular* because the changes extend over two hunks, the code between is not shown. I would strongly advise against trying to be so cute, and just initialize the variable already. Over-optimization in such instances makes the code a lot harder to reason about. Ciao, Johannes
On Mon, Mar 11, 2019 at 04:24:13PM +0700, Duy Nguyen wrote: > > > I think this needs to be: > > > > > > strbuf_setlen(sanitized, sanitized->len - 1); > > > > > > to ensure that NUL-terminator ends up in the correct place if this "." > > > is the very last character in 'refname'. (Otherwise, the NUL will > > > remain after the second ".", thus ".." won't be collapsed to "." at > > > all.) > > > > True. Why doesn't it do the similar "replace with -" it does for > > other unfortunate characters, though? > > I think Jeff saw an opportunity to keep it cleaner ("." looks better > than ".-") and took it. Yeah, that was one thing I was going to comment on your patch. The "rules" I made up were pretty ad-hoc as I was walking through the function (note it also drops ".lock" instead of sanitizing it into "-lock"). But it may make sense to make things more consistent (even if the result isn't entirely reversible). Another option _is_ to actually make it reversible. I.e., use "%2e" instead of ".", which would also necessitate replacing "%". I don't know if that has a huge value for this use-case, but it's a nice property that two sanitized names can't collide (unless they originally identical). -Peff
Jeff King <peff@peff.net> writes: > Another option _is_ to actually make it reversible. I.e., use "%2e" > instead of ".", which would also necessitate replacing "%". I don't know > if that has a huge value for this use-case, but it's a nice property > that two sanitized names can't collide (unless they originally > identical). Yeah. That is a good property to have.
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: >> +static int check_refname_component(const char *refname, int *flags, >> + struct strbuf *sanitized) >> { >> const char *cp; >> char last = '\0'; >> + size_t component_start; > > This variable is uninitialized. It is then... > >> + >> + if (sanitized) >> + component_start = sanitized->len; > > ... initialized only when `sanitized` is not `NULL`, and subsequently... > ... >> + if (refname[0] == '.') { /* Component starts with '.'. */ >> + if (sanitized) >> + sanitized->buf[component_start] = '-'; > ... > ... used a loooooooong time after that, also only if `sanitized` is not > `NULL`. > > Apparently for some GCC versions, this is too cute, and it complains that It does require humans (well, at least it did to this one) to be careful when reading the code to know that component_start is valid when it is used. There unfortunately is no good "default" value to initialize the variable to. When checking a later component in a series of components, it would be looking at non-zero position, so even initializing it to 0 in this function is *not* a more sensible fallback default value than any other random garbage value (which would squelch the compiler, but it would mislead the humans nevertheless). And that (i.e. the lack of any sensible default value when sanitized is NULL) is the reason why the variable is left uninitialized by the patch, I think. I do not think the code is trying to be cute at all. I wonder if we make the caller pass a pointer to struct { struct strbuf result; size_t component_start; } sanitized; sanitized.component_start = sanitized.result.len check_refname_component(refname, flags, &sanitized); and get rid of the assignment to component_start done by the callee, it would appease compilers and makes the code easier to vet. It does introduce one more ad-hoc type, which is a certain downside. I dunno.
diff --git a/builtin/worktree.c b/builtin/worktree.c index 6cc094a453..756cf3a417 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -275,6 +275,7 @@ static int add_worktree(const char *path, const char *refname, struct strbuf symref = STRBUF_INIT; struct commit *commit = NULL; int is_branch = 0; + struct strbuf sb_name = STRBUF_INIT; validate_worktree_add(path, opts); @@ -290,7 +291,13 @@ static int add_worktree(const char *path, const char *refname, die(_("invalid reference: %s"), refname); name = worktree_basename(path, &len); - git_path_buf(&sb_repo, "worktrees/%.*s", (int)(path + len - name), name); + strbuf_add(&sb, name, path + len - name); + sanitize_refname_component(sb.buf, &sb_name); + if (!sb_name.len) + BUG("How come '%s' becomes empty after sanitization?", sb.buf); + strbuf_reset(&sb); + name = sb_name.buf; + git_path_buf(&sb_repo, "worktrees/%s", name); len = sb_repo.len; if (safe_create_leading_directories_const(sb_repo.buf)) die_errno(_("could not create leading directories of '%s'"), @@ -416,6 +423,7 @@ static int add_worktree(const char *path, const char *refname, strbuf_release(&symref); strbuf_release(&sb_repo); strbuf_release(&sb_git); + strbuf_release(&sb_name); return ret; } 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 */ -static int check_refname_component(const char *refname, int *flags) +static int check_refname_component(const char *refname, int *flags, + struct strbuf *sanitized) { const char *cp; char last = '\0'; + size_t component_start; + + if (sanitized) + component_start = sanitized->len; for (cp = refname; ; cp++) { int ch = *cp & 255; unsigned char disp = refname_disposition[ch]; + + if (sanitized && disp != 1) + strbuf_addch(sanitized, ch); + switch (disp) { case 1: goto out; case 2: - if (last == '.') - return -1; /* Refname contains "..". */ + if (last == '.') { /* Refname contains "..". */ + if (sanitized) + sanitized->len--; /* collapse ".." to single "." */ + else + return -1; + } break; case 3: - if (last == '@') - return -1; /* Refname contains "@{". */ + if (last == '@') { /* Refname contains "@{". */ + if (sanitized) + sanitized->buf[sanitized->len-1] = '-'; + else + return -1; + } break; case 4: - return -1; + /* forbidden char */ + if (sanitized) + sanitized->buf[sanitized->len-1] = '-'; + else + return -1; + break; case 5: - if (!(*flags & REFNAME_REFSPEC_PATTERN)) - return -1; /* refspec can't be a pattern */ + if (!(*flags & REFNAME_REFSPEC_PATTERN)) { + /* refspec can't be a pattern */ + if (sanitized) + sanitized->buf[sanitized->len-1] = '-'; + else + return -1; + } /* * Unset the pattern flag so that we only accept @@ -109,26 +136,48 @@ static int check_refname_component(const char *refname, int *flags) out: if (cp == refname) return 0; /* Component has zero length. */ - if (refname[0] == '.') - return -1; /* Component starts with '.'. */ + + if (refname[0] == '.') { /* Component starts with '.'. */ + if (sanitized) + sanitized->buf[component_start] = '-'; + 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 */ + } + } return cp - refname; } -int check_refname_format(const char *refname, int flags) +static int check_or_sanitize_refname(const char *refname, int flags, + struct strbuf *sanitized) { int component_len, component_count = 0; - if (!strcmp(refname, "@")) + if (!strcmp(refname, "@")) { /* Refname is a single character '@'. */ - return -1; + if (sanitized) + strbuf_addch(sanitized, '-'); + else + return -1; + } while (1) { + if (sanitized && sanitized->len) + strbuf_complete(sanitized, '/'); + /* We are at the start of a path component. */ - component_len = check_refname_component(refname, &flags); - if (component_len <= 0) + component_len = check_refname_component(refname, &flags, + sanitized); + if (sanitized && component_len == 0) + ; /* OK, omit empty component */ + else if (component_len <= 0) return -1; component_count++; @@ -138,13 +187,29 @@ int check_refname_format(const char *refname, int flags) refname += component_len + 1; } - if (refname[component_len - 1] == '.') - return -1; /* Refname ends with '.'. */ + if (refname[component_len - 1] == '.') { + /* Refname ends with '.'. */ + if (sanitized) + ; /* omit ending dot */ + else + return -1; + } if (!(flags & REFNAME_ALLOW_ONELEVEL) && component_count < 2) return -1; /* Refname has only one component. */ return 0; } +int check_refname_format(const char *refname, int flags) +{ + return check_or_sanitize_refname(refname, flags, NULL); +} + +void sanitize_refname_component(const char *refname, struct strbuf *out) +{ + if (check_or_sanitize_refname(refname, REFNAME_ALLOW_ONELEVEL, out)) + BUG("sanitizing refname '%s' check returned error", refname); +} + int refname_is_safe(const char *refname) { const char *rest; diff --git a/refs.h b/refs.h index 308fa1f03b..4d8c5465c3 100644 --- a/refs.h +++ b/refs.h @@ -460,6 +460,12 @@ int for_each_reflog(each_ref_fn fn, void *cb_data); */ int check_refname_format(const char *refname, int flags); +/* + * Apply the rules from check_refname_format, but mutate the result until it + * is acceptable, and place the result in "out". + */ +void sanitize_refname_component(const char *refname, struct strbuf *out); + const char *prettify_refname(const char *refname); char *shorten_unambiguous_ref(const char *refname, int strict); diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh index 286bba35d8..c989dbe321 100755 --- a/t/t2400-worktree-add.sh +++ b/t/t2400-worktree-add.sh @@ -570,4 +570,9 @@ test_expect_success '"add" an existing locked but missing worktree' ' git worktree add --force --force --detach gnoo ' +test_expect_success FUNNYNAMES 'sanitize generated worktree name' ' + git worktree add --detach ". weird*..?.lock.lock" && + test -d .git/worktrees/---weird-.- +' + test_done
Worktree names are based on $(basename $GIT_WORK_TREE). They aren't significant until 3a3b9d8cde (refs: new ref types to make per-worktree refs visible to all worktrees - 2018-10-21), where worktree name could be part of a refname and must follow refname rules. 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. Reported-by: Konstantin Kharlamov <hi-angel@yandex.ru> Helped-by: Jeff King <peff@peff.net> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- builtin/worktree.c | 10 +++- refs.c | 103 ++++++++++++++++++++++++++++++++-------- refs.h | 6 +++ t/t2400-worktree-add.sh | 5 ++ 4 files changed, 104 insertions(+), 20 deletions(-)