# Cannot delete remote branch with non-ascii name and --force-with-lease 4 messages from 2020-07-15 to 2020-07-21. Participants: Frej Bjon, brian m. carlson, Junio C Hamano. Thread: https://gitlist.dev/t/53862 ## Frej Bjon, 2020-07-15 09:10 Subject: Cannot delete remote branch with non-ascii name and --force-with-lease Message-ID: URL: https://gitlist.dev/e/CAP8P1T%2BJQUTVDXsJbrWkiZ2AWZmoptFSRdKssANYrNSFa%2BEgmg%40mail.gmail.com ``` What did you do before the bug happened? (Steps to reproduce your issue) - Create a branch with non-ascii character: testbranch-ä - Push to a remote with same name - Delete branch from remote with --force-with-lease: git --force-with-lease --delete origin "testbranch-ä" What did you expect to happen? (Expected behavior) - The branch to be deleted. What happened instead? (Actual behavior) - Git fails with an error error: cannot parse expected object name '32e33fb544d7b1147c446c60220f23a5df465eff"' Anything else you want to add: - The command works if using only --force and not --force-with-lease - The command works if there's no non-ascii character, e.g. "testbranch-x" - Tested with gitlab and github as remotes, with the same error - My locale in git bash is "en_US.UTF-8" - The same happens under Ubuntu 20.04 with git 2.25.1 [System Info] git version: git version 2.27.0.windows.1 cpu: x86_64 built from commit: 907ab1011dce9112700498e034b974ba60f8b407 sizeof-long: 4 sizeof-size_t: 8 uname: Windows 10.0 15063 compiler info: gnuc: 10.1 libc info: no libc information available [Enabled Hooks] None Regards, Frej Bjon ``` ## brian m. carlson, 2020-07-20 22:43 Subject: [PATCH] remote-curl: make --force-with-lease work with non-ASCII ref names Message-ID: <20200720224327.1631947-1-sandals@crustytoothpaste.net> URL: https://gitlist.dev/e/20200720224327.1631947-1-sandals%40crustytoothpaste.net In-Reply-To: ``` When we invoke a remote transport helper and pass an option with an argument, we quote the argument as a C-style string if necessary. This is the case for the cas option, which implements the --force-with-lease command-line flag, when we're passing a non-ASCII refname. However, the remote curl helper isn't designed to parse such an argument, meaning that if we try to use --force-with-lease with an HTTP push and a non-ASCII refname, we get an error like this: error: cannot parse expected object name '0000000000000000000000000000000000000000"' Note the double quote, which get_oid has reminded us is not valid in an hex object ID. Even if we had been able to parse it, we would send the wrong data to the server: we'd send an escaped ref, which would not behave as the user wanted and might accidentally result in updating or deleting a ref we hadn't intended. Since we need to expect a quoted C-style string here, just check if the first argument is a double quote, and if so, unquote it. Note that if the refname contains a double quote, then we will have double-quoted it already, so there is no ambiguity. We test for this case only in the smart protocol, since the DAV-based protocol is not capable of handling this capability. We use UTF-8 because this is nicer in our tests and friendlier to Windows, but the code should work for all non-ASCII refs. Reported-by: Frej Bjon Signed-off-by: brian m. carlson --- remote-curl.c | 8 +++++++- t/t5541-http-push-smart.sh | 15 +++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/remote-curl.c b/remote-curl.c index 5cbc6e5002..ccf0c27daf 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -121,7 +121,13 @@ static int set_option(const char *name, const char *value) } else if (!strcmp(name, "cas")) { struct strbuf val = STRBUF_INIT; - strbuf_addf(&val, "--" CAS_OPT_NAME "=%s", value); + strbuf_addstr(&val, "--" CAS_OPT_NAME "="); + if (*value == '"') { + if (unquote_c_style(&val, value, NULL)) + return -1; + } else { + strbuf_addstr(&val, value); + } string_list_append(&cas_options, val.buf); strbuf_release(&val); return 0; diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh index 463d0f12e5..187454f5dd 100755 --- a/t/t5541-http-push-smart.sh +++ b/t/t5541-http-push-smart.sh @@ -479,6 +479,21 @@ test_expect_success 'clone/fetch scrubs password from reflogs' ' ! grep "$HTTPD_URL_USER_PASS" reflog ' +test_expect_success 'Non-ASCII branch name can be used with --force-with-lease' ' + cd "$ROOT_PATH" && + git clone "$HTTPD_URL_USER_PASS/smart/test_repo.git" non-ascii && + cd non-ascii && + git checkout -b rama-de-árbol && + test_commit F && + git push --force-with-lease origin rama-de-árbol && + git ls-remote origin refs/heads/rama-de-árbol >actual && + git ls-remote . refs/heads/rama-de-árbol >expect && + test_cmp expect actual && + git push --delete --force-with-lease origin rama-de-árbol && + git ls-remote origin refs/heads/rama-de-árbol >actual && + test_must_be_empty actual +' + test_expect_success 'colorize errors/hints' ' cd "$ROOT_PATH"/test_repo_clone && test_must_fail git -c color.transport=always -c color.advice=always \ ``` ## Junio C Hamano, 2020-07-21 00:02 Subject: Re: [PATCH] remote-curl: make --force-with-lease work with non-ASCII ref names Message-ID: URL: https://gitlist.dev/e/xmqqlfjd91ll.fsf%40gitster.c.googlers.com In-Reply-To: <20200720224327.1631947-1-sandals@crustytoothpaste.net> ``` "brian m. carlson" writes: > diff --git a/remote-curl.c b/remote-curl.c > index 5cbc6e5002..ccf0c27daf 100644 > --- a/remote-curl.c > +++ b/remote-curl.c > @@ -121,7 +121,13 @@ static int set_option(const char *name, const char *value) > } > else if (!strcmp(name, "cas")) { > struct strbuf val = STRBUF_INIT; > - strbuf_addf(&val, "--" CAS_OPT_NAME "=%s", value); > + strbuf_addstr(&val, "--" CAS_OPT_NAME "="); > + if (*value == '"') { > + if (unquote_c_style(&val, value, NULL)) > + return -1; > + } else { > + strbuf_addstr(&val, value); > + } I wonder if if (*value != '"') strbuf_addstr(&val, value); else if (unquote_c_style(&val, value, NULL)) return -1; /* error */ is easier to read without having to use {braces}, but that's quite a minor point. A clean-up opportunity I can see here is to declare that we have found a good value for CAS_OPT_NAME and inline the string and remove the C preprocessor macro, but that is obviously an unrelated change to this fix. Thanks. ``` ## brian m. carlson, 2020-07-21 01:15 Subject: [PATCH v2] remote-curl: make --force-with-lease work with non-ASCII ref names Message-ID: <20200721011511.1649048-1-sandals@crustytoothpaste.net> URL: https://gitlist.dev/e/20200721011511.1649048-1-sandals%40crustytoothpaste.net In-Reply-To: ``` When we invoke a remote transport helper and pass an option with an argument, we quote the argument as a C-style string if necessary. This is the case for the cas option, which implements the --force-with-lease command-line flag, when we're passing a non-ASCII refname. However, the remote curl helper isn't designed to parse such an argument, meaning that if we try to use --force-with-lease with an HTTP push and a non-ASCII refname, we get an error like this: error: cannot parse expected object name '0000000000000000000000000000000000000000"' Note the double quote, which get_oid has reminded us is not valid in an hex object ID. Even if we had been able to parse it, we would send the wrong data to the server: we'd send an escaped ref, which would not behave as the user wanted and might accidentally result in updating or deleting a ref we hadn't intended. Since we need to expect a quoted C-style string here, just check if the first argument is a double quote, and if so, unquote it. Note that if the refname contains a double quote, then we will have double-quoted it already, so there is no ambiguity. We test for this case only in the smart protocol, since the DAV-based protocol is not capable of handling this capability. We use UTF-8 because this is nicer in our tests and friendlier to Windows, but the code should work for all non-ASCII refs. While we're at it, since the name of the option is now well established and isn't going to change, let's inline it instead of using the #define constant. Reported-by: Frej Bjon Signed-off-by: brian m. carlson --- remote-curl.c | 6 +++++- t/t5541-http-push-smart.sh | 15 +++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/remote-curl.c b/remote-curl.c index 5cbc6e5002..c9921c552c 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -121,7 +121,11 @@ static int set_option(const char *name, const char *value) } else if (!strcmp(name, "cas")) { struct strbuf val = STRBUF_INIT; - strbuf_addf(&val, "--" CAS_OPT_NAME "=%s", value); + strbuf_addstr(&val, "--force-with-lease="); + if (*value != '"') + strbuf_addstr(&val, value); + else if (unquote_c_style(&val, value, NULL)) + return -1; string_list_append(&cas_options, val.buf); strbuf_release(&val); return 0; diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh index 463d0f12e5..187454f5dd 100755 --- a/t/t5541-http-push-smart.sh +++ b/t/t5541-http-push-smart.sh @@ -479,6 +479,21 @@ test_expect_success 'clone/fetch scrubs password from reflogs' ' ! grep "$HTTPD_URL_USER_PASS" reflog ' +test_expect_success 'Non-ASCII branch name can be used with --force-with-lease' ' + cd "$ROOT_PATH" && + git clone "$HTTPD_URL_USER_PASS/smart/test_repo.git" non-ascii && + cd non-ascii && + git checkout -b rama-de-árbol && + test_commit F && + git push --force-with-lease origin rama-de-árbol && + git ls-remote origin refs/heads/rama-de-árbol >actual && + git ls-remote . refs/heads/rama-de-árbol >expect && + test_cmp expect actual && + git push --delete --force-with-lease origin rama-de-árbol && + git ls-remote origin refs/heads/rama-de-árbol >actual && + test_must_be_empty actual +' + test_expect_success 'colorize errors/hints' ' cd "$ROOT_PATH"/test_repo_clone && test_must_fail git -c color.transport=always -c color.advice=always \ ```