{"thread":{"id":"35594","subject":"[PATCH] drop unnecessary copying in credential_ask_one","startedAt":"2014-01-02T01:06:33Z","lastAt":"2014-01-07T20:02:22Z","messageCount":7,"participants":["Tay Ray Chuan","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"232552","messageId":"1388624793-5563-1-git-send-email-rctay89@gmail.com","threadId":"35594","inReplyTo":null,"subject":"[PATCH] drop unnecessary copying in credential_ask_one","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2014-01-02T01:06:33Z","receivedAt":"2014-01-02T01:06:33Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"We were leaking memory in there, as after obtaining a string from\ngit_getpass, we returned a copy of it, yet no one else held the original\nstring, apart from credential_ask_one.\n\nSigned-off-by: Tay Ray Chuan <rctay89@gmail.com>\n---\n credential.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/credential.c b/credential.c\nindex 86397f3..0d02ad8 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -54,7 +54,7 @@ static char *credential_ask_one(const char *what, struct credential *c)\n \n \tstrbuf_release(&desc);\n \tstrbuf_release(&prompt);\n-\treturn xstrdup(r);\n+\treturn r;\n }\n \n static void credential_getpass(struct credential *c)\n-- \n1.8.5-rc2\n"},{"id":"232553","messageId":"20140102030330.GA10976@sigill.intra.peff.net","threadId":"35594","inReplyTo":"1388624793-5563-1-git-send-email-rctay89@gmail.com","subject":"Re: [PATCH] drop unnecessary copying in credential_ask_one","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-02T03:03:30Z","receivedAt":"2014-01-02T03:03:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 02, 2014 at 09:06:33AM +0800, Tay Ray Chuan wrote:\n\n> We were leaking memory in there, as after obtaining a string from\n> git_getpass, we returned a copy of it, yet no one else held the original\n> string, apart from credential_ask_one.\n\nI don't think this change is correct by itself.\n\ncredential_ask_one calls git_prompt. That function in turn calls\ngit_terminal_prompt, which returns a pointer to a static buffer (because\nit may be backed by the system getpass() implementation).\n\nSo there is no leak there, and dropping the strdup would be bad (the\ncall to ask for the password would overwrite the value we got for the\nusername).\n\nHowever, git_prompt may also call do_askpass if GIT_ASKPASS is set, and\nhere there is a leak, as we duplicate the buffer.  To stop the leak, we\nneed to first harmonize the do_askpass and git_terminal_prompt code\npaths to either both allocate, or both return a static buffer (and then\neither strdup or not in the caller, depending on which way we go).\n\nIt looks like what I originally wrote was correct, as both code paths\nmatched.  But then I stupidly broke it with 31b49d9, which failed to\nnotice the \"static\" specifier on the strbuf in do_askpass, and started\nusing strbuf_detach.\n\nI think this is the simplest fix:\n\n-- >8 --\nSubject: Revert \"prompt: clean up strbuf usage\"\n\nThis reverts commit 31b49d9b653803e7c7fd18b21c8bdd86e3421668.\n\nThat commit taught do_askpass to hand ownership of our\nbuffer back to the caller rather than simply return a\npointer into our internal strbuf.  What it failed to notice,\nthough, was that our internal strbuf is static, because we\nare trying to emulate the getpass() interface.\n\nBy handing off ownership, we created a memory leak that\ncannot be solved. Sometimes git_prompt returns a static\nbuffer from getpass() (or our smarter git_terminal_prompt\nwrapper), and sometimes it returns an allocated string from\ndo_askpass.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n prompt.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/prompt.c b/prompt.c\nindex d851807..d7bb17c 100644\n--- a/prompt.c\n+++ b/prompt.c\n@@ -22,6 +22,7 @@ static char *do_askpass(const char *cmd, const char *prompt)\n \tif (start_command(&pass))\n \t\treturn NULL;\n \n+\tstrbuf_reset(&buffer);\n \tif (strbuf_read(&buffer, pass.out, 20) < 0)\n \t\terr = 1;\n \n@@ -38,7 +39,7 @@ static char *do_askpass(const char *cmd, const char *prompt)\n \n \tstrbuf_setlen(&buffer, strcspn(buffer.buf, \"\\r\\n\"));\n \n-\treturn strbuf_detach(&buffer, NULL);\n+\treturn buffer.buf;\n }\n \n char *git_prompt(const char *prompt, int flags)\n-- \n1.8.5.2.434.g63b1477\n\n\n\n\n\n\n> \n> Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>\n> ---\n>  credential.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/credential.c b/credential.c\n> index 86397f3..0d02ad8 100644\n> --- a/credential.c\n> +++ b/credential.c\n> @@ -54,7 +54,7 @@ static char *credential_ask_one(const char *what, struct credential *c)\n>  \n>  \tstrbuf_release(&desc);\n>  \tstrbuf_release(&prompt);\n> -\treturn xstrdup(r);\n> +\treturn r;\n>  }\n>  \n>  static void credential_getpass(struct credential *c)\n> -- \n> 1.8.5-rc2\n> \n"},{"id":"232554","messageId":"20140102073835.GA5431@sigill.intra.peff.net","threadId":"35594","inReplyTo":"20140102030330.GA10976@sigill.intra.peff.net","subject":"Re: [PATCH] drop unnecessary copying in credential_ask_one","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-02T07:38:35Z","receivedAt":"2014-01-02T07:38:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 01, 2014 at 10:03:30PM -0500, Jeff King wrote:\n\n> On Thu, Jan 02, 2014 at 09:06:33AM +0800, Tay Ray Chuan wrote:\n> \n> > We were leaking memory in there, as after obtaining a string from\n> > git_getpass, we returned a copy of it, yet no one else held the original\n> > string, apart from credential_ask_one.\n> \n> I don't think this change is correct by itself.\n> \n> credential_ask_one calls git_prompt. That function in turn calls\n> git_terminal_prompt, which returns a pointer to a static buffer (because\n> it may be backed by the system getpass() implementation).\n> \n> So there is no leak there, and dropping the strdup would be bad (the\n> call to ask for the password would overwrite the value we got for the\n> username).\n\nBy the way, you can see the breakage from your patch pretty easily by\ntesting the terminal input. Disable any credential helper config you\nhave, and then run:\n\n  GIT_CURL_VERBOSE=1 \\\n  git ls-remote https://github.com/peff/ask-for-auth 2>&1 |\n  perl -lne '/Authorization: Basic (.*)/ and print $1' |\n  openssl base64 -d\n\nenter \"myuser\" and \"mypass\" respectively on the terminal. The result is\nthat we send \"mypass:mypass\" to the server. And then double-free the\nresult, which cases glibc to barf.\n\nI wondered why we did not see this breakage in test suite. My assumption\nwas that it was simply because our test user has the same username and\npassword. So I fixed that, but to my surprise we still did not detect\nthe problem. The issue is that your patch does the right thing when\nGIT_ASKPASS is in use, and breaks only when the user types into the\nterminal. But the test suite, of course, always uses askpass because it\ncannot rely on accessing a terminal (we'd have to do some magic with\nlib-terminal, I think).\n\nSo it doesn't detect the problem in your patch, but I wonder if it is\nworth applying the patch below anyway, as it makes the test suite\nslightly more robust.\n\n-- >8 --\nSubject: use distinct username/password for http auth tests\n\nThe httpd server we set up to test git's http client code\nknows about a single account, in which both the username and\npassword are \"user@host\" (the unusual use of the \"@\" here is\nto verify that we handle the character correctly when URL\nescaped).\n\nThis means that we may miss a certain class of errors in\nwhich the username and password are mixed up internally by\ngit. We can make our tests more robust by having distinct\nvalues for the username and password.\n\nIn addition to tweaking the server passwd file and the\nclient URL, we must teach the \"askpass\" harness to accept\nmultiple values. As a bonus, this makes the setup of some\ntests more obvious; when we are expecting git to ask\nonly about the password, we can seed the username askpass\nresponse with a bogus value.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/lib-httpd.sh        | 15 ++++++++++++---\n t/lib-httpd/passwd    |  2 +-\n t/t5540-http-push.sh  |  4 ++--\n t/t5541-http-push.sh  |  6 +++---\n t/t5550-http-fetch.sh | 10 +++++-----\n t/t5551-http-fetch.sh |  6 +++---\n 6 files changed, 26 insertions(+), 17 deletions(-)\n\ndiff --git a/t/lib-httpd.sh b/t/lib-httpd.sh\nindex c470784..bfdff2a 100644\n--- a/t/lib-httpd.sh\n+++ b/t/lib-httpd.sh\n@@ -129,7 +129,7 @@ prepare_httpd() {\n \tHTTPD_DEST=127.0.0.1:$LIB_HTTPD_PORT\n \tHTTPD_URL=$HTTPD_PROTO://$HTTPD_DEST\n \tHTTPD_URL_USER=$HTTPD_PROTO://user%40host@$HTTPD_DEST\n-\tHTTPD_URL_USER_PASS=$HTTPD_PROTO://user%40host:user%40host@$HTTPD_DEST\n+\tHTTPD_URL_USER_PASS=$HTTPD_PROTO://user%40host:pass%40host@$HTTPD_DEST\n \n \tif test -n \"$LIB_HTTPD_DAV\" -o -n \"$LIB_HTTPD_SVN\"\n \tthen\n@@ -217,7 +217,15 @@ setup_askpass_helper() {\n \ttest_expect_success 'setup askpass helper' '\n \t\twrite_script \"$TRASH_DIRECTORY/askpass\" <<-\\EOF &&\n \t\techo >>\"$TRASH_DIRECTORY/askpass-query\" \"askpass: $*\" &&\n-\t\tcat \"$TRASH_DIRECTORY/askpass-response\"\n+\t\tcase \"$*\" in\n+\t\t*Username*)\n+\t\t\twhat=user\n+\t\t\t;;\n+\t\t*Password*)\n+\t\t\twhat=pass\n+\t\t\t;;\n+\t\tesac &&\n+\t\tcat \"$TRASH_DIRECTORY/askpass-$what\"\n \t\tEOF\n \t\tGIT_ASKPASS=\"$TRASH_DIRECTORY/askpass\" &&\n \t\texport GIT_ASKPASS &&\n@@ -227,7 +235,8 @@ setup_askpass_helper() {\n \n set_askpass() {\n \t>\"$TRASH_DIRECTORY/askpass-query\" &&\n-\techo \"$*\" >\"$TRASH_DIRECTORY/askpass-response\"\n+\techo \"$1\" >\"$TRASH_DIRECTORY/askpass-user\" &&\n+\techo \"$2\" >\"$TRASH_DIRECTORY/askpass-pass\"\n }\n \n expect_askpass() {\ndiff --git a/t/lib-httpd/passwd b/t/lib-httpd/passwd\nindex f2fbcad..99a34d6 100644\n--- a/t/lib-httpd/passwd\n+++ b/t/lib-httpd/passwd\n@@ -1 +1 @@\n-user@host:nKpa8pZUHx/ic\n+user@host:xb4E8pqD81KQs\ndiff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh\nindex 01d0d95..5b0198c 100755\n--- a/t/t5540-http-push.sh\n+++ b/t/t5540-http-push.sh\n@@ -154,7 +154,7 @@ test_http_push_nonff \"$HTTPD_DOCUMENT_ROOT_PATH\"/test_repo.git \\\n \n test_expect_success 'push to password-protected repository (user in URL)' '\n \ttest_commit pw-user &&\n-\tset_askpass user@host &&\n+\tset_askpass user@host pass@host &&\n \tgit push \"$HTTPD_URL_USER/auth/dumb/test_repo.git\" HEAD &&\n \tgit rev-parse --verify HEAD >expect &&\n \tgit --git-dir=\"$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/test_repo.git\" \\\n@@ -168,7 +168,7 @@ test_expect_failure 'user was prompted only once for password' '\n \n test_expect_failure 'push to password-protected repository (no user in URL)' '\n \ttest_commit pw-nouser &&\n-\tset_askpass user@host &&\n+\tset_askpass user@host pass@host &&\n \tgit push \"$HTTPD_URL/auth/dumb/test_repo.git\" HEAD &&\n \texpect_askpass both user@host\n \tgit rev-parse --verify HEAD >expect &&\ndiff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh\nindex 470ac54..bfd241e 100755\n--- a/t/t5541-http-push.sh\n+++ b/t/t5541-http-push.sh\n@@ -274,7 +274,7 @@ test_expect_success 'push over smart http with auth' '\n \tcd \"$ROOT_PATH/test_repo_clone\" &&\n \techo push-auth-test >expect &&\n \ttest_commit push-auth-test &&\n-\tset_askpass user@host &&\n+\tset_askpass user@host pass@host &&\n \tgit push \"$HTTPD_URL\"/auth/smart/test_repo.git &&\n \tgit --git-dir=\"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git\" \\\n \t\tlog -1 --format=%s >actual &&\n@@ -286,7 +286,7 @@ test_expect_success 'push to auth-only-for-push repo' '\n \tcd \"$ROOT_PATH/test_repo_clone\" &&\n \techo push-half-auth >expect &&\n \ttest_commit push-half-auth &&\n-\tset_askpass user@host &&\n+\tset_askpass user@host pass@host &&\n \tgit push \"$HTTPD_URL\"/auth-push/smart/test_repo.git &&\n \tgit --git-dir=\"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git\" \\\n \t\tlog -1 --format=%s >actual &&\n@@ -316,7 +316,7 @@ test_expect_success 'push into half-auth-complete requires password' '\n \tcd \"$ROOT_PATH/half-auth-clone\" &&\n \techo two >expect &&\n \ttest_commit two &&\n-\tset_askpass user@host &&\n+\tset_askpass user@host pass@host &&\n \tgit push \"$HTTPD_URL/half-auth-complete/smart/half-auth.git\" &&\n \tgit --git-dir=\"$HTTPD_DOCUMENT_ROOT_PATH/half-auth.git\" \\\n \t\tlog -1 --format=%s >actual &&\ndiff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh\nindex f7d0f14..8392624 100755\n--- a/t/t5550-http-fetch.sh\n+++ b/t/t5550-http-fetch.sh\n@@ -62,13 +62,13 @@ test_expect_success 'http auth can use user/pass in URL' '\n '\n \n test_expect_success 'http auth can use just user in URL' '\n-\tset_askpass user@host &&\n+\tset_askpass wrong pass@host &&\n \tgit clone \"$HTTPD_URL_USER/auth/dumb/repo.git\" clone-auth-pass &&\n \texpect_askpass pass user@host\n '\n \n test_expect_success 'http auth can request both user and pass' '\n-\tset_askpass user@host &&\n+\tset_askpass user@host pass@host &&\n \tgit clone \"$HTTPD_URL/auth/dumb/repo.git\" clone-auth-both &&\n \texpect_askpass both user@host\n '\n@@ -77,7 +77,7 @@ test_expect_success 'http auth respects credential helper config' '\n \ttest_config_global credential.helper \"!f() {\n \t\tcat >/dev/null\n \t\techo username=user@host\n-\t\techo password=user@host\n+\t\techo password=pass@host\n \t}; f\" &&\n \tset_askpass wrong &&\n \tgit clone \"$HTTPD_URL/auth/dumb/repo.git\" clone-auth-helper &&\n@@ -86,14 +86,14 @@ test_expect_success 'http auth respects credential helper config' '\n \n test_expect_success 'http auth can get username from config' '\n \ttest_config_global \"credential.$HTTPD_URL.username\" user@host &&\n-\tset_askpass user@host &&\n+\tset_askpass wrong pass@host &&\n \tgit clone \"$HTTPD_URL/auth/dumb/repo.git\" clone-auth-user &&\n \texpect_askpass pass user@host\n '\n \n test_expect_success 'configured username does not override URL' '\n \ttest_config_global \"credential.$HTTPD_URL.username\" wrong &&\n-\tset_askpass user@host &&\n+\tset_askpass wrong pass@host &&\n \tgit clone \"$HTTPD_URL_USER/auth/dumb/repo.git\" clone-auth-user2 &&\n \texpect_askpass pass user@host\n '\ndiff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh\nindex afb439e..a124efe 100755\n--- a/t/t5551-http-fetch.sh\n+++ b/t/t5551-http-fetch.sh\n@@ -119,7 +119,7 @@ test_expect_success 'redirects re-root further requests' '\n \n test_expect_success 'clone from password-protected repository' '\n \techo two >expect &&\n-\tset_askpass user@host &&\n+\tset_askpass user@host pass@host &&\n \tgit clone --bare \"$HTTPD_URL/auth/smart/repo.git\" smart-auth &&\n \texpect_askpass both user@host &&\n \tgit --git-dir=smart-auth log -1 --format=%s >actual &&\n@@ -137,7 +137,7 @@ test_expect_success 'clone from auth-only-for-push repository' '\n \n test_expect_success 'clone from auth-only-for-objects repository' '\n \techo two >expect &&\n-\tset_askpass user@host &&\n+\tset_askpass user@host pass@host &&\n \tgit clone --bare \"$HTTPD_URL/auth-fetch/smart/repo.git\" half-auth &&\n \texpect_askpass both user@host &&\n \tgit --git-dir=half-auth log -1 --format=%s >actual &&\n@@ -151,7 +151,7 @@ test_expect_success 'no-op half-auth fetch does not require a password' '\n '\n \n test_expect_success 'redirects send auth to new location' '\n-\tset_askpass user@host &&\n+\tset_askpass user@host pass@host &&\n \tgit -c credential.useHttpPath=true \\\n \t  clone $HTTPD_URL/smart-redir-auth/repo.git repo-redir-auth &&\n \texpect_askpass both user@host auth/smart/repo.git\n-- \n1.8.5.2.437.g500496c\n"},{"id":"232569","messageId":"xmqq7gaiqjzw.fsf@gitster.dls.corp.google.com","threadId":"35594","inReplyTo":"20140102073835.GA5431@sigill.intra.peff.net","subject":"Re: [PATCH] drop unnecessary copying in credential_ask_one","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-02T19:08:51Z","receivedAt":"2014-01-02T19:08:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> ... But the test suite, of course, always uses askpass because it\n> cannot rely on accessing a terminal (we'd have to do some magic with\n> lib-terminal, I think).\n>\n> So it doesn't detect the problem in your patch, but I wonder if it is\n> worth applying the patch below anyway, as it makes the test suite\n> slightly more robust.\n\nSounds like a good first step in the right direction.  Thanks.\n\n\n> -- >8 --\n> Subject: use distinct username/password for http auth tests\n>\n> The httpd server we set up to test git's http client code\n> knows about a single account, in which both the username and\n> password are \"user@host\" (the unusual use of the \"@\" here is\n> to verify that we handle the character correctly when URL\n> escaped).\n>\n> This means that we may miss a certain class of errors in\n> which the username and password are mixed up internally by\n> git. We can make our tests more robust by having distinct\n> values for the username and password.\n>\n> In addition to tweaking the server passwd file and the\n> client URL, we must teach the \"askpass\" harness to accept\n> multiple values. As a bonus, this makes the setup of some\n> tests more obvious; when we are expecting git to ask\n> only about the password, we can seed the username askpass\n> response with a bogus value.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  t/lib-httpd.sh        | 15 ++++++++++++---\n>  t/lib-httpd/passwd    |  2 +-\n>  t/t5540-http-push.sh  |  4 ++--\n>  t/t5541-http-push.sh  |  6 +++---\n>  t/t5550-http-fetch.sh | 10 +++++-----\n>  t/t5551-http-fetch.sh |  6 +++---\n>  6 files changed, 26 insertions(+), 17 deletions(-)\n>\n> diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh\n> index c470784..bfdff2a 100644\n> --- a/t/lib-httpd.sh\n> +++ b/t/lib-httpd.sh\n> @@ -129,7 +129,7 @@ prepare_httpd() {\n>  \tHTTPD_DEST=127.0.0.1:$LIB_HTTPD_PORT\n>  \tHTTPD_URL=$HTTPD_PROTO://$HTTPD_DEST\n>  \tHTTPD_URL_USER=$HTTPD_PROTO://user%40host@$HTTPD_DEST\n> -\tHTTPD_URL_USER_PASS=$HTTPD_PROTO://user%40host:user%40host@$HTTPD_DEST\n> +\tHTTPD_URL_USER_PASS=$HTTPD_PROTO://user%40host:pass%40host@$HTTPD_DEST\n>  \n>  \tif test -n \"$LIB_HTTPD_DAV\" -o -n \"$LIB_HTTPD_SVN\"\n>  \tthen\n> @@ -217,7 +217,15 @@ setup_askpass_helper() {\n>  \ttest_expect_success 'setup askpass helper' '\n>  \t\twrite_script \"$TRASH_DIRECTORY/askpass\" <<-\\EOF &&\n>  \t\techo >>\"$TRASH_DIRECTORY/askpass-query\" \"askpass: $*\" &&\n> -\t\tcat \"$TRASH_DIRECTORY/askpass-response\"\n> +\t\tcase \"$*\" in\n> +\t\t*Username*)\n> +\t\t\twhat=user\n> +\t\t\t;;\n> +\t\t*Password*)\n> +\t\t\twhat=pass\n> +\t\t\t;;\n> +\t\tesac &&\n> +\t\tcat \"$TRASH_DIRECTORY/askpass-$what\"\n>  \t\tEOF\n>  \t\tGIT_ASKPASS=\"$TRASH_DIRECTORY/askpass\" &&\n>  \t\texport GIT_ASKPASS &&\n> @@ -227,7 +235,8 @@ setup_askpass_helper() {\n>  \n>  set_askpass() {\n>  \t>\"$TRASH_DIRECTORY/askpass-query\" &&\n> -\techo \"$*\" >\"$TRASH_DIRECTORY/askpass-response\"\n> +\techo \"$1\" >\"$TRASH_DIRECTORY/askpass-user\" &&\n> +\techo \"$2\" >\"$TRASH_DIRECTORY/askpass-pass\"\n>  }\n>  \n>  expect_askpass() {\n> diff --git a/t/lib-httpd/passwd b/t/lib-httpd/passwd\n> index f2fbcad..99a34d6 100644\n> --- a/t/lib-httpd/passwd\n> +++ b/t/lib-httpd/passwd\n> @@ -1 +1 @@\n> -user@host:nKpa8pZUHx/ic\n> +user@host:xb4E8pqD81KQs\n> diff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh\n> index 01d0d95..5b0198c 100755\n> --- a/t/t5540-http-push.sh\n> +++ b/t/t5540-http-push.sh\n> @@ -154,7 +154,7 @@ test_http_push_nonff \"$HTTPD_DOCUMENT_ROOT_PATH\"/test_repo.git \\\n>  \n>  test_expect_success 'push to password-protected repository (user in URL)' '\n>  \ttest_commit pw-user &&\n> -\tset_askpass user@host &&\n> +\tset_askpass user@host pass@host &&\n>  \tgit push \"$HTTPD_URL_USER/auth/dumb/test_repo.git\" HEAD &&\n>  \tgit rev-parse --verify HEAD >expect &&\n>  \tgit --git-dir=\"$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/test_repo.git\" \\\n> @@ -168,7 +168,7 @@ test_expect_failure 'user was prompted only once for password' '\n>  \n>  test_expect_failure 'push to password-protected repository (no user in URL)' '\n>  \ttest_commit pw-nouser &&\n> -\tset_askpass user@host &&\n> +\tset_askpass user@host pass@host &&\n>  \tgit push \"$HTTPD_URL/auth/dumb/test_repo.git\" HEAD &&\n>  \texpect_askpass both user@host\n>  \tgit rev-parse --verify HEAD >expect &&\n> diff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh\n> index 470ac54..bfd241e 100755\n> --- a/t/t5541-http-push.sh\n> +++ b/t/t5541-http-push.sh\n> @@ -274,7 +274,7 @@ test_expect_success 'push over smart http with auth' '\n>  \tcd \"$ROOT_PATH/test_repo_clone\" &&\n>  \techo push-auth-test >expect &&\n>  \ttest_commit push-auth-test &&\n> -\tset_askpass user@host &&\n> +\tset_askpass user@host pass@host &&\n>  \tgit push \"$HTTPD_URL\"/auth/smart/test_repo.git &&\n>  \tgit --git-dir=\"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git\" \\\n>  \t\tlog -1 --format=%s >actual &&\n> @@ -286,7 +286,7 @@ test_expect_success 'push to auth-only-for-push repo' '\n>  \tcd \"$ROOT_PATH/test_repo_clone\" &&\n>  \techo push-half-auth >expect &&\n>  \ttest_commit push-half-auth &&\n> -\tset_askpass user@host &&\n> +\tset_askpass user@host pass@host &&\n>  \tgit push \"$HTTPD_URL\"/auth-push/smart/test_repo.git &&\n>  \tgit --git-dir=\"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git\" \\\n>  \t\tlog -1 --format=%s >actual &&\n> @@ -316,7 +316,7 @@ test_expect_success 'push into half-auth-complete requires password' '\n>  \tcd \"$ROOT_PATH/half-auth-clone\" &&\n>  \techo two >expect &&\n>  \ttest_commit two &&\n> -\tset_askpass user@host &&\n> +\tset_askpass user@host pass@host &&\n>  \tgit push \"$HTTPD_URL/half-auth-complete/smart/half-auth.git\" &&\n>  \tgit --git-dir=\"$HTTPD_DOCUMENT_ROOT_PATH/half-auth.git\" \\\n>  \t\tlog -1 --format=%s >actual &&\n> diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh\n> index f7d0f14..8392624 100755\n> --- a/t/t5550-http-fetch.sh\n> +++ b/t/t5550-http-fetch.sh\n> @@ -62,13 +62,13 @@ test_expect_success 'http auth can use user/pass in URL' '\n>  '\n>  \n>  test_expect_success 'http auth can use just user in URL' '\n> -\tset_askpass user@host &&\n> +\tset_askpass wrong pass@host &&\n>  \tgit clone \"$HTTPD_URL_USER/auth/dumb/repo.git\" clone-auth-pass &&\n>  \texpect_askpass pass user@host\n>  '\n>  \n>  test_expect_success 'http auth can request both user and pass' '\n> -\tset_askpass user@host &&\n> +\tset_askpass user@host pass@host &&\n>  \tgit clone \"$HTTPD_URL/auth/dumb/repo.git\" clone-auth-both &&\n>  \texpect_askpass both user@host\n>  '\n> @@ -77,7 +77,7 @@ test_expect_success 'http auth respects credential helper config' '\n>  \ttest_config_global credential.helper \"!f() {\n>  \t\tcat >/dev/null\n>  \t\techo username=user@host\n> -\t\techo password=user@host\n> +\t\techo password=pass@host\n>  \t}; f\" &&\n>  \tset_askpass wrong &&\n>  \tgit clone \"$HTTPD_URL/auth/dumb/repo.git\" clone-auth-helper &&\n> @@ -86,14 +86,14 @@ test_expect_success 'http auth respects credential helper config' '\n>  \n>  test_expect_success 'http auth can get username from config' '\n>  \ttest_config_global \"credential.$HTTPD_URL.username\" user@host &&\n> -\tset_askpass user@host &&\n> +\tset_askpass wrong pass@host &&\n>  \tgit clone \"$HTTPD_URL/auth/dumb/repo.git\" clone-auth-user &&\n>  \texpect_askpass pass user@host\n>  '\n>  \n>  test_expect_success 'configured username does not override URL' '\n>  \ttest_config_global \"credential.$HTTPD_URL.username\" wrong &&\n> -\tset_askpass user@host &&\n> +\tset_askpass wrong pass@host &&\n>  \tgit clone \"$HTTPD_URL_USER/auth/dumb/repo.git\" clone-auth-user2 &&\n>  \texpect_askpass pass user@host\n>  '\n> diff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh\n> index afb439e..a124efe 100755\n> --- a/t/t5551-http-fetch.sh\n> +++ b/t/t5551-http-fetch.sh\n> @@ -119,7 +119,7 @@ test_expect_success 'redirects re-root further requests' '\n>  \n>  test_expect_success 'clone from password-protected repository' '\n>  \techo two >expect &&\n> -\tset_askpass user@host &&\n> +\tset_askpass user@host pass@host &&\n>  \tgit clone --bare \"$HTTPD_URL/auth/smart/repo.git\" smart-auth &&\n>  \texpect_askpass both user@host &&\n>  \tgit --git-dir=smart-auth log -1 --format=%s >actual &&\n> @@ -137,7 +137,7 @@ test_expect_success 'clone from auth-only-for-push repository' '\n>  \n>  test_expect_success 'clone from auth-only-for-objects repository' '\n>  \techo two >expect &&\n> -\tset_askpass user@host &&\n> +\tset_askpass user@host pass@host &&\n>  \tgit clone --bare \"$HTTPD_URL/auth-fetch/smart/repo.git\" half-auth &&\n>  \texpect_askpass both user@host &&\n>  \tgit --git-dir=half-auth log -1 --format=%s >actual &&\n> @@ -151,7 +151,7 @@ test_expect_success 'no-op half-auth fetch does not require a password' '\n>  '\n>  \n>  test_expect_success 'redirects send auth to new location' '\n> -\tset_askpass user@host &&\n> +\tset_askpass user@host pass@host &&\n>  \tgit -c credential.useHttpPath=true \\\n>  \t  clone $HTTPD_URL/smart-redir-auth/repo.git repo-redir-auth &&\n>  \texpect_askpass both user@host auth/smart/repo.git\n"},{"id":"232818","messageId":"20140107175009.GA19691@sigill.intra.peff.net","threadId":"35594","inReplyTo":"xmqq7gaiqjzw.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] drop unnecessary copying in credential_ask_one","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-07T17:50:09Z","receivedAt":"2014-01-07T17:50:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 02, 2014 at 11:08:51AM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > ... But the test suite, of course, always uses askpass because it\n> > cannot rely on accessing a terminal (we'd have to do some magic with\n> > lib-terminal, I think).\n> >\n> > So it doesn't detect the problem in your patch, but I wonder if it is\n> > worth applying the patch below anyway, as it makes the test suite\n> > slightly more robust.\n> \n> Sounds like a good first step in the right direction.  Thanks.\n\nI took a brief look at adding \"real\" terminal tests for the credential\ncode using our test-terminal/lib-terminal.sh setup. Unfortunately, it\nfalls short of what we need.\n\ntest-terminal only handles stdout and stderr streams as fake terminals.\nWe could pretty easily add stdin for input, as it uses fork() to work\nasynchronously.  But the credential code does not actually read from\nstdin. It opens and reads from /dev/tty explicitly. So I think we'd have\nto actually fake setting up a controlling terminal. And that means magic\nwith setsid() and ioctl(TIOCSCTTY), which in turn sounds like a\nportability headache.\n\nSo it's definitely possible under Linux, and probably under most Unixes.\nBut I'm not sure it's worth the effort, given that review already caught\nthe potential bug here.\n\nAnother option would be to instrument git_terminal_prompt with a\nmock-terminal interface (say, reading from a file specified in an\nenvironment variable). But I really hate polluting the code with test\ncruft, and it would not actually be testing an interesting segment of\nthe code, anyway.\n\n-Peff\n"},{"id":"232836","messageId":"xmqqlhyrd1bz.fsf@gitster.dls.corp.google.com","threadId":"35594","inReplyTo":"20140107175009.GA19691@sigill.intra.peff.net","subject":"Re: [PATCH] drop unnecessary copying in credential_ask_one","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-07T19:44:00Z","receivedAt":"2014-01-07T19:44:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Thu, Jan 02, 2014 at 11:08:51AM -0800, Junio C Hamano wrote:\n>\n>> Jeff King <peff@peff.net> writes:\n>> \n>> > ... But the test suite, of course, always uses askpass because it\n>> > cannot rely on accessing a terminal (we'd have to do some magic with\n>> > lib-terminal, I think).\n>> >\n>> > So it doesn't detect the problem in your patch, but I wonder if it is\n>> > worth applying the patch below anyway, as it makes the test suite\n>> > slightly more robust.\n>> \n>> Sounds like a good first step in the right direction.  Thanks.\n>\n> I took a brief look at adding \"real\" terminal tests for the credential\n> code using our test-terminal/lib-terminal.sh setup. Unfortunately, it\n> falls short of what we need.\n>\n> test-terminal only handles stdout and stderr streams as fake terminals.\n> We could pretty easily add stdin for input, as it uses fork() to work\n> asynchronously.  But the credential code does not actually read from\n> stdin. It opens and reads from /dev/tty explicitly. So I think we'd have\n> to actually fake setting up a controlling terminal. And that means magic\n> with setsid() and ioctl(TIOCSCTTY), which in turn sounds like a\n> portability headache.\n\nI wonder if \"expect\" has already solved that for us.\n\n> So it's definitely possible under Linux, and probably under most Unixes.\n> But I'm not sure it's worth the effort, given that review already caught\n> the potential bug here.\n>\n> Another option would be to instrument git_terminal_prompt with a\n> mock-terminal interface (say, reading from a file specified in an\n> environment variable). But I really hate polluting the code with test\n> cruft, and it would not actually be testing an interesting segment of\n> the code, anyway.\n\nAgreed.\n"},{"id":"232843","messageId":"20140107200221.GB21812@sigill.intra.peff.net","threadId":"35594","inReplyTo":"xmqqlhyrd1bz.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] drop unnecessary copying in credential_ask_one","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-07T20:02:22Z","receivedAt":"2014-01-07T20:02:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 07, 2014 at 11:44:00AM -0800, Junio C Hamano wrote:\n\n> > test-terminal only handles stdout and stderr streams as fake terminals.\n> > We could pretty easily add stdin for input, as it uses fork() to work\n> > asynchronously.  But the credential code does not actually read from\n> > stdin. It opens and reads from /dev/tty explicitly. So I think we'd have\n> > to actually fake setting up a controlling terminal. And that means magic\n> > with setsid() and ioctl(TIOCSCTTY), which in turn sounds like a\n> > portability headache.\n> \n> I wonder if \"expect\" has already solved that for us.\n\nI would not be surprised if it did. Though it introduces its own\nportability issues, since we cannot depend on having it. But it is\nprobably enough to just\n\n  test_lazy_prereq EXPECT 'expect --version'\n\nor something. I dunno. I have never used expect, do not have it\ninstalled, and am not excited about introducing a new tool dependency.\nBut if you want to explore it, be my guest.\n\n-Peff\n"}]}