{"thread":{"id":"59860","subject":"[PATCH 0/2] credential: improvements to erase in helpers","startedAt":"2023-06-14T11:23:12Z","lastAt":"2023-06-16T16:56:16Z","messageCount":26,"participants":["M Hickford via GitGitGadget","Junio C Hamano","M Hickford","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"478361","messageId":"pull.1525.git.git.1686741785.gitgitgadget@gmail.com","threadId":"59860","inReplyTo":null,"subject":"[PATCH 0/2] credential: improvements to erase in helpers","fromName":"M Hickford via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-06-14T11:23:03Z","receivedAt":"2023-06-14T11:23:12Z","isPatch":true,"sender":{"key":"mirth.hickford@gmail.com","avatar":"https://avatars.githubusercontent.com/u/105314?v=4"},"body":"cc. Jeff King peff@peff.net cc: Matthew John Cheetham mjcheetham@outlook.com\n\nM Hickford (2):\n credential: avoid erasing distinct password\n credential: erase all matching credentials\n\n Documentation/git-credential.txt | 4 +-\n Documentation/gitcredentials.txt | 2 +-\n builtin/credential-cache--daemon.c | 11 ++++--\n builtin/credential-store.c | 21 ++++++-----\n credential.c | 11 +++---\n credential.h | 2 +-\n t/lib-credential.sh | 59 ++++++++++++++++++++++++++++++\n 7 files changed, 88 insertions(+), 22 deletions(-)\n\n\nbase-commit: fe86abd7511a9a6862d5706c6fa1d9b57a63ba09\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1525%2Fhickford%2Ferase-test-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1525/hickford/erase-test-v1\nPull-Request: https://github.com/git/git/pull/1525\n-- \ngitgitgadget\n"},{"id":"478362","messageId":"35ee1795bcdb974cdb9ca0f55ddf8f8a5bc562ae.1686741785.git.gitgitgadget@gmail.com","threadId":"59860","inReplyTo":"pull.1525.git.git.1686741785.gitgitgadget@gmail.com","subject":"[PATCH 1/2] credential: avoid erasing distinct password","fromName":"M Hickford via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-06-14T11:23:04Z","receivedAt":"2023-06-14T11:23:14Z","isPatch":true,"sender":{"key":"mirth.hickford@gmail.com","avatar":"https://avatars.githubusercontent.com/u/105314?v=4"},"body":"From: M Hickford \n\nTest that credential helpers do not erase a password distinct from the\ninput. Such calls can happen when multiple credential helpers are\nconfigured.\n\nFixes for credential-cache and credential-store.\n\nSigned-off-by: M Hickford \n---\n builtin/credential-cache--daemon.c | 8 ++++----\n builtin/credential-store.c | 18 ++++++++++--------\n credential.c | 11 ++++++-----\n credential.h | 2 +-\n t/lib-credential.sh | 26 ++++++++++++++++++++++++++\n 5 files changed, 47 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c\nindex 756c5f02aef..82f376d3351 100644\n--- a/builtin/credential-cache--daemon.c\n+++ b/builtin/credential-cache--daemon.c\n@@ -33,12 +33,12 @@ static void cache_credential(struct credential *c, int timeout)\n \te->expiration = time(NULL) + timeout;\n }\n \n-static struct credential_cache_entry *lookup_credential(const struct credential *c)\n+static struct credential_cache_entry *lookup_credential(const struct credential *c, int match_password)\n {\n \tint i;\n \tfor (i = 0; i < entries_nr; i++) {\n \t\tstruct credential *e = &entries[i].item;\n-\t\tif (credential_match(c, e))\n+\t\tif (credential_match(c, e, match_password))\n \t\t\treturn &entries[i];\n \t}\n \treturn NULL;\n@@ -48,7 +48,7 @@ static void remove_credential(const struct credential *c)\n {\n \tstruct credential_cache_entry *e;\n \n-\te = lookup_credential(c);\n+\te = lookup_credential(c, c->password != NULL);\n \tif (e)\n \t\te->expiration = 0;\n }\n@@ -127,7 +127,7 @@ static void serve_one_client(FILE *in, FILE *out)\n \tif (read_request(in, &c, &action, &timeout) < 0)\n \t\t/* ignore error */ ;\n \telse if (!strcmp(action.buf, \"get\")) {\n-\t\tstruct credential_cache_entry *e = lookup_credential(&c);\n+\t\tstruct credential_cache_entry *e = lookup_credential(&c, 0);\n \t\tif (e) {\n \t\t\tfprintf(out, \"username=%s\\n\", e->item.username);\n \t\t\tfprintf(out, \"password=%s\\n\", e->item.password);\ndiff --git a/builtin/credential-store.c b/builtin/credential-store.c\nindex 30c6ccf56c0..e0ae028b1c3 100644\n--- a/builtin/credential-store.c\n+++ b/builtin/credential-store.c\n@@ -13,12 +13,14 @@ static struct lock_file credential_lock;\n static int parse_credential_file(const char *fn,\n \t\t\t\t struct credential *c,\n \t\t\t\t void (*match_cb)(struct credential *),\n-\t\t\t\t void (*other_cb)(struct strbuf *))\n+\t\t\t\t void (*other_cb)(struct strbuf *),\n+\t\t\t\t const char* op)\n {\n \tFILE *fh;\n \tstruct strbuf line = STRBUF_INIT;\n \tstruct credential entry = CREDENTIAL_INIT;\n \tint found_credential = 0;\n+\tconst int match_password = !strcmp(op, \"erase\") && c->password != NULL;\n \n \tfh = fopen(fn, \"r\");\n \tif (!fh) {\n@@ -29,8 +31,8 @@ static int parse_credential_file(const char *fn,\n \n \twhile (strbuf_getline_lf(&line, fh) != EOF) {\n \t\tif (!credential_from_url_gently(&entry, line.buf, 1) &&\n-\t\t entry.username && entry.password &&\n-\t\t credential_match(c, &entry)) {\n+\t\t\tentry.username && entry.password &&\n+\t\t\tcredential_match(c, &entry, match_password)) {\n \t\t\tfound_credential = 1;\n \t\t\tif (match_cb) {\n \t\t\t\tmatch_cb(&entry);\n@@ -60,7 +62,7 @@ static void print_line(struct strbuf *buf)\n }\n \n static void rewrite_credential_file(const char *fn, struct credential *c,\n-\t\t\t\t struct strbuf *extra)\n+\t\t\t\t struct strbuf *extra, const char *op)\n {\n \tint timeout_ms = 1000;\n \n@@ -69,7 +71,7 @@ static void rewrite_credential_file(const char *fn, struct credential *c,\n \t\tdie_errno(_(\"unable to get credential storage lock in %d ms\"), timeout_ms);\n \tif (extra)\n \t\tprint_line(extra);\n-\tparse_credential_file(fn, c, NULL, print_line);\n+\tparse_credential_file(fn, c, NULL, print_line, op);\n \tif (commit_lock_file(&credential_lock) < 0)\n \t\tdie_errno(\"unable to write credential store\");\n }\n@@ -91,7 +93,7 @@ static void store_credential_file(const char *fn, struct credential *c)\n \t\t\t\t\tis_rfc3986_reserved_or_unreserved);\n \t}\n \n-\trewrite_credential_file(fn, c, &buf);\n+\trewrite_credential_file(fn, c, &buf, \"store\");\n \tstrbuf_release(&buf);\n }\n \n@@ -138,7 +140,7 @@ static void remove_credential(const struct string_list *fns, struct credential *\n \t\treturn;\n \tfor_each_string_list_item(fn, fns)\n \t\tif (!access(fn->string, F_OK))\n-\t\t\trewrite_credential_file(fn->string, c, NULL);\n+\t\t\trewrite_credential_file(fn->string, c, NULL, \"erase\");\n }\n \n static void lookup_credential(const struct string_list *fns, struct credential *c)\n@@ -146,7 +148,7 @@ static void lookup_credential(const struct string_list *fns, struct credential *\n \tstruct string_list_item *fn;\n \n \tfor_each_string_list_item(fn, fns)\n-\t\tif (parse_credential_file(fn->string, c, print_entry, NULL))\n+\t\tif (parse_credential_file(fn->string, c, print_entry, NULL, \"get\"))\n \t\t\treturn; /* Found credential */\n }\n \ndiff --git a/credential.c b/credential.c\nindex 023b59d5711..9157ff0865e 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -33,13 +33,14 @@ void credential_clear(struct credential *c)\n }\n \n int credential_match(const struct credential *want,\n-\t\t const struct credential *have)\n+\t\t const struct credential *have, int match_password)\n {\n #define CHECK(x) (!want->x || (have->x && !strcmp(want->x, have->x)))\n \treturn CHECK(protocol) &&\n-\t CHECK(host) &&\n-\t CHECK(path) &&\n-\t CHECK(username);\n+\t\tCHECK(host) &&\n+\t\tCHECK(path) &&\n+\t\tCHECK(username) &&\n+\t\t(!match_password || CHECK(password));\n #undef CHECK\n }\n \n@@ -102,7 +103,7 @@ static int match_partial_url(const char *url, void *cb)\n \t\twarning(_(\"skipping credential lookup for key: credential.%s\"),\n \t\t\turl);\n \telse\n-\t\tmatches = credential_match(&want, c);\n+\t\tmatches = credential_match(&want, c, 0);\n \tcredential_clear(&want);\n \n \treturn matches;\ndiff --git a/credential.h b/credential.h\nindex b8e2936d1dc..acc41adf548 100644\n--- a/credential.h\n+++ b/credential.h\n@@ -211,6 +211,6 @@ void credential_from_url(struct credential *, const char *url);\n int credential_from_url_gently(struct credential *, const char *url, int quiet);\n \n int credential_match(const struct credential *want,\n-\t\t const struct credential *have);\n+\t\t const struct credential *have, int match_password);\n \n #endif /* CREDENTIAL_H */\ndiff --git a/t/lib-credential.sh b/t/lib-credential.sh\nindex f1ab92ba35c..f7e4e29c5e1 100644\n--- a/t/lib-credential.sh\n+++ b/t/lib-credential.sh\n@@ -44,6 +44,7 @@ helper_test_clean() {\n \treject $1 https example.com user1\n \treject $1 https example.com user2\n \treject $1 https example.com user4\n+\treject $1 https example.com user5\n \treject $1 http path.tld user\n \treject $1 https timeout.tld user\n \treject $1 https sso.tld\n@@ -221,6 +222,31 @@ helper_test() {\n \t\tEOF\n \t'\n \n+\ttest_expect_success \"helper ($HELPER) does not erase a password distinct from input\" '\n+\t\tcheck approve $HELPER <<-\\EOF &&\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\tusername=user5\n+\t\tpassword=pass1\n+\t\tEOF\n+\t\tcheck reject $HELPER <<-\\EOF &&\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\tusername=user5\n+\t\tpassword=pass2\n+\t\tEOF\n+\t\tcheck fill $HELPER <<-\\EOF\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\tusername=user5\n+\t\t--\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\tusername=user5\n+\t\tpassword=pass1\n+\t\tEOF\n+\t'\n+\n \ttest_expect_success \"helper ($HELPER) can forget user\" '\n \t\tcheck reject $HELPER <<-\\EOF &&\n \t\tprotocol=https\n-- \ngitgitgadget\n\n"},{"id":"478363","messageId":"fcdb579263f87dd089c50fc5799cf30b21f4d12c.1686741785.git.gitgitgadget@gmail.com","threadId":"59860","inReplyTo":"pull.1525.git.git.1686741785.gitgitgadget@gmail.com","subject":"[PATCH 2/2] credential: erase all matching credentials","fromName":"M Hickford via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-06-14T11:23:05Z","receivedAt":"2023-06-14T11:23:16Z","isPatch":true,"sender":{"key":"mirth.hickford@gmail.com","avatar":"https://avatars.githubusercontent.com/u/105314?v=4"},"body":"From: M Hickford \n\nUsers expect that\n`echo \"url=https://example.com\" | git credential reject` or\n`echo \"url=https://example.com\\nusername=tim\" | git credential reject`\nerase all matching credentials.\n\nFixes for credential-cache and credential-store.\n\nSigned-off-by: M Hickford \n---\n Documentation/git-credential.txt | 4 ++--\n Documentation/gitcredentials.txt | 2 +-\n builtin/credential-cache--daemon.c | 15 ++++++++------\n builtin/credential-store.c | 3 ++-\n t/lib-credential.sh | 33 ++++++++++++++++++++++++++++++\n 5 files changed, 47 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt\nindex 0e6d9e85ec7..04bfb918de6 100644\n--- a/Documentation/git-credential.txt\n+++ b/Documentation/git-credential.txt\n@@ -38,8 +38,8 @@ to any configured credential helpers, which may store the credential\n for later use.\n \n If the action is `reject`, git-credential will send the description to\n-any configured credential helpers, which may erase any stored\n-credential matching the description.\n+any configured credential helpers, which may erase stored credentials\n+matching the description.\n \n If the action is `approve` or `reject`, no output should be emitted.\n \ndiff --git a/Documentation/gitcredentials.txt b/Documentation/gitcredentials.txt\nindex 100f045bb1a..65d652dc40e 100644\n--- a/Documentation/gitcredentials.txt\n+++ b/Documentation/gitcredentials.txt\n@@ -260,7 +260,7 @@ appended to its command line, which is one of:\n \n `erase`::\n \n-\tRemove a matching credential, if any, from the helper's storage.\n+\tRemove matching credentials, if any, from the helper's storage.\n \n The details of the credential will be provided on the helper's stdin\n stream. The exact format is the same as the input/output format of the\ndiff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c\nindex 82f376d3351..5e3a766e42d 100644\n--- a/builtin/credential-cache--daemon.c\n+++ b/builtin/credential-cache--daemon.c\n@@ -33,12 +33,12 @@ static void cache_credential(struct credential *c, int timeout)\n \te->expiration = time(NULL) + timeout;\n }\n \n-static struct credential_cache_entry *lookup_credential(const struct credential *c, int match_password)\n+static struct credential_cache_entry *lookup_credential(const struct credential *c)\n {\n \tint i;\n \tfor (i = 0; i < entries_nr; i++) {\n \t\tstruct credential *e = &entries[i].item;\n-\t\tif (credential_match(c, e, match_password))\n+\t\tif (credential_match(c, e, 0))\n \t\t\treturn &entries[i];\n \t}\n \treturn NULL;\n@@ -48,9 +48,12 @@ static void remove_credential(const struct credential *c)\n {\n \tstruct credential_cache_entry *e;\n \n-\te = lookup_credential(c, c->password != NULL);\n-\tif (e)\n-\t\te->expiration = 0;\n+\tint i;\n+\tfor (i = 0; i < entries_nr; i++) {\n+\t\te = &entries[i];\n+\t\tif (credential_match(c, &e->item, c->password != NULL))\n+\t\t\te->expiration = 0;\n+\t}\n }\n \n static timestamp_t check_expirations(void)\n@@ -127,7 +130,7 @@ static void serve_one_client(FILE *in, FILE *out)\n \tif (read_request(in, &c, &action, &timeout) < 0)\n \t\t/* ignore error */ ;\n \telse if (!strcmp(action.buf, \"get\")) {\n-\t\tstruct credential_cache_entry *e = lookup_credential(&c, 0);\n+\t\tstruct credential_cache_entry *e = lookup_credential(&c);\n \t\tif (e) {\n \t\t\tfprintf(out, \"username=%s\\n\", e->item.username);\n \t\t\tfprintf(out, \"password=%s\\n\", e->item.password);\ndiff --git a/builtin/credential-store.c b/builtin/credential-store.c\nindex e0ae028b1c3..85b147e460f 100644\n--- a/builtin/credential-store.c\n+++ b/builtin/credential-store.c\n@@ -36,7 +36,8 @@ static int parse_credential_file(const char *fn,\n \t\t\tfound_credential = 1;\n \t\t\tif (match_cb) {\n \t\t\t\tmatch_cb(&entry);\n-\t\t\t\tbreak;\n+\t\t\t\tif (strcmp(op, \"erase\"))\n+\t\t\t\t\tbreak;\n \t\t\t}\n \t\t}\n \t\telse if (other_cb)\ndiff --git a/t/lib-credential.sh b/t/lib-credential.sh\nindex f7e4e29c5e1..3f4100b6ce2 100644\n--- a/t/lib-credential.sh\n+++ b/t/lib-credential.sh\n@@ -45,6 +45,8 @@ helper_test_clean() {\n \treject $1 https example.com user2\n \treject $1 https example.com user4\n \treject $1 https example.com user5\n+\treject $1 https example.com user6\n+\treject $1 https example.com user7\n \treject $1 http path.tld user\n \treject $1 https timeout.tld user\n \treject $1 https sso.tld\n@@ -298,6 +300,37 @@ helper_test() {\n \t\tEOF\n \t'\n \n+\ttest_expect_success \"helper ($HELPER) erases all matching credentials\" '\n+\t\tcheck approve $HELPER <<-\\EOF &&\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\tusername=user6\n+\t\tpassword=pass1\n+\t\tEOF\n+\t\tcheck approve $HELPER <<-\\EOF &&\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\tusername=user7\n+\t\tpassword=pass1\n+\t\tEOF\n+\t\tcheck reject $HELPER <<-\\EOF &&\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\tEOF\n+\t\tcheck fill $HELPER <<-\\EOF\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\t--\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\tusername=askpass-username\n+\t\tpassword=askpass-password\n+\t\t--\n+\t\taskpass: Username for '\\''https://example.com'\\'':\n+\t\taskpass: Password for '\\''https://askpass-username@example.com'\\'':\n+\t\tEOF\n+\t'\n+\n \t: ${GIT_TEST_LONG_CRED_BUFFER:=1024}\n \t# 23 bytes accounts for \"wwwauth[]=basic realm=\" plus NUL\n \tLONG_VALUE_LEN=$((GIT_TEST_LONG_CRED_BUFFER - 23))\n-- \ngitgitgadget\n"},{"id":"478375","messageId":"xmqqv8fqxcdj.fsf@gitster.g","threadId":"59860","inReplyTo":"fcdb579263f87dd089c50fc5799cf30b21f4d12c.1686741785.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] credential: erase all matching credentials","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-14T16:00:24Z","receivedAt":"2023-06-14T16:00:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"M Hickford via GitGitGadget\" writes:\n\n> From: M Hickford \n>\n> Users expect that\n> `echo \"url=https://example.com\" | git credential reject` or\n> `echo \"url=https://example.com\\nusername=tim\" | git credential reject`\n> erase all matching credentials.\n\nPerhaps that is a sensible expectation. It is unclear from the\nabove description what happens instead without the \"fix\", though.\n\nBy the way, I do not think your\n\n cc. Jeff King peff@peff.net cc: Matthew John Cheetham mjcheetham@outlook.com\n\nin 0/2 is doing anything; I've manually added them to Cc: of this\nmessage and left everything in the patch below, even though I am not\ncommenting on anything there myself, to give them easier reference.\n\nThanks.\n\n> Fixes for credential-cache and credential-store.\n>\n> Signed-off-by: M Hickford \n> ---\n> Documentation/git-credential.txt | 4 ++--\n> Documentation/gitcredentials.txt | 2 +-\n> builtin/credential-cache--daemon.c | 15 ++++++++------\n> builtin/credential-store.c | 3 ++-\n> t/lib-credential.sh | 33 ++++++++++++++++++++++++++++++\n> 5 files changed, 47 insertions(+), 10 deletions(-)\n>\n> diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt\n> index 0e6d9e85ec7..04bfb918de6 100644\n> --- a/Documentation/git-credential.txt\n> +++ b/Documentation/git-credential.txt\n> @@ -38,8 +38,8 @@ to any configured credential helpers, which may store the credential\n> for later use.\n> \n> If the action is `reject`, git-credential will send the description to\n> -any configured credential helpers, which may erase any stored\n> -credential matching the description.\n> +any configured credential helpers, which may erase stored credentials\n> +matching the description.\n> \n> If the action is `approve` or `reject`, no output should be emitted.\n> \n> diff --git a/Documentation/gitcredentials.txt b/Documentation/gitcredentials.txt\n> index 100f045bb1a..65d652dc40e 100644\n> --- a/Documentation/gitcredentials.txt\n> +++ b/Documentation/gitcredentials.txt\n> @@ -260,7 +260,7 @@ appended to its command line, which is one of:\n> \n> `erase`::\n> \n> -\tRemove a matching credential, if any, from the helper's storage.\n> +\tRemove matching credentials, if any, from the helper's storage.\n> \n> The details of the credential will be provided on the helper's stdin\n> stream. The exact format is the same as the input/output format of the\n> diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c\n> index 82f376d3351..5e3a766e42d 100644\n> --- a/builtin/credential-cache--daemon.c\n> +++ b/builtin/credential-cache--daemon.c\n> @@ -33,12 +33,12 @@ static void cache_credential(struct credential *c, int timeout)\n> \te->expiration = time(NULL) + timeout;\n> }\n> \n> -static struct credential_cache_entry *lookup_credential(const struct credential *c, int match_password)\n> +static struct credential_cache_entry *lookup_credential(const struct credential *c)\n> {\n> \tint i;\n> \tfor (i = 0; i < entries_nr; i++) {\n> \t\tstruct credential *e = &entries[i].item;\n> -\t\tif (credential_match(c, e, match_password))\n> +\t\tif (credential_match(c, e, 0))\n> \t\t\treturn &entries[i];\n> \t}\n> \treturn NULL;\n> @@ -48,9 +48,12 @@ static void remove_credential(const struct credential *c)\n> {\n> \tstruct credential_cache_entry *e;\n> \n> -\te = lookup_credential(c, c->password != NULL);\n> -\tif (e)\n> -\t\te->expiration = 0;\n> +\tint i;\n> +\tfor (i = 0; i < entries_nr; i++) {\n> +\t\te = &entries[i];\n> +\t\tif (credential_match(c, &e->item, c->password != NULL))\n> +\t\t\te->expiration = 0;\n> +\t}\n> }\n> \n> static timestamp_t check_expirations(void)\n> @@ -127,7 +130,7 @@ static void serve_one_client(FILE *in, FILE *out)\n> \tif (read_request(in, &c, &action, &timeout) < 0)\n> \t\t/* ignore error */ ;\n> \telse if (!strcmp(action.buf, \"get\")) {\n> -\t\tstruct credential_cache_entry *e = lookup_credential(&c, 0);\n> +\t\tstruct credential_cache_entry *e = lookup_credential(&c);\n> \t\tif (e) {\n> \t\t\tfprintf(out, \"username=%s\\n\", e->item.username);\n> \t\t\tfprintf(out, \"password=%s\\n\", e->item.password);\n> diff --git a/builtin/credential-store.c b/builtin/credential-store.c\n> index e0ae028b1c3..85b147e460f 100644\n> --- a/builtin/credential-store.c\n> +++ b/builtin/credential-store.c\n> @@ -36,7 +36,8 @@ static int parse_credential_file(const char *fn,\n> \t\t\tfound_credential = 1;\n> \t\t\tif (match_cb) {\n> \t\t\t\tmatch_cb(&entry);\n> -\t\t\t\tbreak;\n> +\t\t\t\tif (strcmp(op, \"erase\"))\n> +\t\t\t\t\tbreak;\n> \t\t\t}\n> \t\t}\n> \t\telse if (other_cb)\n> diff --git a/t/lib-credential.sh b/t/lib-credential.sh\n> index f7e4e29c5e1..3f4100b6ce2 100644\n> --- a/t/lib-credential.sh\n> +++ b/t/lib-credential.sh\n> @@ -45,6 +45,8 @@ helper_test_clean() {\n> \treject $1 https example.com user2\n> \treject $1 https example.com user4\n> \treject $1 https example.com user5\n> +\treject $1 https example.com user6\n> +\treject $1 https example.com user7\n> \treject $1 http path.tld user\n> \treject $1 https timeout.tld user\n> \treject $1 https sso.tld\n> @@ -298,6 +300,37 @@ helper_test() {\n> \t\tEOF\n> \t'\n> \n> +\ttest_expect_success \"helper ($HELPER) erases all matching credentials\" '\n> +\t\tcheck approve $HELPER <<-\\EOF &&\n> +\t\tprotocol=https\n> +\t\thost=example.com\n> +\t\tusername=user6\n> +\t\tpassword=pass1\n> +\t\tEOF\n> +\t\tcheck approve $HELPER <<-\\EOF &&\n> +\t\tprotocol=https\n> +\t\thost=example.com\n> +\t\tusername=user7\n> +\t\tpassword=pass1\n> +\t\tEOF\n> +\t\tcheck reject $HELPER <<-\\EOF &&\n> +\t\tprotocol=https\n> +\t\thost=example.com\n> +\t\tEOF\n> +\t\tcheck fill $HELPER <<-\\EOF\n> +\t\tprotocol=https\n> +\t\thost=example.com\n> +\t\t--\n> +\t\tprotocol=https\n> +\t\thost=example.com\n> +\t\tusername=askpass-username\n> +\t\tpassword=askpass-password\n> +\t\t--\n> +\t\taskpass: Username for '\\''https://example.com'\\'':\n> +\t\taskpass: Password for '\\''https://askpass-username@example.com'\\'':\n> +\t\tEOF\n> +\t'\n> +\n> \t: ${GIT_TEST_LONG_CRED_BUFFER:=1024}\n> \t# 23 bytes accounts for \"wwwauth[]=basic realm=\" plus NUL\n> \tLONG_VALUE_LEN=$((GIT_TEST_LONG_CRED_BUFFER - 23))\n"},{"id":"478398","messageId":"CAGJzqsmmBYep-iRYushkEztnNMTr52APKmC_nMRQBDbGujt3rg@mail.gmail.com","threadId":"59860","inReplyTo":"xmqqv8fqxcdj.fsf@gitster.g","subject":"Re: [PATCH 2/2] credential: erase all matching credentials","fromName":"M Hickford","fromEmail":"mirth.hickford@gmail.com","sentAt":"2023-06-14T21:35:20Z","receivedAt":"2023-06-14T21:36:01Z","isPatch":true,"sender":{"key":"mirth.hickford@gmail.com","avatar":"https://avatars.githubusercontent.com/u/105314?v=4"},"body":"On Wed, 14 Jun 2023 at 17:00, Junio C Hamano wrote:\n>\n> \"M Hickford via GitGitGadget\" writes:\n>\n> > From: M Hickford \n> >\n> > Users expect that\n> > `echo \"url=https://example.com\" | git credential reject` or\n> > `echo \"url=https://example.com\\nusername=tim\" | git credential reject`\n> > erase all matching credentials.\n>\n> Perhaps that is a sensible expectation. It is unclear from the\n> above description what happens instead without the \"fix\", though.\n\n`credential reject` sends the erase action to each helper, but the\nexact behaviour of erase isn't specified in documentation or tests.\nSome helpers (such as credential-libsecret) delete all matching\ncredentials, others (such as credential-cache and credential-store)\ndelete at most one matching credential.\n\nTest that helpers erase all matching credentials. This behaviour is\neasiest to reason about. Users expect that `echo\n\"url=https://example.com\" | git credential reject` or `echo\n\"url=https://example.com\\nusername=tim\" | git credential reject` erase\nall matching credentials.\n\nI'll update the commit message in patch v2.\n\n>\n> By the way, I do not think your\n>\n> cc. Jeff King peff@peff.net cc: Matthew John Cheetham mjcheetham@outlook.com\n>\n> in 0/2 is doing anything; I've manually added them to Cc: of this\n> message and left everything in the patch below, even though I am not\n> commenting on anything there myself, to give them easier reference.\n\nThanks Junio. I'll check the GitGitGadget documentation.\n\n>\n> Thanks.\n>\n> > Fixes for credential-cache and credential-store.\n> >\n> > Signed-off-by: M Hickford \n> > ---\n> > Documentation/git-credential.txt | 4 ++--\n> > Documentation/gitcredentials.txt | 2 +-\n> > builtin/credential-cache--daemon.c | 15 ++++++++------\n> > builtin/credential-store.c | 3 ++-\n> > t/lib-credential.sh | 33 ++++++++++++++++++++++++++++++\n> > 5 files changed, 47 insertions(+), 10 deletions(-)\n> >\n> > diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt\n> > index 0e6d9e85ec7..04bfb918de6 100644\n> > --- a/Documentation/git-credential.txt\n> > +++ b/Documentation/git-credential.txt\n> > @@ -38,8 +38,8 @@ to any configured credential helpers, which may store the credential\n> > for later use.\n> >\n> > If the action is `reject`, git-credential will send the description to\n> > -any configured credential helpers, which may erase any stored\n> > -credential matching the description.\n> > +any configured credential helpers, which may erase stored credentials\n> > +matching the description.\n> >\n> > If the action is `approve` or `reject`, no output should be emitted.\n> >\n> > diff --git a/Documentation/gitcredentials.txt b/Documentation/gitcredentials.txt\n> > index 100f045bb1a..65d652dc40e 100644\n> > --- a/Documentation/gitcredentials.txt\n> > +++ b/Documentation/gitcredentials.txt\n> > @@ -260,7 +260,7 @@ appended to its command line, which is one of:\n> >\n> > `erase`::\n> >\n> > - Remove a matching credential, if any, from the helper's storage.\n> > + Remove matching credentials, if any, from the helper's storage.\n> >\n> > The details of the credential will be provided on the helper's stdin\n> > stream. The exact format is the same as the input/output format of the\n> > diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c\n> > index 82f376d3351..5e3a766e42d 100644\n> > --- a/builtin/credential-cache--daemon.c\n> > +++ b/builtin/credential-cache--daemon.c\n> > @@ -33,12 +33,12 @@ static void cache_credential(struct credential *c, int timeout)\n> > e->expiration = time(NULL) + timeout;\n> > }\n> >\n> > -static struct credential_cache_entry *lookup_credential(const struct credential *c, int match_password)\n> > +static struct credential_cache_entry *lookup_credential(const struct credential *c)\n> > {\n> > int i;\n> > for (i = 0; i < entries_nr; i++) {\n> > struct credential *e = &entries[i].item;\n> > - if (credential_match(c, e, match_password))\n> > + if (credential_match(c, e, 0))\n> > return &entries[i];\n> > }\n> > return NULL;\n> > @@ -48,9 +48,12 @@ static void remove_credential(const struct credential *c)\n> > {\n> > struct credential_cache_entry *e;\n> >\n> > - e = lookup_credential(c, c->password != NULL);\n> > - if (e)\n> > - e->expiration = 0;\n> > + int i;\n> > + for (i = 0; i < entries_nr; i++) {\n> > + e = &entries[i];\n> > + if (credential_match(c, &e->item, c->password != NULL))\n> > + e->expiration = 0;\n> > + }\n> > }\n> >\n> > static timestamp_t check_expirations(void)\n> > @@ -127,7 +130,7 @@ static void serve_one_client(FILE *in, FILE *out)\n> > if (read_request(in, &c, &action, &timeout) < 0)\n> > /* ignore error */ ;\n> > else if (!strcmp(action.buf, \"get\")) {\n> > - struct credential_cache_entry *e = lookup_credential(&c, 0);\n> > + struct credential_cache_entry *e = lookup_credential(&c);\n> > if (e) {\n> > fprintf(out, \"username=%s\\n\", e->item.username);\n> > fprintf(out, \"password=%s\\n\", e->item.password);\n> > diff --git a/builtin/credential-store.c b/builtin/credential-store.c\n> > index e0ae028b1c3..85b147e460f 100644\n> > --- a/builtin/credential-store.c\n> > +++ b/builtin/credential-store.c\n> > @@ -36,7 +36,8 @@ static int parse_credential_file(const char *fn,\n> > found_credential = 1;\n> > if (match_cb) {\n> > match_cb(&entry);\n> > - break;\n> > + if (strcmp(op, \"erase\"))\n> > + break;\n> > }\n> > }\n> > else if (other_cb)\n> > diff --git a/t/lib-credential.sh b/t/lib-credential.sh\n> > index f7e4e29c5e1..3f4100b6ce2 100644\n> > --- a/t/lib-credential.sh\n> > +++ b/t/lib-credential.sh\n> > @@ -45,6 +45,8 @@ helper_test_clean() {\n> > reject $1 https example.com user2\n> > reject $1 https example.com user4\n> > reject $1 https example.com user5\n> > + reject $1 https example.com user6\n> > + reject $1 https example.com user7\n> > reject $1 http path.tld user\n> > reject $1 https timeout.tld user\n> > reject $1 https sso.tld\n> > @@ -298,6 +300,37 @@ helper_test() {\n> > EOF\n> > '\n> >\n> > + test_expect_success \"helper ($HELPER) erases all matching credentials\" '\n> > + check approve $HELPER <<-\\EOF &&\n> > + protocol=https\n> > + host=example.com\n> > + username=user6\n> > + password=pass1\n> > + EOF\n> > + check approve $HELPER <<-\\EOF &&\n> > + protocol=https\n> > + host=example.com\n> > + username=user7\n> > + password=pass1\n> > + EOF\n> > + check reject $HELPER <<-\\EOF &&\n> > + protocol=https\n> > + host=example.com\n> > + EOF\n> > + check fill $HELPER <<-\\EOF\n> > + protocol=https\n> > + host=example.com\n> > + --\n> > + protocol=https\n> > + host=example.com\n> > + username=askpass-username\n> > + password=askpass-password\n> > + --\n> > + askpass: Username for '\\''https://example.com'\\'':\n> > + askpass: Password for '\\''https://askpass-username@example.com'\\'':\n> > + EOF\n> > + '\n> > +\n> > : ${GIT_TEST_LONG_CRED_BUFFER:=1024}\n> > # 23 bytes accounts for \"wwwauth[]=basic realm=\" plus NUL\n> > LONG_VALUE_LEN=$((GIT_TEST_LONG_CRED_BUFFER - 23))\n"},{"id":"478400","messageId":"pull.1525.v2.git.git.1686778838.gitgitgadget@gmail.com","threadId":"59860","inReplyTo":"pull.1525.git.git.1686741785.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] credential: improvements to erase in helpers","fromName":"M Hickford via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-06-14T21:40:36Z","receivedAt":"2023-06-14T21:40:45Z","isPatch":true,"sender":{"key":"mirth.hickford@gmail.com","avatar":"https://avatars.githubusercontent.com/u/105314?v=4"},"body":"M Hickford (2):\n credential: avoid erasing distinct password\n credential: erase all matching credentials\n\n Documentation/git-credential.txt | 4 +-\n Documentation/gitcredentials.txt | 2 +-\n builtin/credential-cache--daemon.c | 11 ++++--\n builtin/credential-store.c | 21 ++++++-----\n credential.c | 11 +++---\n credential.h | 2 +-\n t/lib-credential.sh | 59 ++++++++++++++++++++++++++++++\n 7 files changed, 88 insertions(+), 22 deletions(-)\n\n\nbase-commit: fe86abd7511a9a6862d5706c6fa1d9b57a63ba09\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1525%2Fhickford%2Ferase-test-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1525/hickford/erase-test-v2\nPull-Request: https://github.com/git/git/pull/1525\n\nRange-diff vs v1:\n\n 1: 35ee1795bcd = 1: 35ee1795bcd credential: avoid erasing distinct password\n 2: fcdb579263f ! 2: 9b12f17dc7e credential: erase all matching credentials\n @@ Metadata\n ## Commit message ##\n credential: erase all matching credentials\n \n - Users expect that\n - `echo \"url=https://example.com\" | git credential reject` or\n - `echo \"url=https://example.com\\nusername=tim\" | git credential reject`\n - erase all matching credentials.\n + `credential reject` sends the erase action to each helper, but the\n + exact behaviour of erase isn't specified in documentation or tests.\n + Some helpers (such as credential-libsecret) delete all matching\n + credentials, others (such as credential-cache and credential-store)\n + delete at most one matching credential.\n \n - Fixes for credential-cache and credential-store.\n + Test that helpers erase all matching credentials. This behaviour is\n + easiest to reason about. Users expect that `echo\n + \"url=https://example.com\" | git credential reject` or `echo\n + \"url=https://example.com\\nusername=tim\" | git credential reject` erase\n + all matching credentials.\n +\n + Fix credential-cache and credential-store.\n \n Signed-off-by: M Hickford \n \n\n-- \ngitgitgadget\n"},{"id":"478401","messageId":"35ee1795bcdb974cdb9ca0f55ddf8f8a5bc562ae.1686778838.git.gitgitgadget@gmail.com","threadId":"59860","inReplyTo":"pull.1525.v2.git.git.1686778838.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] credential: avoid erasing distinct password","fromName":"M Hickford via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-06-14T21:40:37Z","receivedAt":"2023-06-14T21:40:47Z","isPatch":true,"sender":{"key":"mirth.hickford@gmail.com","avatar":"https://avatars.githubusercontent.com/u/105314?v=4"},"body":"From: M Hickford \n\nTest that credential helpers do not erase a password distinct from the\ninput. Such calls can happen when multiple credential helpers are\nconfigured.\n\nFixes for credential-cache and credential-store.\n\nSigned-off-by: M Hickford \n---\n builtin/credential-cache--daemon.c | 8 ++++----\n builtin/credential-store.c | 18 ++++++++++--------\n credential.c | 11 ++++++-----\n credential.h | 2 +-\n t/lib-credential.sh | 26 ++++++++++++++++++++++++++\n 5 files changed, 47 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c\nindex 756c5f02aef..82f376d3351 100644\n--- a/builtin/credential-cache--daemon.c\n+++ b/builtin/credential-cache--daemon.c\n@@ -33,12 +33,12 @@ static void cache_credential(struct credential *c, int timeout)\n \te->expiration = time(NULL) + timeout;\n }\n \n-static struct credential_cache_entry *lookup_credential(const struct credential *c)\n+static struct credential_cache_entry *lookup_credential(const struct credential *c, int match_password)\n {\n \tint i;\n \tfor (i = 0; i < entries_nr; i++) {\n \t\tstruct credential *e = &entries[i].item;\n-\t\tif (credential_match(c, e))\n+\t\tif (credential_match(c, e, match_password))\n \t\t\treturn &entries[i];\n \t}\n \treturn NULL;\n@@ -48,7 +48,7 @@ static void remove_credential(const struct credential *c)\n {\n \tstruct credential_cache_entry *e;\n \n-\te = lookup_credential(c);\n+\te = lookup_credential(c, c->password != NULL);\n \tif (e)\n \t\te->expiration = 0;\n }\n@@ -127,7 +127,7 @@ static void serve_one_client(FILE *in, FILE *out)\n \tif (read_request(in, &c, &action, &timeout) < 0)\n \t\t/* ignore error */ ;\n \telse if (!strcmp(action.buf, \"get\")) {\n-\t\tstruct credential_cache_entry *e = lookup_credential(&c);\n+\t\tstruct credential_cache_entry *e = lookup_credential(&c, 0);\n \t\tif (e) {\n \t\t\tfprintf(out, \"username=%s\\n\", e->item.username);\n \t\t\tfprintf(out, \"password=%s\\n\", e->item.password);\ndiff --git a/builtin/credential-store.c b/builtin/credential-store.c\nindex 30c6ccf56c0..e0ae028b1c3 100644\n--- a/builtin/credential-store.c\n+++ b/builtin/credential-store.c\n@@ -13,12 +13,14 @@ static struct lock_file credential_lock;\n static int parse_credential_file(const char *fn,\n \t\t\t\t struct credential *c,\n \t\t\t\t void (*match_cb)(struct credential *),\n-\t\t\t\t void (*other_cb)(struct strbuf *))\n+\t\t\t\t void (*other_cb)(struct strbuf *),\n+\t\t\t\t const char* op)\n {\n \tFILE *fh;\n \tstruct strbuf line = STRBUF_INIT;\n \tstruct credential entry = CREDENTIAL_INIT;\n \tint found_credential = 0;\n+\tconst int match_password = !strcmp(op, \"erase\") && c->password != NULL;\n \n \tfh = fopen(fn, \"r\");\n \tif (!fh) {\n@@ -29,8 +31,8 @@ static int parse_credential_file(const char *fn,\n \n \twhile (strbuf_getline_lf(&line, fh) != EOF) {\n \t\tif (!credential_from_url_gently(&entry, line.buf, 1) &&\n-\t\t entry.username && entry.password &&\n-\t\t credential_match(c, &entry)) {\n+\t\t\tentry.username && entry.password &&\n+\t\t\tcredential_match(c, &entry, match_password)) {\n \t\t\tfound_credential = 1;\n \t\t\tif (match_cb) {\n \t\t\t\tmatch_cb(&entry);\n@@ -60,7 +62,7 @@ static void print_line(struct strbuf *buf)\n }\n \n static void rewrite_credential_file(const char *fn, struct credential *c,\n-\t\t\t\t struct strbuf *extra)\n+\t\t\t\t struct strbuf *extra, const char *op)\n {\n \tint timeout_ms = 1000;\n \n@@ -69,7 +71,7 @@ static void rewrite_credential_file(const char *fn, struct credential *c,\n \t\tdie_errno(_(\"unable to get credential storage lock in %d ms\"), timeout_ms);\n \tif (extra)\n \t\tprint_line(extra);\n-\tparse_credential_file(fn, c, NULL, print_line);\n+\tparse_credential_file(fn, c, NULL, print_line, op);\n \tif (commit_lock_file(&credential_lock) < 0)\n \t\tdie_errno(\"unable to write credential store\");\n }\n@@ -91,7 +93,7 @@ static void store_credential_file(const char *fn, struct credential *c)\n \t\t\t\t\tis_rfc3986_reserved_or_unreserved);\n \t}\n \n-\trewrite_credential_file(fn, c, &buf);\n+\trewrite_credential_file(fn, c, &buf, \"store\");\n \tstrbuf_release(&buf);\n }\n \n@@ -138,7 +140,7 @@ static void remove_credential(const struct string_list *fns, struct credential *\n \t\treturn;\n \tfor_each_string_list_item(fn, fns)\n \t\tif (!access(fn->string, F_OK))\n-\t\t\trewrite_credential_file(fn->string, c, NULL);\n+\t\t\trewrite_credential_file(fn->string, c, NULL, \"erase\");\n }\n \n static void lookup_credential(const struct string_list *fns, struct credential *c)\n@@ -146,7 +148,7 @@ static void lookup_credential(const struct string_list *fns, struct credential *\n \tstruct string_list_item *fn;\n \n \tfor_each_string_list_item(fn, fns)\n-\t\tif (parse_credential_file(fn->string, c, print_entry, NULL))\n+\t\tif (parse_credential_file(fn->string, c, print_entry, NULL, \"get\"))\n \t\t\treturn; /* Found credential */\n }\n \ndiff --git a/credential.c b/credential.c\nindex 023b59d5711..9157ff0865e 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -33,13 +33,14 @@ void credential_clear(struct credential *c)\n }\n \n int credential_match(const struct credential *want,\n-\t\t const struct credential *have)\n+\t\t const struct credential *have, int match_password)\n {\n #define CHECK(x) (!want->x || (have->x && !strcmp(want->x, have->x)))\n \treturn CHECK(protocol) &&\n-\t CHECK(host) &&\n-\t CHECK(path) &&\n-\t CHECK(username);\n+\t\tCHECK(host) &&\n+\t\tCHECK(path) &&\n+\t\tCHECK(username) &&\n+\t\t(!match_password || CHECK(password));\n #undef CHECK\n }\n \n@@ -102,7 +103,7 @@ static int match_partial_url(const char *url, void *cb)\n \t\twarning(_(\"skipping credential lookup for key: credential.%s\"),\n \t\t\turl);\n \telse\n-\t\tmatches = credential_match(&want, c);\n+\t\tmatches = credential_match(&want, c, 0);\n \tcredential_clear(&want);\n \n \treturn matches;\ndiff --git a/credential.h b/credential.h\nindex b8e2936d1dc..acc41adf548 100644\n--- a/credential.h\n+++ b/credential.h\n@@ -211,6 +211,6 @@ void credential_from_url(struct credential *, const char *url);\n int credential_from_url_gently(struct credential *, const char *url, int quiet);\n \n int credential_match(const struct credential *want,\n-\t\t const struct credential *have);\n+\t\t const struct credential *have, int match_password);\n \n #endif /* CREDENTIAL_H */\ndiff --git a/t/lib-credential.sh b/t/lib-credential.sh\nindex f1ab92ba35c..f7e4e29c5e1 100644\n--- a/t/lib-credential.sh\n+++ b/t/lib-credential.sh\n@@ -44,6 +44,7 @@ helper_test_clean() {\n \treject $1 https example.com user1\n \treject $1 https example.com user2\n \treject $1 https example.com user4\n+\treject $1 https example.com user5\n \treject $1 http path.tld user\n \treject $1 https timeout.tld user\n \treject $1 https sso.tld\n@@ -221,6 +222,31 @@ helper_test() {\n \t\tEOF\n \t'\n \n+\ttest_expect_success \"helper ($HELPER) does not erase a password distinct from input\" '\n+\t\tcheck approve $HELPER <<-\\EOF &&\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\tusername=user5\n+\t\tpassword=pass1\n+\t\tEOF\n+\t\tcheck reject $HELPER <<-\\EOF &&\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\tusername=user5\n+\t\tpassword=pass2\n+\t\tEOF\n+\t\tcheck fill $HELPER <<-\\EOF\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\tusername=user5\n+\t\t--\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\tusername=user5\n+\t\tpassword=pass1\n+\t\tEOF\n+\t'\n+\n \ttest_expect_success \"helper ($HELPER) can forget user\" '\n \t\tcheck reject $HELPER <<-\\EOF &&\n \t\tprotocol=https\n-- \ngitgitgadget\n\n"},{"id":"478402","messageId":"9b12f17dc7ebbab9b1625ad0c6058133b8a53323.1686778838.git.gitgitgadget@gmail.com","threadId":"59860","inReplyTo":"pull.1525.v2.git.git.1686778838.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] credential: erase all matching credentials","fromName":"M Hickford via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-06-14T21:40:38Z","receivedAt":"2023-06-14T21:40:47Z","isPatch":true,"sender":{"key":"mirth.hickford@gmail.com","avatar":"https://avatars.githubusercontent.com/u/105314?v=4"},"body":"From: M Hickford \n\n`credential reject` sends the erase action to each helper, but the\nexact behaviour of erase isn't specified in documentation or tests.\nSome helpers (such as credential-libsecret) delete all matching\ncredentials, others (such as credential-cache and credential-store)\ndelete at most one matching credential.\n\nTest that helpers erase all matching credentials. This behaviour is\neasiest to reason about. Users expect that `echo\n\"url=https://example.com\" | git credential reject` or `echo\n\"url=https://example.com\\nusername=tim\" | git credential reject` erase\nall matching credentials.\n\nFix credential-cache and credential-store.\n\nSigned-off-by: M Hickford \n---\n Documentation/git-credential.txt | 4 ++--\n Documentation/gitcredentials.txt | 2 +-\n builtin/credential-cache--daemon.c | 15 ++++++++------\n builtin/credential-store.c | 3 ++-\n t/lib-credential.sh | 33 ++++++++++++++++++++++++++++++\n 5 files changed, 47 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt\nindex 0e6d9e85ec7..04bfb918de6 100644\n--- a/Documentation/git-credential.txt\n+++ b/Documentation/git-credential.txt\n@@ -38,8 +38,8 @@ to any configured credential helpers, which may store the credential\n for later use.\n \n If the action is `reject`, git-credential will send the description to\n-any configured credential helpers, which may erase any stored\n-credential matching the description.\n+any configured credential helpers, which may erase stored credentials\n+matching the description.\n \n If the action is `approve` or `reject`, no output should be emitted.\n \ndiff --git a/Documentation/gitcredentials.txt b/Documentation/gitcredentials.txt\nindex 100f045bb1a..65d652dc40e 100644\n--- a/Documentation/gitcredentials.txt\n+++ b/Documentation/gitcredentials.txt\n@@ -260,7 +260,7 @@ appended to its command line, which is one of:\n \n `erase`::\n \n-\tRemove a matching credential, if any, from the helper's storage.\n+\tRemove matching credentials, if any, from the helper's storage.\n \n The details of the credential will be provided on the helper's stdin\n stream. The exact format is the same as the input/output format of the\ndiff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c\nindex 82f376d3351..5e3a766e42d 100644\n--- a/builtin/credential-cache--daemon.c\n+++ b/builtin/credential-cache--daemon.c\n@@ -33,12 +33,12 @@ static void cache_credential(struct credential *c, int timeout)\n \te->expiration = time(NULL) + timeout;\n }\n \n-static struct credential_cache_entry *lookup_credential(const struct credential *c, int match_password)\n+static struct credential_cache_entry *lookup_credential(const struct credential *c)\n {\n \tint i;\n \tfor (i = 0; i < entries_nr; i++) {\n \t\tstruct credential *e = &entries[i].item;\n-\t\tif (credential_match(c, e, match_password))\n+\t\tif (credential_match(c, e, 0))\n \t\t\treturn &entries[i];\n \t}\n \treturn NULL;\n@@ -48,9 +48,12 @@ static void remove_credential(const struct credential *c)\n {\n \tstruct credential_cache_entry *e;\n \n-\te = lookup_credential(c, c->password != NULL);\n-\tif (e)\n-\t\te->expiration = 0;\n+\tint i;\n+\tfor (i = 0; i < entries_nr; i++) {\n+\t\te = &entries[i];\n+\t\tif (credential_match(c, &e->item, c->password != NULL))\n+\t\t\te->expiration = 0;\n+\t}\n }\n \n static timestamp_t check_expirations(void)\n@@ -127,7 +130,7 @@ static void serve_one_client(FILE *in, FILE *out)\n \tif (read_request(in, &c, &action, &timeout) < 0)\n \t\t/* ignore error */ ;\n \telse if (!strcmp(action.buf, \"get\")) {\n-\t\tstruct credential_cache_entry *e = lookup_credential(&c, 0);\n+\t\tstruct credential_cache_entry *e = lookup_credential(&c);\n \t\tif (e) {\n \t\t\tfprintf(out, \"username=%s\\n\", e->item.username);\n \t\t\tfprintf(out, \"password=%s\\n\", e->item.password);\ndiff --git a/builtin/credential-store.c b/builtin/credential-store.c\nindex e0ae028b1c3..85b147e460f 100644\n--- a/builtin/credential-store.c\n+++ b/builtin/credential-store.c\n@@ -36,7 +36,8 @@ static int parse_credential_file(const char *fn,\n \t\t\tfound_credential = 1;\n \t\t\tif (match_cb) {\n \t\t\t\tmatch_cb(&entry);\n-\t\t\t\tbreak;\n+\t\t\t\tif (strcmp(op, \"erase\"))\n+\t\t\t\t\tbreak;\n \t\t\t}\n \t\t}\n \t\telse if (other_cb)\ndiff --git a/t/lib-credential.sh b/t/lib-credential.sh\nindex f7e4e29c5e1..3f4100b6ce2 100644\n--- a/t/lib-credential.sh\n+++ b/t/lib-credential.sh\n@@ -45,6 +45,8 @@ helper_test_clean() {\n \treject $1 https example.com user2\n \treject $1 https example.com user4\n \treject $1 https example.com user5\n+\treject $1 https example.com user6\n+\treject $1 https example.com user7\n \treject $1 http path.tld user\n \treject $1 https timeout.tld user\n \treject $1 https sso.tld\n@@ -298,6 +300,37 @@ helper_test() {\n \t\tEOF\n \t'\n \n+\ttest_expect_success \"helper ($HELPER) erases all matching credentials\" '\n+\t\tcheck approve $HELPER <<-\\EOF &&\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\tusername=user6\n+\t\tpassword=pass1\n+\t\tEOF\n+\t\tcheck approve $HELPER <<-\\EOF &&\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\tusername=user7\n+\t\tpassword=pass1\n+\t\tEOF\n+\t\tcheck reject $HELPER <<-\\EOF &&\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\tEOF\n+\t\tcheck fill $HELPER <<-\\EOF\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\t--\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\tusername=askpass-username\n+\t\tpassword=askpass-password\n+\t\t--\n+\t\taskpass: Username for '\\''https://example.com'\\'':\n+\t\taskpass: Password for '\\''https://askpass-username@example.com'\\'':\n+\t\tEOF\n+\t'\n+\n \t: ${GIT_TEST_LONG_CRED_BUFFER:=1024}\n \t# 23 bytes accounts for \"wwwauth[]=basic realm=\" plus NUL\n \tLONG_VALUE_LEN=$((GIT_TEST_LONG_CRED_BUFFER - 23))\n-- \ngitgitgadget\n"},{"id":"478404","messageId":"xmqqjzw5wvwk.fsf@gitster.g","threadId":"59860","inReplyTo":"pull.1525.v2.git.git.1686778838.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/2] credential: improvements to erase in helpers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-14T21:56:11Z","receivedAt":"2023-06-14T21:56:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"M Hickford via GitGitGadget\" writes:\n\n> M Hickford (2):\n> credential: avoid erasing distinct password\n> credential: erase all matching credentials\n\nWill queue. Comments by folks with more experience (than me) who\nhave worked on the credential subsystem are very much welcomed.\n\nThanks.\n\n>\n> Documentation/git-credential.txt | 4 +-\n> Documentation/gitcredentials.txt | 2 +-\n> builtin/credential-cache--daemon.c | 11 ++++--\n> builtin/credential-store.c | 21 ++++++-----\n> credential.c | 11 +++---\n> credential.h | 2 +-\n> t/lib-credential.sh | 59 ++++++++++++++++++++++++++++++\n> 7 files changed, 88 insertions(+), 22 deletions(-)\n>\n>\n> base-commit: fe86abd7511a9a6862d5706c6fa1d9b57a63ba09\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1525%2Fhickford%2Ferase-test-v2\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1525/hickford/erase-test-v2\n> Pull-Request: https://github.com/git/git/pull/1525\n>\n> Range-diff vs v1:\n>\n> 1: 35ee1795bcd = 1: 35ee1795bcd credential: avoid erasing distinct password\n> 2: fcdb579263f ! 2: 9b12f17dc7e credential: erase all matching credentials\n> @@ Metadata\n> ## Commit message ##\n> credential: erase all matching credentials\n> \n> - Users expect that\n> - `echo \"url=https://example.com\" | git credential reject` or\n> - `echo \"url=https://example.com\\nusername=tim\" | git credential reject`\n> - erase all matching credentials.\n> + `credential reject` sends the erase action to each helper, but the\n> + exact behaviour of erase isn't specified in documentation or tests.\n> + Some helpers (such as credential-libsecret) delete all matching\n> + credentials, others (such as credential-cache and credential-store)\n> + delete at most one matching credential.\n> \n> - Fixes for credential-cache and credential-store.\n> + Test that helpers erase all matching credentials. This behaviour is\n> + easiest to reason about. Users expect that `echo\n> + \"url=https://example.com\" | git credential reject` or `echo\n> + \"url=https://example.com\\nusername=tim\" | git credential reject` erase\n> + all matching credentials.\n> +\n> + Fix credential-cache and credential-store.\n> \n> Signed-off-by: M Hickford \n"},{"id":"478405","messageId":"20230614224354.GA1441072@coredump.intra.peff.net","threadId":"59860","inReplyTo":"35ee1795bcdb974cdb9ca0f55ddf8f8a5bc562ae.1686778838.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/2] credential: avoid erasing distinct password","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-06-14T22:43:54Z","receivedAt":"2023-06-14T22:44:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 14, 2023 at 09:40:37PM +0000, M Hickford via GitGitGadget wrote:\n\n> From: M Hickford \n> \n> Test that credential helpers do not erase a password distinct from the\n> input. Such calls can happen when multiple credential helpers are\n> configured.\n> \n> Fixes for credential-cache and credential-store.\n\nI think this goal makes sense. I missed the \"multiple helpers\" part of\nyour message at first, and wondered how you would even get such\nduplicate entries stored in a helper. But the point is that you might\nhave two helpers with two separate passwords.\n\nWhich is...weird, I'd think, because we will only ever use one of them.\nBut from past discussions, I guess you're in a situation where you could\nuse some kind of renewable token _or_ possibly a more stable password,\nvia two different helpers. And when the token expires, you want to be\nable to remove it from a caching helper without asking for the stable\npassword to be dropped.\n\nI don't think it's strictly necessary to spell out the intended use in\nthe commit message. The behavior you're proposing seems like the\nleast-surprising thing to me in general (and it is only because the\nsituation is rare that nobody complained about the existing behavior in\nthe last decade). But it might not hurt to outline the case that we'd\nexpect this to help.\n\nThe other thing I'd watch out for is how this behavior would affect\nnon-erase modes. Let's see...\n\n> diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c\n> index 756c5f02aef..82f376d3351 100644\n> --- a/builtin/credential-cache--daemon.c\n> +++ b/builtin/credential-cache--daemon.c\n> @@ -33,12 +33,12 @@ static void cache_credential(struct credential *c, int timeout)\n> \te->expiration = time(NULL) + timeout;\n> }\n> \n> -static struct credential_cache_entry *lookup_credential(const struct credential *c)\n> +static struct credential_cache_entry *lookup_credential(const struct credential *c, int match_password)\n> {\n> \tint i;\n> \tfor (i = 0; i < entries_nr; i++) {\n> \t\tstruct credential *e = &entries[i].item;\n> -\t\tif (credential_match(c, e))\n> +\t\tif (credential_match(c, e, match_password))\n> \t\t\treturn &entries[i];\n> \t}\n> \treturn NULL;\n> [...]\n> @@ -127,7 +127,7 @@ static void serve_one_client(FILE *in, FILE *out)\n> \tif (read_request(in, &c, &action, &timeout) < 0)\n> \t\t/* ignore error */ ;\n> \telse if (!strcmp(action.buf, \"get\")) {\n> -\t\tstruct credential_cache_entry *e = lookup_credential(&c);\n> +\t\tstruct credential_cache_entry *e = lookup_credential(&c, 0);\n> \t\tif (e) {\n> \t\t\tfprintf(out, \"username=%s\\n\", e->item.username);\n> \t\t\tfprintf(out, \"password=%s\\n\", e->item.password);\n\nOK, for a cache \"get\" we'll continue not to match the password. This\nshouldn't trigger in practice, as Git will stop asking helpers once it\nhas a username and password, but it is good to retain the existing\nbehavior here.\n\n> @@ -48,7 +48,7 @@ static void remove_credential(const struct credential *c)\n> {\n> \tstruct credential_cache_entry *e;\n> \n> -\te = lookup_credential(c);\n> +\te = lookup_credential(c, c->password != NULL);\n> \tif (e)\n> \t\te->expiration = 0;\n> }\n\nAnd this is erase, where we do want to respect it. I actually think\npassing an unconditional \"1\" here instead of the NULL check would be OK,\nas the CHECK() macro in credential_match() treats NULL as \"always\nmatch\". But I am also OK with making that logic explicit here.\n\nThis made me wonder how \"store\" works, since it's not touched here. It's\nimplemented as remove_credential(), plus an unconditional allocation via\ncache_credential().\n\nIt seems like overwriting would be broken, then, wouldn't it? If I\nstore:\n\n https://user:pass1@example.com\n\nand then try to store:\n\n https://user:pass2@example.com\n\nthen the call to remove_credential() will see a \"struct credential\" with\nthe password set to \"pass2\". And because it always passes\nmatch_password, it will fail to remove it, and we'll end up with two.\n\nWe should be able to test that pretty easily, like this:\n\n echo url=https://user:pass1@example.com | git credential-cache store\n echo url=https://user:pass2@example.com | git credential-cache store\n echo url=https://example.com | git credential-cache get\n\nBefore your patch that will return \"pass1\". After, it returns \"pass2\",\nbecause the old credential is left in place. I think you'd want\nsomething like:\n\ndiff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c\nindex 82f376d335..f64dd21d33 100644\n--- a/builtin/credential-cache--daemon.c\n+++ b/builtin/credential-cache--daemon.c\n@@ -44,11 +44,11 @@ static struct credential_cache_entry *lookup_credential(const struct credential\n \treturn NULL;\n }\n \n-static void remove_credential(const struct credential *c)\n+static void remove_credential(const struct credential *c, int match_password)\n {\n \tstruct credential_cache_entry *e;\n \n-\te = lookup_credential(c, c->password != NULL);\n+\te = lookup_credential(c, match_password);\n \tif (e)\n \t\te->expiration = 0;\n }\n@@ -151,14 +151,14 @@ static void serve_one_client(FILE *in, FILE *out)\n \t\texit(0);\n \t}\n \telse if (!strcmp(action.buf, \"erase\"))\n-\t\tremove_credential(&c);\n+\t\tremove_credential(&c, 1);\n \telse if (!strcmp(action.buf, \"store\")) {\n \t\tif (timeout < 0)\n \t\t\twarning(\"cache client didn't specify a timeout\");\n \t\telse if (!c.username || !c.password)\n \t\t\twarning(\"cache client gave us a partial credential\");\n \t\telse {\n-\t\t\tremove_credential(&c);\n+\t\t\tremove_credential(&c, 0);\n \t\t\tcache_credential(&c, timeout);\n \t\t}\n \t}\n\nUnless your goal really is to store two entries that differ only in\ntheir passwords within a single cache. That seems weird, though. Again,\nit might help if your use case was fleshed out a bit more.\n\n> diff --git a/builtin/credential-store.c b/builtin/credential-store.c\n> index 30c6ccf56c0..e0ae028b1c3 100644\n> --- a/builtin/credential-store.c\n> +++ b/builtin/credential-store.c\n> @@ -13,12 +13,14 @@ static struct lock_file credential_lock;\n> static int parse_credential_file(const char *fn,\n> \t\t\t\t struct credential *c,\n> \t\t\t\t void (*match_cb)(struct credential *),\n> -\t\t\t\t void (*other_cb)(struct strbuf *))\n> +\t\t\t\t void (*other_cb)(struct strbuf *),\n> +\t\t\t\t const char* op)\n> {\n> \tFILE *fh;\n> \tstruct strbuf line = STRBUF_INIT;\n> \tstruct credential entry = CREDENTIAL_INIT;\n> \tint found_credential = 0;\n> +\tconst int match_password = !strcmp(op, \"erase\") && c->password != NULL;\n> \n> \tfh = fopen(fn, \"r\");\n> \tif (!fh) {\n> @@ -29,8 +31,8 @@ static int parse_credential_file(const char *fn,\n> \n> \twhile (strbuf_getline_lf(&line, fh) != EOF) {\n> \t\tif (!credential_from_url_gently(&entry, line.buf, 1) &&\n> -\t\t entry.username && entry.password &&\n> -\t\t credential_match(c, &entry)) {\n> +\t\t\tentry.username && entry.password &&\n> +\t\t\tcredential_match(c, &entry, match_password)) {\n> \t\t\tfound_credential = 1;\n> \t\t\tif (match_cb) {\n> \t\t\t\tmatch_cb(&entry);\n\nOK, so for credential-store we only kick in the new behavior for\n\"erase\". So it would not have the same quirk that I mentioned above.\n\nI do think your logic could likewise here be:\n\n const int match_password = !strcmp(op, \"erase\");\n\nbecause credential_match() handles the NULL case already.\n\n(It's also unusual for us to declare non-parameter variables as \"const\";\nit's not wrong to do so, so it's really just a style thing, but there's\nnot much benefit).\n\n> @@ -60,7 +62,7 @@ static void print_line(struct strbuf *buf)\n> }\n> \n> static void rewrite_credential_file(const char *fn, struct credential *c,\n> -\t\t\t\t struct strbuf *extra)\n> +\t\t\t\t struct strbuf *extra, const char *op)\n> {\n> \tint timeout_ms = 1000;\n> \n> @@ -69,7 +71,7 @@ static void rewrite_credential_file(const char *fn, struct credential *c,\n> \t\tdie_errno(_(\"unable to get credential storage lock in %d ms\"), timeout_ms);\n> \tif (extra)\n> \t\tprint_line(extra);\n> -\tparse_credential_file(fn, c, NULL, print_line);\n> +\tparse_credential_file(fn, c, NULL, print_line, op);\n> \tif (commit_lock_file(&credential_lock) < 0)\n> \t\tdie_errno(\"unable to write credential store\");\n> }\n\nIf we have to plumb through this \"op\" string, I wonder if it would be\nmore flexible and robust to just pass along a match_password flag.\nThat's generally a bit more robust than string matching (where a\nmisspelling in one string would not be caught by the compiler, unlike a\nboolean variable).\n\n> @@ -221,6 +222,31 @@ helper_test() {\n> \t\tEOF\n> \t'\n> \n> +\ttest_expect_success \"helper ($HELPER) does not erase a password distinct from input\" '\n> +\t\tcheck approve $HELPER <<-\\EOF &&\n> +\t\tprotocol=https\n> +\t\thost=example.com\n> +\t\tusername=user5\n> +\t\tpassword=pass1\n> +\t\tEOF\n> +\t\tcheck reject $HELPER <<-\\EOF &&\n> +\t\tprotocol=https\n> +\t\thost=example.com\n> +\t\tusername=user5\n> +\t\tpassword=pass2\n> +\t\tEOF\n> +\t\tcheck fill $HELPER <<-\\EOF\n> +\t\tprotocol=https\n> +\t\thost=example.com\n> +\t\tusername=user5\n> +\t\t--\n> +\t\tprotocol=https\n> +\t\thost=example.com\n> +\t\tusername=user5\n> +\t\tpassword=pass1\n> +\t\tEOF\n> +\t'\n\nThe test makes sense. We are not using multiple helpers, but the\nbehavior can be checked using a single helper, since the point is that\nit should be independent of what any other helper is doing.\n\n-Peff\n"},{"id":"478406","messageId":"20230614225116.GB1441072@coredump.intra.peff.net","threadId":"59860","inReplyTo":"9b12f17dc7ebbab9b1625ad0c6058133b8a53323.1686778838.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/2] credential: erase all matching credentials","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-06-14T22:51:16Z","receivedAt":"2023-06-14T22:51:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 14, 2023 at 09:40:38PM +0000, M Hickford via GitGitGadget wrote:\n\n> From: M Hickford \n> \n> `credential reject` sends the erase action to each helper, but the\n> exact behaviour of erase isn't specified in documentation or tests.\n> Some helpers (such as credential-libsecret) delete all matching\n> credentials, others (such as credential-cache and credential-store)\n> delete at most one matching credential.\n> \n> Test that helpers erase all matching credentials. This behaviour is\n> easiest to reason about. Users expect that `echo\n> \"url=https://example.com\" | git credential reject` or `echo\n> \"url=https://example.com\\nusername=tim\" | git credential reject` erase\n> all matching credentials.\n> \n> Fix credential-cache and credential-store.\n\nI think this was how it was intended to work all along, but I agree that\ncredential-cache does not behave this way. I think credential-store\nalready works, though.\n\n> diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt\n> index 0e6d9e85ec7..04bfb918de6 100644\n> --- a/Documentation/git-credential.txt\n> +++ b/Documentation/git-credential.txt\n> @@ -38,8 +38,8 @@ to any configured credential helpers, which may store the credential\n> for later use.\n> \n> If the action is `reject`, git-credential will send the description to\n> -any configured credential helpers, which may erase any stored\n> -credential matching the description.\n> +any configured credential helpers, which may erase stored credentials\n> +matching the description.\n\nI read the existing documentation as claiming to delete all such\nmatching credentials, but I guess it is ambiguous (and I did not look,\nbut it is almost a certainty that I wrote the original ;) ).\n\nSaying \"credentials\" in the plural makes it more clear we are getting\nall of them. But losing \"any\" makes it less clear to me. Maybe:\n\n ...which may erase any stored credentials matching the description.\n\nis the best of both?\n\n> diff --git a/Documentation/gitcredentials.txt b/Documentation/gitcredentials.txt\n> index 100f045bb1a..65d652dc40e 100644\n> --- a/Documentation/gitcredentials.txt\n> +++ b/Documentation/gitcredentials.txt\n> @@ -260,7 +260,7 @@ appended to its command line, which is one of:\n> \n> `erase`::\n> \n> -\tRemove a matching credential, if any, from the helper's storage.\n> +\tRemove matching credentials, if any, from the helper's storage.\n\nThis one seems like a strict improvement in ambiguity.\n\n> diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c\n> index 82f376d3351..5e3a766e42d 100644\n> --- a/builtin/credential-cache--daemon.c\n> +++ b/builtin/credential-cache--daemon.c\n> [...]\n> @@ -48,9 +48,12 @@ static void remove_credential(const struct credential *c)\n> {\n> \tstruct credential_cache_entry *e;\n> \n> -\te = lookup_credential(c, c->password != NULL);\n> -\tif (e)\n> -\t\te->expiration = 0;\n> +\tint i;\n> +\tfor (i = 0; i < entries_nr; i++) {\n> +\t\te = &entries[i];\n> +\t\tif (credential_match(c, &e->item, c->password != NULL))\n> +\t\t\te->expiration = 0;\n> +\t}\n> }\n\nOK, so now instead of looking up a single credential, we loop and cover\nthem all. That makes perfect sense. It is a little funny that we undo\nthe extra parameter to lookup_credential() that was added in the\nprevious commit. I wonder if reversing the order of the patches would\nmake it simpler, but it is probably not a big deal either way.\n\n> diff --git a/builtin/credential-store.c b/builtin/credential-store.c\n> index e0ae028b1c3..85b147e460f 100644\n> --- a/builtin/credential-store.c\n> +++ b/builtin/credential-store.c\n> @@ -36,7 +36,8 @@ static int parse_credential_file(const char *fn,\n> \t\t\tfound_credential = 1;\n> \t\t\tif (match_cb) {\n> \t\t\t\tmatch_cb(&entry);\n> -\t\t\t\tbreak;\n> +\t\t\t\tif (strcmp(op, \"erase\"))\n> +\t\t\t\t\tbreak;\n> \t\t\t}\n> \t\t}\n> \t\telse if (other_cb)\n\nI think this hunk does nothing. When the op is \"erase\", we do not pass a\nmatch_cb at all, so this break would never trigger (and indeed, it would\nbe disastrous if it did, because we would stop copying entries from the\nold file to the new, losing everything after the matched entry).\n\nAnd I think if you revert this, your new test still passes.\n\n> @@ -298,6 +300,37 @@ helper_test() {\n> \t\tEOF\n> \t'\n> \n> +\ttest_expect_success \"helper ($HELPER) erases all matching credentials\" '\n> +\t\tcheck approve $HELPER <<-\\EOF &&\n> +\t\tprotocol=https\n> +\t\thost=example.com\n> +\t\tusername=user6\n> +\t\tpassword=pass1\n> +\t\tEOF\n> +\t\tcheck approve $HELPER <<-\\EOF &&\n> +\t\tprotocol=https\n> +\t\thost=example.com\n> +\t\tusername=user7\n> +\t\tpassword=pass1\n> +\t\tEOF\n> +\t\tcheck reject $HELPER <<-\\EOF &&\n> +\t\tprotocol=https\n> +\t\thost=example.com\n> +\t\tEOF\n> +\t\tcheck fill $HELPER <<-\\EOF\n> +\t\tprotocol=https\n> +\t\thost=example.com\n> +\t\t--\n> +\t\tprotocol=https\n> +\t\thost=example.com\n> +\t\tusername=askpass-username\n> +\t\tpassword=askpass-password\n> +\t\t--\n> +\t\taskpass: Username for '\\''https://example.com'\\'':\n> +\t\taskpass: Password for '\\''https://askpass-username@example.com'\\'':\n> +\t\tEOF\n> +\t'\n\nThe test makes sense. We add two, delete them both, and then check that\nnothing is left to serve.\n\n-Peff\n"},{"id":"478407","messageId":"20230614225154.GC1441072@coredump.intra.peff.net","threadId":"59860","inReplyTo":"xmqqjzw5wvwk.fsf@gitster.g","subject":"Re: [PATCH v2 0/2] credential: improvements to erase in helpers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-06-14T22:51:54Z","receivedAt":"2023-06-14T22:51:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 14, 2023 at 02:56:11PM -0700, Junio C Hamano wrote:\n\n> \"M Hickford via GitGitGadget\" writes:\n> \n> > M Hickford (2):\n> > credential: avoid erasing distinct password\n> > credential: erase all matching credentials\n> \n> Will queue. Comments by folks with more experience (than me) who\n> have worked on the credential subsystem are very much welcomed.\n\nI think the direction is good, but there are a few small bits in each\npatch to tweak, and we'll want a v3.\n\n-Peff\n"},{"id":"478424","messageId":"CAGJzqsm9iVydk7Ycg+D7B04NU=nW-jb3vz6fEWJ9nh5pLjZgnQ@mail.gmail.com","threadId":"59860","inReplyTo":"20230614224354.GA1441072@coredump.intra.peff.net","subject":"Re: [PATCH v2 1/2] credential: avoid erasing distinct password","fromName":"M Hickford","fromEmail":"mirth.hickford@gmail.com","sentAt":"2023-06-15T04:51:49Z","receivedAt":"2023-06-15T04:52:32Z","isPatch":true,"sender":{"key":"mirth.hickford@gmail.com","avatar":"https://avatars.githubusercontent.com/u/105314?v=4"},"body":"On Wed, 14 Jun 2023 at 23:44, Jeff King wrote:\n>\n> On Wed, Jun 14, 2023 at 09:40:37PM +0000, M Hickford via GitGitGadget wrote:\n>\n> > From: M Hickford \n> >\n> > Test that credential helpers do not erase a password distinct from the\n> > input. Such calls can happen when multiple credential helpers are\n> > configured.\n> >\n> > Fixes for credential-cache and credential-store.\n>\n> I think this goal makes sense. I missed the \"multiple helpers\" part of\n> your message at first, and wondered how you would even get such\n> duplicate entries stored in a helper. But the point is that you might\n> have two helpers with two separate passwords.\n>\n> Which is...weird, I'd think, because we will only ever use one of them.\n> But from past discussions, I guess you're in a situation where you could\n> use some kind of renewable token _or_ possibly a more stable password,\n> via two different helpers. And when the token expires, you want to be\n> able to remove it from a caching helper without asking for the stable\n> password to be dropped.\n>\n> I don't think it's strictly necessary to spell out the intended use in\n> the commit message. The behavior you're proposing seems like the\n> least-surprising thing to me in general (and it is only because the\n> situation is rare that nobody complained about the existing behavior in\n> the last decade). But it might not hurt to outline the case that we'd\n> expect this to help.\n>\n> The other thing I'd watch out for is how this behavior would affect\n> non-erase modes. Let's see...\n>\n> > diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c\n> > index 756c5f02aef..82f376d3351 100644\n> > --- a/builtin/credential-cache--daemon.c\n> > +++ b/builtin/credential-cache--daemon.c\n> > @@ -33,12 +33,12 @@ static void cache_credential(struct credential *c, int timeout)\n> > e->expiration = time(NULL) + timeout;\n> > }\n> >\n> > -static struct credential_cache_entry *lookup_credential(const struct credential *c)\n> > +static struct credential_cache_entry *lookup_credential(const struct credential *c, int match_password)\n> > {\n> > int i;\n> > for (i = 0; i < entries_nr; i++) {\n> > struct credential *e = &entries[i].item;\n> > - if (credential_match(c, e))\n> > + if (credential_match(c, e, match_password))\n> > return &entries[i];\n> > }\n> > return NULL;\n> > [...]\n> > @@ -127,7 +127,7 @@ static void serve_one_client(FILE *in, FILE *out)\n> > if (read_request(in, &c, &action, &timeout) < 0)\n> > /* ignore error */ ;\n> > else if (!strcmp(action.buf, \"get\")) {\n> > - struct credential_cache_entry *e = lookup_credential(&c);\n> > + struct credential_cache_entry *e = lookup_credential(&c, 0);\n> > if (e) {\n> > fprintf(out, \"username=%s\\n\", e->item.username);\n> > fprintf(out, \"password=%s\\n\", e->item.password);\n>\n> OK, for a cache \"get\" we'll continue not to match the password. This\n> shouldn't trigger in practice, as Git will stop asking helpers once it\n> has a username and password, but it is good to retain the existing\n> behavior here.\n>\n> > @@ -48,7 +48,7 @@ static void remove_credential(const struct credential *c)\n> > {\n> > struct credential_cache_entry *e;\n> >\n> > - e = lookup_credential(c);\n> > + e = lookup_credential(c, c->password != NULL);\n> > if (e)\n> > e->expiration = 0;\n> > }\n>\n> And this is erase, where we do want to respect it. I actually think\n> passing an unconditional \"1\" here instead of the NULL check would be OK,\n> as the CHECK() macro in credential_match() treats NULL as \"always\n> match\". But I am also OK with making that logic explicit here.\n>\n> This made me wonder how \"store\" works, since it's not touched here. It's\n> implemented as remove_credential(), plus an unconditional allocation via\n> cache_credential().\n>\n> It seems like overwriting would be broken, then, wouldn't it? If I\n\nThanks Jeff for spotting and for the fix. I'll apply and add a test in patch v3.\n\n> store:\n>\n> https://user:pass1@example.com\n>\n> and then try to store:\n>\n> https://user:pass2@example.com\n>\n> then the call to remove_credential() will see a \"struct credential\" with\n> the password set to \"pass2\". And because it always passes\n> match_password, it will fail to remove it, and we'll end up with two.\n>\n> We should be able to test that pretty easily, like this:\n>\n> echo url=https://user:pass1@example.com | git credential-cache store\n> echo url=https://user:pass2@example.com | git credential-cache store\n> echo url=https://example.com | git credential-cache get\n>\n> Before your patch that will return \"pass1\". After, it returns \"pass2\",\n> because the old credential is left in place. I think you'd want\n> something like:\n>\n> diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c\n> index 82f376d335..f64dd21d33 100644\n> --- a/builtin/credential-cache--daemon.c\n> +++ b/builtin/credential-cache--daemon.c\n> @@ -44,11 +44,11 @@ static struct credential_cache_entry *lookup_credential(const struct credential\n> return NULL;\n> }\n>\n> -static void remove_credential(const struct credential *c)\n> +static void remove_credential(const struct credential *c, int match_password)\n> {\n> struct credential_cache_entry *e;\n>\n> - e = lookup_credential(c, c->password != NULL);\n> + e = lookup_credential(c, match_password);\n> if (e)\n> e->expiration = 0;\n> }\n> @@ -151,14 +151,14 @@ static void serve_one_client(FILE *in, FILE *out)\n> exit(0);\n> }\n> else if (!strcmp(action.buf, \"erase\"))\n> - remove_credential(&c);\n> + remove_credential(&c, 1);\n> else if (!strcmp(action.buf, \"store\")) {\n> if (timeout < 0)\n> warning(\"cache client didn't specify a timeout\");\n> else if (!c.username || !c.password)\n> warning(\"cache client gave us a partial credential\");\n> else {\n> - remove_credential(&c);\n> + remove_credential(&c, 0);\n> cache_credential(&c, timeout);\n> }\n> }\n>\n> Unless your goal really is to store two entries that differ only in\n> their passwords within a single cache. That seems weird, though. Again,\n> it might help if your use case was fleshed out a bit more.\n>\n> > diff --git a/builtin/credential-store.c b/builtin/credential-store.c\n> > index 30c6ccf56c0..e0ae028b1c3 100644\n> > --- a/builtin/credential-store.c\n> > +++ b/builtin/credential-store.c\n> > @@ -13,12 +13,14 @@ static struct lock_file credential_lock;\n> > static int parse_credential_file(const char *fn,\n> > struct credential *c,\n> > void (*match_cb)(struct credential *),\n> > - void (*other_cb)(struct strbuf *))\n> > + void (*other_cb)(struct strbuf *),\n> > + const char* op)\n> > {\n> > FILE *fh;\n> > struct strbuf line = STRBUF_INIT;\n> > struct credential entry = CREDENTIAL_INIT;\n> > int found_credential = 0;\n> > + const int match_password = !strcmp(op, \"erase\") && c->password != NULL;\n> >\n> > fh = fopen(fn, \"r\");\n> > if (!fh) {\n> > @@ -29,8 +31,8 @@ static int parse_credential_file(const char *fn,\n> >\n> > while (strbuf_getline_lf(&line, fh) != EOF) {\n> > if (!credential_from_url_gently(&entry, line.buf, 1) &&\n> > - entry.username && entry.password &&\n> > - credential_match(c, &entry)) {\n> > + entry.username && entry.password &&\n> > + credential_match(c, &entry, match_password)) {\n> > found_credential = 1;\n> > if (match_cb) {\n> > match_cb(&entry);\n>\n> OK, so for credential-store we only kick in the new behavior for\n> \"erase\". So it would not have the same quirk that I mentioned above.\n>\n> I do think your logic could likewise here be:\n>\n> const int match_password = !strcmp(op, \"erase\");\n>\n> because credential_match() handles the NULL case already.\n>\n> (It's also unusual for us to declare non-parameter variables as \"const\";\n> it's not wrong to do so, so it's really just a style thing, but there's\n> not much benefit).\n>\n> > @@ -60,7 +62,7 @@ static void print_line(struct strbuf *buf)\n> > }\n> >\n> > static void rewrite_credential_file(const char *fn, struct credential *c,\n> > - struct strbuf *extra)\n> > + struct strbuf *extra, const char *op)\n> > {\n> > int timeout_ms = 1000;\n> >\n> > @@ -69,7 +71,7 @@ static void rewrite_credential_file(const char *fn, struct credential *c,\n> > die_errno(_(\"unable to get credential storage lock in %d ms\"), timeout_ms);\n> > if (extra)\n> > print_line(extra);\n> > - parse_credential_file(fn, c, NULL, print_line);\n> > + parse_credential_file(fn, c, NULL, print_line, op);\n> > if (commit_lock_file(&credential_lock) < 0)\n> > die_errno(\"unable to write credential store\");\n> > }\n>\n> If we have to plumb through this \"op\" string, I wonder if it would be\n> more flexible and robust to just pass along a match_password flag.\n> That's generally a bit more robust than string matching (where a\n> misspelling in one string would not be caught by the compiler, unlike a\n> boolean variable).\n\nGood idea. Will do in patch v3.\n\n>\n> > @@ -221,6 +222,31 @@ helper_test() {\n> > EOF\n> > '\n> >\n> > + test_expect_success \"helper ($HELPER) does not erase a password distinct from input\" '\n> > + check approve $HELPER <<-\\EOF &&\n> > + protocol=https\n> > + host=example.com\n> > + username=user5\n> > + password=pass1\n> > + EOF\n> > + check reject $HELPER <<-\\EOF &&\n> > + protocol=https\n> > + host=example.com\n> > + username=user5\n> > + password=pass2\n> > + EOF\n> > + check fill $HELPER <<-\\EOF\n> > + protocol=https\n> > + host=example.com\n> > + username=user5\n> > + --\n> > + protocol=https\n> > + host=example.com\n> > + username=user5\n> > + password=pass1\n> > + EOF\n> > + '\n>\n> The test makes sense. We are not using multiple helpers, but the\n> behavior can be checked using a single helper, since the point is that\n> it should be independent of what any other helper is doing.\n>\n> -Peff\n"},{"id":"478425","messageId":"CAGJzqsk-dNtPJFyqGxm_szi4f7Rj3uU7d_WiSybqeJqX5=4isg@mail.gmail.com","threadId":"59860","inReplyTo":"20230614225116.GB1441072@coredump.intra.peff.net","subject":"Re: [PATCH v2 2/2] credential: erase all matching credentials","fromName":"M Hickford","fromEmail":"mirth.hickford@gmail.com","sentAt":"2023-06-15T04:57:09Z","receivedAt":"2023-06-15T04:58:04Z","isPatch":true,"sender":{"key":"mirth.hickford@gmail.com","avatar":"https://avatars.githubusercontent.com/u/105314?v=4"},"body":"On Wed, 14 Jun 2023 at 23:51, Jeff King wrote:\n>\n> On Wed, Jun 14, 2023 at 09:40:38PM +0000, M Hickford via GitGitGadget wrote:\n>\n> > From: M Hickford \n> >\n> > `credential reject` sends the erase action to each helper, but the\n> > exact behaviour of erase isn't specified in documentation or tests.\n> > Some helpers (such as credential-libsecret) delete all matching\n> > credentials, others (such as credential-cache and credential-store)\n> > delete at most one matching credential.\n> >\n> > Test that helpers erase all matching credentials. This behaviour is\n> > easiest to reason about. Users expect that `echo\n> > \"url=https://example.com\" | git credential reject` or `echo\n> > \"url=https://example.com\\nusername=tim\" | git credential reject` erase\n> > all matching credentials.\n> >\n> > Fix credential-cache and credential-store.\n>\n> I think this was how it was intended to work all along, but I agree that\n> credential-cache does not behave this way. I think credential-store\n> already works, though.\n>\n> > diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt\n> > index 0e6d9e85ec7..04bfb918de6 100644\n> > --- a/Documentation/git-credential.txt\n> > +++ b/Documentation/git-credential.txt\n> > @@ -38,8 +38,8 @@ to any configured credential helpers, which may store the credential\n> > for later use.\n> >\n> > If the action is `reject`, git-credential will send the description to\n> > -any configured credential helpers, which may erase any stored\n> > -credential matching the description.\n> > +any configured credential helpers, which may erase stored credentials\n> > +matching the description.\n>\n> I read the existing documentation as claiming to delete all such\n> matching credentials, but I guess it is ambiguous (and I did not look,\n> but it is almost a certainty that I wrote the original ;) ).\n>\n> Saying \"credentials\" in the plural makes it more clear we are getting\n> all of them. But losing \"any\" makes it less clear to me. Maybe:\n>\n> ...which may erase any stored credentials matching the description.\n>\n> is the best of both?\n\nWill update in patch v3.\n\n>\n> > diff --git a/Documentation/gitcredentials.txt b/Documentation/gitcredentials.txt\n> > index 100f045bb1a..65d652dc40e 100644\n> > --- a/Documentation/gitcredentials.txt\n> > +++ b/Documentation/gitcredentials.txt\n> > @@ -260,7 +260,7 @@ appended to its command line, which is one of:\n> >\n> > `erase`::\n> >\n> > - Remove a matching credential, if any, from the helper's storage.\n> > + Remove matching credentials, if any, from the helper's storage.\n>\n> This one seems like a strict improvement in ambiguity.\n>\n> > diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c\n> > index 82f376d3351..5e3a766e42d 100644\n> > --- a/builtin/credential-cache--daemon.c\n> > +++ b/builtin/credential-cache--daemon.c\n> > [...]\n> > @@ -48,9 +48,12 @@ static void remove_credential(const struct credential *c)\n> > {\n> > struct credential_cache_entry *e;\n> >\n> > - e = lookup_credential(c, c->password != NULL);\n> > - if (e)\n> > - e->expiration = 0;\n> > + int i;\n> > + for (i = 0; i < entries_nr; i++) {\n> > + e = &entries[i];\n> > + if (credential_match(c, &e->item, c->password != NULL))\n> > + e->expiration = 0;\n> > + }\n> > }\n>\n> OK, so now instead of looking up a single credential, we loop and cover\n> them all. That makes perfect sense. It is a little funny that we undo\n> the extra parameter to lookup_credential() that was added in the\n> previous commit. I wonder if reversing the order of the patches would\n> make it simpler, but it is probably not a big deal either way.\n>\n> > diff --git a/builtin/credential-store.c b/builtin/credential-store.c\n> > index e0ae028b1c3..85b147e460f 100644\n> > --- a/builtin/credential-store.c\n> > +++ b/builtin/credential-store.c\n> > @@ -36,7 +36,8 @@ static int parse_credential_file(const char *fn,\n> > found_credential = 1;\n> > if (match_cb) {\n> > match_cb(&entry);\n> > - break;\n> > + if (strcmp(op, \"erase\"))\n> > + break;\n> > }\n> > }\n> > else if (other_cb)\n>\n> I think this hunk does nothing. When the op is \"erase\", we do not pass a\n> match_cb at all, so this break would never trigger (and indeed, it would\n> be disastrous if it did, because we would stop copying entries from the\n> old file to the new, losing everything after the matched entry).\n>\n> And I think if you revert this, your new test still passes.\n\nYou're right. I'll revert in patch v3.\n\n>\n> > @@ -298,6 +300,37 @@ helper_test() {\n> > EOF\n> > '\n> >\n> > + test_expect_success \"helper ($HELPER) erases all matching credentials\" '\n> > + check approve $HELPER <<-\\EOF &&\n> > + protocol=https\n> > + host=example.com\n> > + username=user6\n> > + password=pass1\n> > + EOF\n> > + check approve $HELPER <<-\\EOF &&\n> > + protocol=https\n> > + host=example.com\n> > + username=user7\n> > + password=pass1\n> > + EOF\n> > + check reject $HELPER <<-\\EOF &&\n> > + protocol=https\n> > + host=example.com\n> > + EOF\n> > + check fill $HELPER <<-\\EOF\n> > + protocol=https\n> > + host=example.com\n> > + --\n> > + protocol=https\n> > + host=example.com\n> > + username=askpass-username\n> > + password=askpass-password\n> > + --\n> > + askpass: Username for '\\''https://example.com'\\'':\n> > + askpass: Password for '\\''https://askpass-username@example.com'\\'':\n> > + EOF\n> > + '\n>\n> The test makes sense. We add two, delete them both, and then check that\n> nothing is left to serve.\n>\n> -Peff\n"},{"id":"478426","messageId":"pull.1525.v3.git.git.1686809004.gitgitgadget@gmail.com","threadId":"59860","inReplyTo":"pull.1525.v2.git.git.1686778838.gitgitgadget@gmail.com","subject":"[PATCH v3 0/2] credential: improvements to erase in helpers","fromName":"M Hickford via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-06-15T06:03:22Z","receivedAt":"2023-06-15T06:04:41Z","isPatch":true,"sender":{"key":"mirth.hickford@gmail.com","avatar":"https://avatars.githubusercontent.com/u/105314?v=4"},"body":"M Hickford (2):\n credential: avoid erasing distinct password\n credential: erase all matching credentials\n\n Documentation/git-credential.txt | 2 +-\n Documentation/gitcredentials.txt | 2 +-\n builtin/credential-cache--daemon.c | 17 +++--\n builtin/credential-store.c | 18 ++---\n credential.c | 11 +--\n credential.h | 2 +-\n t/lib-credential.sh | 103 +++++++++++++++++++++++++++++\n 7 files changed, 131 insertions(+), 24 deletions(-)\n\n\nbase-commit: d7d8841f67f29e6ecbad85a11805c907d0f00d5d\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1525%2Fhickford%2Ferase-test-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1525/hickford/erase-test-v3\nPull-Request: https://github.com/git/git/pull/1525\n\nRange-diff vs v2:\n\n 1: 35ee1795bcd ! 1: df3c8a15bf8 credential: avoid erasing distinct password\n @@ builtin/credential-cache--daemon.c: static void cache_credential(struct credenti\n \t\t\treturn &entries[i];\n \t}\n \treturn NULL;\n -@@ builtin/credential-cache--daemon.c: static void remove_credential(const struct credential *c)\n + }\n + \n +-static void remove_credential(const struct credential *c)\n ++static void remove_credential(const struct credential *c, int match_password)\n {\n \tstruct credential_cache_entry *e;\n \n -\te = lookup_credential(c);\n -+\te = lookup_credential(c, c->password != NULL);\n ++\te = lookup_credential(c, match_password);\n \tif (e)\n \t\te->expiration = 0;\n }\n @@ builtin/credential-cache--daemon.c: static void serve_one_client(FILE *in, FILE\n \t\tif (e) {\n \t\t\tfprintf(out, \"username=%s\\n\", e->item.username);\n \t\t\tfprintf(out, \"password=%s\\n\", e->item.password);\n +@@ builtin/credential-cache--daemon.c: static void serve_one_client(FILE *in, FILE *out)\n + \t\texit(0);\n + \t}\n + \telse if (!strcmp(action.buf, \"erase\"))\n +-\t\tremove_credential(&c);\n ++\t\tremove_credential(&c, 1);\n + \telse if (!strcmp(action.buf, \"store\")) {\n + \t\tif (timeout < 0)\n + \t\t\twarning(\"cache client didn't specify a timeout\");\n + \t\telse if (!c.username || !c.password)\n + \t\t\twarning(\"cache client gave us a partial credential\");\n + \t\telse {\n +-\t\t\tremove_credential(&c);\n ++\t\t\tremove_credential(&c, 0);\n + \t\t\tcache_credential(&c, timeout);\n + \t\t}\n + \t}\n \n ## builtin/credential-store.c ##\n @@ builtin/credential-store.c: static struct lock_file credential_lock;\n @@ builtin/credential-store.c: static struct lock_file credential_lock;\n \t\t\t\t void (*match_cb)(struct credential *),\n -\t\t\t\t void (*other_cb)(struct strbuf *))\n +\t\t\t\t void (*other_cb)(struct strbuf *),\n -+\t\t\t\t const char* op)\n ++\t\t\t\t int match_password)\n {\n \tFILE *fh;\n \tstruct strbuf line = STRBUF_INIT;\n - \tstruct credential entry = CREDENTIAL_INIT;\n - \tint found_credential = 0;\n -+\tconst int match_password = !strcmp(op, \"erase\") && c->password != NULL;\n - \n - \tfh = fopen(fn, \"r\");\n - \tif (!fh) {\n @@ builtin/credential-store.c: static int parse_credential_file(const char *fn,\n \n \twhile (strbuf_getline_lf(&line, fh) != EOF) {\n @@ builtin/credential-store.c: static void print_line(struct strbuf *buf)\n \n static void rewrite_credential_file(const char *fn, struct credential *c,\n -\t\t\t\t struct strbuf *extra)\n -+\t\t\t\t struct strbuf *extra, const char *op)\n ++\t\t\t\t struct strbuf *extra, int match_password)\n {\n \tint timeout_ms = 1000;\n \n @@ builtin/credential-store.c: static void rewrite_credential_file(const char *fn,\n \tif (extra)\n \t\tprint_line(extra);\n -\tparse_credential_file(fn, c, NULL, print_line);\n -+\tparse_credential_file(fn, c, NULL, print_line, op);\n ++\tparse_credential_file(fn, c, NULL, print_line, match_password);\n \tif (commit_lock_file(&credential_lock) < 0)\n \t\tdie_errno(\"unable to write credential store\");\n }\n @@ builtin/credential-store.c: static void store_credential_file(const char *fn, st\n \t}\n \n -\trewrite_credential_file(fn, c, &buf);\n -+\trewrite_credential_file(fn, c, &buf, \"store\");\n ++\trewrite_credential_file(fn, c, &buf, 0);\n \tstrbuf_release(&buf);\n }\n \n @@ builtin/credential-store.c: static void remove_credential(const struct string_li\n \tfor_each_string_list_item(fn, fns)\n \t\tif (!access(fn->string, F_OK))\n -\t\t\trewrite_credential_file(fn->string, c, NULL);\n -+\t\t\trewrite_credential_file(fn->string, c, NULL, \"erase\");\n ++\t\t\trewrite_credential_file(fn->string, c, NULL, 1);\n }\n \n static void lookup_credential(const struct string_list *fns, struct credential *c)\n @@ builtin/credential-store.c: static void lookup_credential(const struct string_li\n \n \tfor_each_string_list_item(fn, fns)\n -\t\tif (parse_credential_file(fn->string, c, print_entry, NULL))\n -+\t\tif (parse_credential_file(fn->string, c, print_entry, NULL, \"get\"))\n ++\t\tif (parse_credential_file(fn->string, c, print_entry, NULL, 0))\n \t\t\treturn; /* Found credential */\n }\n \n @@ t/lib-credential.sh: helper_test_clean() {\n \treject $1 https example.com user2\n \treject $1 https example.com user4\n +\treject $1 https example.com user5\n ++\treject $1 https example.com user8\n \treject $1 http path.tld user\n \treject $1 https timeout.tld user\n \treject $1 https sso.tld\n @@ t/lib-credential.sh: helper_test() {\n \t\tEOF\n \t'\n \n ++\ttest_expect_success \"helper ($HELPER) overwrites on store\" '\n ++\t\tcheck approve $HELPER <<-\\EOF &&\n ++\t\tprotocol=https\n ++\t\thost=example.com\n ++\t\tusername=user8\n ++\t\tpassword=pass1\n ++\t\tEOF\n ++\t\tcheck approve $HELPER <<-\\EOF &&\n ++\t\tprotocol=https\n ++\t\thost=example.com\n ++\t\tusername=user8\n ++\t\tpassword=pass2\n ++\t\tEOF\n ++\t\tcheck fill $HELPER <<-\\EOF &&\n ++\t\tprotocol=https\n ++\t\thost=example.com\n ++\t\tusername=user8\n ++\t\t--\n ++\t\tprotocol=https\n ++\t\thost=example.com\n ++\t\tusername=user8\n ++\t\tpassword=pass2\n ++\t\tEOF\n ++\t\tcheck reject $HELPER <<-\\EOF &&\n ++\t\tprotocol=https\n ++\t\thost=example.com\n ++\t\tusername=user8\n ++\t\tpassword=pass2\n ++\t\tEOF\n ++\t\tcheck fill $HELPER <<-\\EOF\n ++\t\tprotocol=https\n ++\t\thost=example.com\n ++\t\tusername=user8\n ++\t\t--\n ++\t\tprotocol=https\n ++\t\thost=example.com\n ++\t\tusername=user8\n ++\t\tpassword=askpass-password\n ++\t\t--\n ++\t\taskpass: Password for '\\''https://user8@example.com'\\'':\n ++\t\tEOF\n ++\t'\n ++\n + \ttest_expect_success \"helper ($HELPER) can forget host\" '\n + \t\tcheck reject $HELPER <<-\\EOF &&\n + \t\tprotocol=https\n +@@ t/lib-credential.sh: helper_test() {\n + \t\tEOF\n + \t'\n + \n +\ttest_expect_success \"helper ($HELPER) does not erase a password distinct from input\" '\n +\t\tcheck approve $HELPER <<-\\EOF &&\n +\t\tprotocol=https\n 2: 9b12f17dc7e ! 2: e06d80e99a0 credential: erase all matching credentials\n @@ Commit message\n Signed-off-by: M Hickford \n \n ## Documentation/git-credential.txt ##\n -@@ Documentation/git-credential.txt: to any configured credential helpers, which may store the credential\n - for later use.\n +@@ Documentation/git-credential.txt: for later use.\n \n If the action is `reject`, git-credential will send the description to\n --any configured credential helpers, which may erase any stored\n + any configured credential helpers, which may erase any stored\n -credential matching the description.\n -+any configured credential helpers, which may erase stored credentials\n -+matching the description.\n ++credentials matching the description.\n \n If the action is `approve` or `reject`, no output should be emitted.\n \n @@ builtin/credential-cache--daemon.c: static void cache_credential(struct credenti\n \t\t\treturn &entries[i];\n \t}\n \treturn NULL;\n -@@ builtin/credential-cache--daemon.c: static void remove_credential(const struct credential *c)\n +@@ builtin/credential-cache--daemon.c: static void remove_credential(const struct credential *c, int match_password)\n {\n \tstruct credential_cache_entry *e;\n \n --\te = lookup_credential(c, c->password != NULL);\n +-\te = lookup_credential(c, match_password);\n -\tif (e)\n -\t\te->expiration = 0;\n +\tint i;\n +\tfor (i = 0; i < entries_nr; i++) {\n +\t\te = &entries[i];\n -+\t\tif (credential_match(c, &e->item, c->password != NULL))\n ++\t\tif (credential_match(c, &e->item, match_password))\n +\t\t\te->expiration = 0;\n +\t}\n }\n @@ builtin/credential-store.c: static int parse_credential_file(const char *fn,\n \t\t\tif (match_cb) {\n \t\t\t\tmatch_cb(&entry);\n -\t\t\t\tbreak;\n -+\t\t\t\tif (strcmp(op, \"erase\"))\n -+\t\t\t\t\tbreak;\n \t\t\t}\n \t\t}\n \t\telse if (other_cb)\n @@ t/lib-credential.sh: helper_test_clean() {\n \treject $1 https example.com user5\n +\treject $1 https example.com user6\n +\treject $1 https example.com user7\n + \treject $1 https example.com user8\n \treject $1 http path.tld user\n \treject $1 https timeout.tld user\n - \treject $1 https sso.tld\n @@ t/lib-credential.sh: helper_test() {\n \t\tEOF\n \t'\n\n-- \ngitgitgadget\n"},{"id":"478427","messageId":"df3c8a15bf8ef3258ed6959568cf4cfd1042f71d.1686809004.git.gitgitgadget@gmail.com","threadId":"59860","inReplyTo":"pull.1525.v3.git.git.1686809004.gitgitgadget@gmail.com","subject":"[PATCH v3 1/2] credential: avoid erasing distinct password","fromName":"M Hickford via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-06-15T06:03:23Z","receivedAt":"2023-06-15T06:05:04Z","isPatch":true,"sender":{"key":"mirth.hickford@gmail.com","avatar":"https://avatars.githubusercontent.com/u/105314?v=4"},"body":"From: M Hickford \n\nTest that credential helpers do not erase a password distinct from the\ninput. Such calls can happen when multiple credential helpers are\nconfigured.\n\nFixes for credential-cache and credential-store.\n\nSigned-off-by: M Hickford \n---\n builtin/credential-cache--daemon.c | 14 +++---\n builtin/credential-store.c | 17 ++++----\n credential.c | 11 ++---\n credential.h | 2 +-\n t/lib-credential.sh | 70 ++++++++++++++++++++++++++++++\n 5 files changed, 93 insertions(+), 21 deletions(-)\n\ndiff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c\nindex 756c5f02aef..f64dd21d335 100644\n--- a/builtin/credential-cache--daemon.c\n+++ b/builtin/credential-cache--daemon.c\n@@ -33,22 +33,22 @@ static void cache_credential(struct credential *c, int timeout)\n \te->expiration = time(NULL) + timeout;\n }\n \n-static struct credential_cache_entry *lookup_credential(const struct credential *c)\n+static struct credential_cache_entry *lookup_credential(const struct credential *c, int match_password)\n {\n \tint i;\n \tfor (i = 0; i < entries_nr; i++) {\n \t\tstruct credential *e = &entries[i].item;\n-\t\tif (credential_match(c, e))\n+\t\tif (credential_match(c, e, match_password))\n \t\t\treturn &entries[i];\n \t}\n \treturn NULL;\n }\n \n-static void remove_credential(const struct credential *c)\n+static void remove_credential(const struct credential *c, int match_password)\n {\n \tstruct credential_cache_entry *e;\n \n-\te = lookup_credential(c);\n+\te = lookup_credential(c, match_password);\n \tif (e)\n \t\te->expiration = 0;\n }\n@@ -127,7 +127,7 @@ static void serve_one_client(FILE *in, FILE *out)\n \tif (read_request(in, &c, &action, &timeout) < 0)\n \t\t/* ignore error */ ;\n \telse if (!strcmp(action.buf, \"get\")) {\n-\t\tstruct credential_cache_entry *e = lookup_credential(&c);\n+\t\tstruct credential_cache_entry *e = lookup_credential(&c, 0);\n \t\tif (e) {\n \t\t\tfprintf(out, \"username=%s\\n\", e->item.username);\n \t\t\tfprintf(out, \"password=%s\\n\", e->item.password);\n@@ -151,14 +151,14 @@ static void serve_one_client(FILE *in, FILE *out)\n \t\texit(0);\n \t}\n \telse if (!strcmp(action.buf, \"erase\"))\n-\t\tremove_credential(&c);\n+\t\tremove_credential(&c, 1);\n \telse if (!strcmp(action.buf, \"store\")) {\n \t\tif (timeout < 0)\n \t\t\twarning(\"cache client didn't specify a timeout\");\n \t\telse if (!c.username || !c.password)\n \t\t\twarning(\"cache client gave us a partial credential\");\n \t\telse {\n-\t\t\tremove_credential(&c);\n+\t\t\tremove_credential(&c, 0);\n \t\t\tcache_credential(&c, timeout);\n \t\t}\n \t}\ndiff --git a/builtin/credential-store.c b/builtin/credential-store.c\nindex 30c6ccf56c0..f80ff59f18a 100644\n--- a/builtin/credential-store.c\n+++ b/builtin/credential-store.c\n@@ -13,7 +13,8 @@ static struct lock_file credential_lock;\n static int parse_credential_file(const char *fn,\n \t\t\t\t struct credential *c,\n \t\t\t\t void (*match_cb)(struct credential *),\n-\t\t\t\t void (*other_cb)(struct strbuf *))\n+\t\t\t\t void (*other_cb)(struct strbuf *),\n+\t\t\t\t int match_password)\n {\n \tFILE *fh;\n \tstruct strbuf line = STRBUF_INIT;\n@@ -29,8 +30,8 @@ static int parse_credential_file(const char *fn,\n \n \twhile (strbuf_getline_lf(&line, fh) != EOF) {\n \t\tif (!credential_from_url_gently(&entry, line.buf, 1) &&\n-\t\t entry.username && entry.password &&\n-\t\t credential_match(c, &entry)) {\n+\t\t\tentry.username && entry.password &&\n+\t\t\tcredential_match(c, &entry, match_password)) {\n \t\t\tfound_credential = 1;\n \t\t\tif (match_cb) {\n \t\t\t\tmatch_cb(&entry);\n@@ -60,7 +61,7 @@ static void print_line(struct strbuf *buf)\n }\n \n static void rewrite_credential_file(const char *fn, struct credential *c,\n-\t\t\t\t struct strbuf *extra)\n+\t\t\t\t struct strbuf *extra, int match_password)\n {\n \tint timeout_ms = 1000;\n \n@@ -69,7 +70,7 @@ static void rewrite_credential_file(const char *fn, struct credential *c,\n \t\tdie_errno(_(\"unable to get credential storage lock in %d ms\"), timeout_ms);\n \tif (extra)\n \t\tprint_line(extra);\n-\tparse_credential_file(fn, c, NULL, print_line);\n+\tparse_credential_file(fn, c, NULL, print_line, match_password);\n \tif (commit_lock_file(&credential_lock) < 0)\n \t\tdie_errno(\"unable to write credential store\");\n }\n@@ -91,7 +92,7 @@ static void store_credential_file(const char *fn, struct credential *c)\n \t\t\t\t\tis_rfc3986_reserved_or_unreserved);\n \t}\n \n-\trewrite_credential_file(fn, c, &buf);\n+\trewrite_credential_file(fn, c, &buf, 0);\n \tstrbuf_release(&buf);\n }\n \n@@ -138,7 +139,7 @@ static void remove_credential(const struct string_list *fns, struct credential *\n \t\treturn;\n \tfor_each_string_list_item(fn, fns)\n \t\tif (!access(fn->string, F_OK))\n-\t\t\trewrite_credential_file(fn->string, c, NULL);\n+\t\t\trewrite_credential_file(fn->string, c, NULL, 1);\n }\n \n static void lookup_credential(const struct string_list *fns, struct credential *c)\n@@ -146,7 +147,7 @@ static void lookup_credential(const struct string_list *fns, struct credential *\n \tstruct string_list_item *fn;\n \n \tfor_each_string_list_item(fn, fns)\n-\t\tif (parse_credential_file(fn->string, c, print_entry, NULL))\n+\t\tif (parse_credential_file(fn->string, c, print_entry, NULL, 0))\n \t\t\treturn; /* Found credential */\n }\n \ndiff --git a/credential.c b/credential.c\nindex 023b59d5711..9157ff0865e 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -33,13 +33,14 @@ void credential_clear(struct credential *c)\n }\n \n int credential_match(const struct credential *want,\n-\t\t const struct credential *have)\n+\t\t const struct credential *have, int match_password)\n {\n #define CHECK(x) (!want->x || (have->x && !strcmp(want->x, have->x)))\n \treturn CHECK(protocol) &&\n-\t CHECK(host) &&\n-\t CHECK(path) &&\n-\t CHECK(username);\n+\t\tCHECK(host) &&\n+\t\tCHECK(path) &&\n+\t\tCHECK(username) &&\n+\t\t(!match_password || CHECK(password));\n #undef CHECK\n }\n \n@@ -102,7 +103,7 @@ static int match_partial_url(const char *url, void *cb)\n \t\twarning(_(\"skipping credential lookup for key: credential.%s\"),\n \t\t\turl);\n \telse\n-\t\tmatches = credential_match(&want, c);\n+\t\tmatches = credential_match(&want, c, 0);\n \tcredential_clear(&want);\n \n \treturn matches;\ndiff --git a/credential.h b/credential.h\nindex b8e2936d1dc..acc41adf548 100644\n--- a/credential.h\n+++ b/credential.h\n@@ -211,6 +211,6 @@ void credential_from_url(struct credential *, const char *url);\n int credential_from_url_gently(struct credential *, const char *url, int quiet);\n \n int credential_match(const struct credential *want,\n-\t\t const struct credential *have);\n+\t\t const struct credential *have, int match_password);\n \n #endif /* CREDENTIAL_H */\ndiff --git a/t/lib-credential.sh b/t/lib-credential.sh\nindex f1ab92ba35c..7b4fdd011d7 100644\n--- a/t/lib-credential.sh\n+++ b/t/lib-credential.sh\n@@ -44,6 +44,8 @@ helper_test_clean() {\n \treject $1 https example.com user1\n \treject $1 https example.com user2\n \treject $1 https example.com user4\n+\treject $1 https example.com user5\n+\treject $1 https example.com user8\n \treject $1 http path.tld user\n \treject $1 https timeout.tld user\n \treject $1 https sso.tld\n@@ -167,6 +169,49 @@ helper_test() {\n \t\tEOF\n \t'\n \n+\ttest_expect_success \"helper ($HELPER) overwrites on store\" '\n+\t\tcheck approve $HELPER <<-\\EOF &&\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\tusername=user8\n+\t\tpassword=pass1\n+\t\tEOF\n+\t\tcheck approve $HELPER <<-\\EOF &&\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\tusername=user8\n+\t\tpassword=pass2\n+\t\tEOF\n+\t\tcheck fill $HELPER <<-\\EOF &&\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\tusername=user8\n+\t\t--\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\tusername=user8\n+\t\tpassword=pass2\n+\t\tEOF\n+\t\tcheck reject $HELPER <<-\\EOF &&\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\tusername=user8\n+\t\tpassword=pass2\n+\t\tEOF\n+\t\tcheck fill $HELPER <<-\\EOF\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\tusername=user8\n+\t\t--\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\tusername=user8\n+\t\tpassword=askpass-password\n+\t\t--\n+\t\taskpass: Password for '\\''https://user8@example.com'\\'':\n+\t\tEOF\n+\t'\n+\n \ttest_expect_success \"helper ($HELPER) can forget host\" '\n \t\tcheck reject $HELPER <<-\\EOF &&\n \t\tprotocol=https\n@@ -221,6 +266,31 @@ helper_test() {\n \t\tEOF\n \t'\n \n+\ttest_expect_success \"helper ($HELPER) does not erase a password distinct from input\" '\n+\t\tcheck approve $HELPER <<-\\EOF &&\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\tusername=user5\n+\t\tpassword=pass1\n+\t\tEOF\n+\t\tcheck reject $HELPER <<-\\EOF &&\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\tusername=user5\n+\t\tpassword=pass2\n+\t\tEOF\n+\t\tcheck fill $HELPER <<-\\EOF\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\tusername=user5\n+\t\t--\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\tusername=user5\n+\t\tpassword=pass1\n+\t\tEOF\n+\t'\n+\n \ttest_expect_success \"helper ($HELPER) can forget user\" '\n \t\tcheck reject $HELPER <<-\\EOF &&\n \t\tprotocol=https\n-- \ngitgitgadget\n\n"},{"id":"478428","messageId":"e06d80e99a00c30fb8430afcd457576bb0bf9ce5.1686809004.git.gitgitgadget@gmail.com","threadId":"59860","inReplyTo":"pull.1525.v3.git.git.1686809004.gitgitgadget@gmail.com","subject":"[PATCH v3 2/2] credential: erase all matching credentials","fromName":"M Hickford via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-06-15T06:03:24Z","receivedAt":"2023-06-15T06:05:04Z","isPatch":true,"sender":{"key":"mirth.hickford@gmail.com","avatar":"https://avatars.githubusercontent.com/u/105314?v=4"},"body":"From: M Hickford \n\n`credential reject` sends the erase action to each helper, but the\nexact behaviour of erase isn't specified in documentation or tests.\nSome helpers (such as credential-libsecret) delete all matching\ncredentials, others (such as credential-cache and credential-store)\ndelete at most one matching credential.\n\nTest that helpers erase all matching credentials. This behaviour is\neasiest to reason about. Users expect that `echo\n\"url=https://example.com\" | git credential reject` or `echo\n\"url=https://example.com\\nusername=tim\" | git credential reject` erase\nall matching credentials.\n\nFix credential-cache and credential-store.\n\nSigned-off-by: M Hickford \n---\n Documentation/git-credential.txt | 2 +-\n Documentation/gitcredentials.txt | 2 +-\n builtin/credential-cache--daemon.c | 15 ++++++++------\n builtin/credential-store.c | 1 -\n t/lib-credential.sh | 33 ++++++++++++++++++++++++++++++\n 5 files changed, 44 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt\nindex 0e6d9e85ec7..a220afed4f3 100644\n--- a/Documentation/git-credential.txt\n+++ b/Documentation/git-credential.txt\n@@ -39,7 +39,7 @@ for later use.\n \n If the action is `reject`, git-credential will send the description to\n any configured credential helpers, which may erase any stored\n-credential matching the description.\n+credentials matching the description.\n \n If the action is `approve` or `reject`, no output should be emitted.\n \ndiff --git a/Documentation/gitcredentials.txt b/Documentation/gitcredentials.txt\nindex 100f045bb1a..65d652dc40e 100644\n--- a/Documentation/gitcredentials.txt\n+++ b/Documentation/gitcredentials.txt\n@@ -260,7 +260,7 @@ appended to its command line, which is one of:\n \n `erase`::\n \n-\tRemove a matching credential, if any, from the helper's storage.\n+\tRemove matching credentials, if any, from the helper's storage.\n \n The details of the credential will be provided on the helper's stdin\n stream. The exact format is the same as the input/output format of the\ndiff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c\nindex f64dd21d335..dc1cf2d25fc 100644\n--- a/builtin/credential-cache--daemon.c\n+++ b/builtin/credential-cache--daemon.c\n@@ -33,12 +33,12 @@ static void cache_credential(struct credential *c, int timeout)\n \te->expiration = time(NULL) + timeout;\n }\n \n-static struct credential_cache_entry *lookup_credential(const struct credential *c, int match_password)\n+static struct credential_cache_entry *lookup_credential(const struct credential *c)\n {\n \tint i;\n \tfor (i = 0; i < entries_nr; i++) {\n \t\tstruct credential *e = &entries[i].item;\n-\t\tif (credential_match(c, e, match_password))\n+\t\tif (credential_match(c, e, 0))\n \t\t\treturn &entries[i];\n \t}\n \treturn NULL;\n@@ -48,9 +48,12 @@ static void remove_credential(const struct credential *c, int match_password)\n {\n \tstruct credential_cache_entry *e;\n \n-\te = lookup_credential(c, match_password);\n-\tif (e)\n-\t\te->expiration = 0;\n+\tint i;\n+\tfor (i = 0; i < entries_nr; i++) {\n+\t\te = &entries[i];\n+\t\tif (credential_match(c, &e->item, match_password))\n+\t\t\te->expiration = 0;\n+\t}\n }\n \n static timestamp_t check_expirations(void)\n@@ -127,7 +130,7 @@ static void serve_one_client(FILE *in, FILE *out)\n \tif (read_request(in, &c, &action, &timeout) < 0)\n \t\t/* ignore error */ ;\n \telse if (!strcmp(action.buf, \"get\")) {\n-\t\tstruct credential_cache_entry *e = lookup_credential(&c, 0);\n+\t\tstruct credential_cache_entry *e = lookup_credential(&c);\n \t\tif (e) {\n \t\t\tfprintf(out, \"username=%s\\n\", e->item.username);\n \t\t\tfprintf(out, \"password=%s\\n\", e->item.password);\ndiff --git a/builtin/credential-store.c b/builtin/credential-store.c\nindex f80ff59f18a..06bbfa4dd03 100644\n--- a/builtin/credential-store.c\n+++ b/builtin/credential-store.c\n@@ -35,7 +35,6 @@ static int parse_credential_file(const char *fn,\n \t\t\tfound_credential = 1;\n \t\t\tif (match_cb) {\n \t\t\t\tmatch_cb(&entry);\n-\t\t\t\tbreak;\n \t\t\t}\n \t\t}\n \t\telse if (other_cb)\ndiff --git a/t/lib-credential.sh b/t/lib-credential.sh\nindex 7b4fdd011d7..3f21ba66929 100644\n--- a/t/lib-credential.sh\n+++ b/t/lib-credential.sh\n@@ -45,6 +45,8 @@ helper_test_clean() {\n \treject $1 https example.com user2\n \treject $1 https example.com user4\n \treject $1 https example.com user5\n+\treject $1 https example.com user6\n+\treject $1 https example.com user7\n \treject $1 https example.com user8\n \treject $1 http path.tld user\n \treject $1 https timeout.tld user\n@@ -342,6 +344,37 @@ helper_test() {\n \t\tEOF\n \t'\n \n+\ttest_expect_success \"helper ($HELPER) erases all matching credentials\" '\n+\t\tcheck approve $HELPER <<-\\EOF &&\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\tusername=user6\n+\t\tpassword=pass1\n+\t\tEOF\n+\t\tcheck approve $HELPER <<-\\EOF &&\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\tusername=user7\n+\t\tpassword=pass1\n+\t\tEOF\n+\t\tcheck reject $HELPER <<-\\EOF &&\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\tEOF\n+\t\tcheck fill $HELPER <<-\\EOF\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\t--\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\tusername=askpass-username\n+\t\tpassword=askpass-password\n+\t\t--\n+\t\taskpass: Username for '\\''https://example.com'\\'':\n+\t\taskpass: Password for '\\''https://askpass-username@example.com'\\'':\n+\t\tEOF\n+\t'\n+\n \t: ${GIT_TEST_LONG_CRED_BUFFER:=1024}\n \t# 23 bytes accounts for \"wwwauth[]=basic realm=\" plus NUL\n \tLONG_VALUE_LEN=$((GIT_TEST_LONG_CRED_BUFFER - 23))\n-- \ngitgitgadget\n"},{"id":"478429","messageId":"20230615070809.GA1460739@coredump.intra.peff.net","threadId":"59860","inReplyTo":"df3c8a15bf8ef3258ed6959568cf4cfd1042f71d.1686809004.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 1/2] credential: avoid erasing distinct password","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-06-15T07:08:09Z","receivedAt":"2023-06-15T07:11:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 15, 2023 at 06:03:23AM +0000, M Hickford via GitGitGadget wrote:\n\n> @@ -151,14 +151,14 @@ static void serve_one_client(FILE *in, FILE *out)\n> \t\texit(0);\n> \t}\n> \telse if (!strcmp(action.buf, \"erase\"))\n> -\t\tremove_credential(&c);\n> +\t\tremove_credential(&c, 1);\n> \telse if (!strcmp(action.buf, \"store\")) {\n> \t\tif (timeout < 0)\n> \t\t\twarning(\"cache client didn't specify a timeout\");\n> \t\telse if (!c.username || !c.password)\n> \t\t\twarning(\"cache client gave us a partial credential\");\n> \t\telse {\n> -\t\t\tremove_credential(&c);\n> +\t\t\tremove_credential(&c, 0);\n> \t\t\tcache_credential(&c, timeout);\n> \t\t}\n> \t}\n\nGreat, this hunk fixes the overwrite problem from the previous round.\nAll of the credential-cache bits look good to me.\n\n> @@ -29,8 +30,8 @@ static int parse_credential_file(const char *fn,\n> \n> \twhile (strbuf_getline_lf(&line, fh) != EOF) {\n> \t\tif (!credential_from_url_gently(&entry, line.buf, 1) &&\n> -\t\t entry.username && entry.password &&\n> -\t\t credential_match(c, &entry)) {\n> +\t\t\tentry.username && entry.password &&\n> +\t\t\tcredential_match(c, &entry, match_password)) {\n> \t\t\tfound_credential = 1;\n> \t\t\tif (match_cb) {\n> \t\t\t\tmatch_cb(&entry);\n\nThis hunk is from credential-store. I think the code change is doing the\nright thing, but the modified indentation is wrong (we'd usually do a\n4-space indentation to line up the conditions; doing a full tab confuses\nthe conditions with the actual body).\n\n> diff --git a/credential.c b/credential.c\n> index 023b59d5711..9157ff0865e 100644\n> --- a/credential.c\n> +++ b/credential.c\n> @@ -33,13 +33,14 @@ void credential_clear(struct credential *c)\n> }\n> \n> int credential_match(const struct credential *want,\n> -\t\t const struct credential *have)\n> +\t\t const struct credential *have, int match_password)\n> {\n> #define CHECK(x) (!want->x || (have->x && !strcmp(want->x, have->x)))\n> \treturn CHECK(protocol) &&\n> -\t CHECK(host) &&\n> -\t CHECK(path) &&\n> -\t CHECK(username);\n> +\t\tCHECK(host) &&\n> +\t\tCHECK(path) &&\n> +\t\tCHECK(username) &&\n> +\t\t(!match_password || CHECK(password));\n> #undef CHECK\n> }\n\nThe indentation here seemed funny to me, too, though I think it's a\nmatter of taste (indent a tab versus line up with the seven chars of\n\"return \". I'd have a slight preference to leave it as-is, just because\nit makes the diff noisier, but I'm OK with it either way.\n\n> diff --git a/t/lib-credential.sh b/t/lib-credential.sh\n> index f1ab92ba35c..7b4fdd011d7 100644\n> --- a/t/lib-credential.sh\n> +++ b/t/lib-credential.sh\n> @@ -44,6 +44,8 @@ helper_test_clean() {\n> \treject $1 https example.com user1\n> \treject $1 https example.com user2\n> \treject $1 https example.com user4\n> +\treject $1 https example.com user5\n> +\treject $1 https example.com user8\n> \treject $1 http path.tld user\n> \treject $1 https timeout.tld user\n> \treject $1 https sso.tld\n\nThis is a funny gap to have (I realize it is because you use 6-7 in the\nnext patch, but usually we'd try to do things in order and rebase\ndownstream patches as appropriate). Doubly funny here is that the test\nfor \"8\" comes before \"5\". That's not wrong from a code/testing\nstandpoint, but it may make the script harder to read or modify later\non.\n\nMaybe it would be easier to read if we gave these users non-numeric\nnames that match how we will use them. E.g., can we call user8\nuser-overwrite or something? That makes the intention obvious, and then\nwe do not have to worry about maintaining ordering constraints.\n\n> @@ -167,6 +169,49 @@ helper_test() {\n> \t\tEOF\n> \t'\n> \n> +\ttest_expect_success \"helper ($HELPER) overwrites on store\" '\n> [...]\n\nGreat, thank you for adding a test for this case. The test itself looks\ngood to me.\n\n> @@ -221,6 +266,31 @@ helper_test() {\n> \t\tEOF\n> \t'\n> \n> +\ttest_expect_success \"helper ($HELPER) does not erase a password distinct from input\" '\n> [...]\n\nAnd this one continues to look good. I think the username could be\n\"user-distinct-pass\" or something if we wanted to go with\nhuman-understandable ones.\n\n-Peff\n"},{"id":"478430","messageId":"20230615070930.GB1460739@coredump.intra.peff.net","threadId":"59860","inReplyTo":"e06d80e99a00c30fb8430afcd457576bb0bf9ce5.1686809004.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 2/2] credential: erase all matching credentials","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-06-15T07:09:30Z","receivedAt":"2023-06-15T07:11:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 15, 2023 at 06:03:24AM +0000, M Hickford via GitGitGadget wrote:\n\n> From: M Hickford \n> \n> `credential reject` sends the erase action to each helper, but the\n> exact behaviour of erase isn't specified in documentation or tests.\n> Some helpers (such as credential-libsecret) delete all matching\n> credentials, others (such as credential-cache and credential-store)\n> delete at most one matching credential.\n> \n> Test that helpers erase all matching credentials. This behaviour is\n> easiest to reason about. Users expect that `echo\n> \"url=https://example.com\" | git credential reject` or `echo\n> \"url=https://example.com\\nusername=tim\" | git credential reject` erase\n> all matching credentials.\n> \n> Fix credential-cache and credential-store.\n\nThis last sentence is out of date now, right? We are only fixing\ncredential-cache.\n\n> diff --git a/builtin/credential-store.c b/builtin/credential-store.c\n> index f80ff59f18a..06bbfa4dd03 100644\n> --- a/builtin/credential-store.c\n> +++ b/builtin/credential-store.c\n> @@ -35,7 +35,6 @@ static int parse_credential_file(const char *fn,\n> \t\t\tfound_credential = 1;\n> \t\t\tif (match_cb) {\n> \t\t\t\tmatch_cb(&entry);\n> -\t\t\t\tbreak;\n> \t\t\t}\n> \t\t}\n> \t\telse if (other_cb)\n\nThis hunk is wrong, isn't it? We should not be touching credential-store\nat all (and losing the \"break\" means that we'd accidentally print\nmultiple matching entries for \"get\").\n\n-Peff\n"},{"id":"478449","messageId":"pull.1525.v4.git.git.1686856773.gitgitgadget@gmail.com","threadId":"59860","inReplyTo":"pull.1525.v3.git.git.1686809004.gitgitgadget@gmail.com","subject":"[PATCH v4 0/2] credential: improvements to erase in helpers","fromName":"M Hickford via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-06-15T19:19:31Z","receivedAt":"2023-06-15T19:19:41Z","isPatch":true,"sender":{"key":"mirth.hickford@gmail.com","avatar":"https://avatars.githubusercontent.com/u/105314?v=4"},"body":"M Hickford (2):\n credential: avoid erasing distinct password\n credential: erase all matching credentials\n\n Documentation/git-credential.txt | 2 +-\n Documentation/gitcredentials.txt | 2 +-\n builtin/credential-cache--daemon.c | 17 +++--\n builtin/credential-store.c | 15 +++--\n credential.c | 7 +-\n credential.h | 2 +-\n t/lib-credential.sh | 103 +++++++++++++++++++++++++++++\n 7 files changed, 128 insertions(+), 20 deletions(-)\n\n\nbase-commit: d7d8841f67f29e6ecbad85a11805c907d0f00d5d\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1525%2Fhickford%2Ferase-test-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1525/hickford/erase-test-v4\nPull-Request: https://github.com/git/git/pull/1525\n\nRange-diff vs v3:\n\n 1: df3c8a15bf8 ! 1: 91d4b04b5e1 credential: avoid erasing distinct password\n @@ builtin/credential-store.c: static struct lock_file credential_lock;\n \tFILE *fh;\n \tstruct strbuf line = STRBUF_INIT;\n @@ builtin/credential-store.c: static int parse_credential_file(const char *fn,\n - \n \twhile (strbuf_getline_lf(&line, fh) != EOF) {\n \t\tif (!credential_from_url_gently(&entry, line.buf, 1) &&\n --\t\t entry.username && entry.password &&\n + \t\t entry.username && entry.password &&\n -\t\t credential_match(c, &entry)) {\n -+\t\t\tentry.username && entry.password &&\n -+\t\t\tcredential_match(c, &entry, match_password)) {\n ++\t\t credential_match(c, &entry, match_password)) {\n \t\t\tfound_credential = 1;\n \t\t\tif (match_cb) {\n \t\t\t\tmatch_cb(&entry);\n @@ credential.c: void credential_clear(struct credential *c)\n {\n #define CHECK(x) (!want->x || (have->x && !strcmp(want->x, have->x)))\n \treturn CHECK(protocol) &&\n --\t CHECK(host) &&\n --\t CHECK(path) &&\n + \t CHECK(host) &&\n + \t CHECK(path) &&\n -\t CHECK(username);\n -+\t\tCHECK(host) &&\n -+\t\tCHECK(path) &&\n -+\t\tCHECK(username) &&\n -+\t\t(!match_password || CHECK(password));\n ++\t CHECK(username) &&\n ++\t (!match_password || CHECK(password));\n #undef CHECK\n }\n \n @@ t/lib-credential.sh: helper_test_clean() {\n \treject $1 https example.com user1\n \treject $1 https example.com user2\n \treject $1 https example.com user4\n -+\treject $1 https example.com user5\n -+\treject $1 https example.com user8\n ++\treject $1 https example.com user-distinct-pass\n ++\treject $1 https example.com user-overwrite\n \treject $1 http path.tld user\n \treject $1 https timeout.tld user\n \treject $1 https sso.tld\n @@ t/lib-credential.sh: helper_test() {\n +\t\tcheck approve $HELPER <<-\\EOF &&\n +\t\tprotocol=https\n +\t\thost=example.com\n -+\t\tusername=user8\n ++\t\tusername=user-overwrite\n +\t\tpassword=pass1\n +\t\tEOF\n +\t\tcheck approve $HELPER <<-\\EOF &&\n +\t\tprotocol=https\n +\t\thost=example.com\n -+\t\tusername=user8\n ++\t\tusername=user-overwrite\n +\t\tpassword=pass2\n +\t\tEOF\n +\t\tcheck fill $HELPER <<-\\EOF &&\n +\t\tprotocol=https\n +\t\thost=example.com\n -+\t\tusername=user8\n ++\t\tusername=user-overwrite\n +\t\t--\n +\t\tprotocol=https\n +\t\thost=example.com\n -+\t\tusername=user8\n ++\t\tusername=user-overwrite\n +\t\tpassword=pass2\n +\t\tEOF\n +\t\tcheck reject $HELPER <<-\\EOF &&\n +\t\tprotocol=https\n +\t\thost=example.com\n -+\t\tusername=user8\n ++\t\tusername=user-overwrite\n +\t\tpassword=pass2\n +\t\tEOF\n +\t\tcheck fill $HELPER <<-\\EOF\n +\t\tprotocol=https\n +\t\thost=example.com\n -+\t\tusername=user8\n ++\t\tusername=user-overwrite\n +\t\t--\n +\t\tprotocol=https\n +\t\thost=example.com\n -+\t\tusername=user8\n ++\t\tusername=user-overwrite\n +\t\tpassword=askpass-password\n +\t\t--\n -+\t\taskpass: Password for '\\''https://user8@example.com'\\'':\n ++\t\taskpass: Password for '\\''https://user-overwrite@example.com'\\'':\n +\t\tEOF\n +\t'\n +\n @@ t/lib-credential.sh: helper_test() {\n +\t\tcheck approve $HELPER <<-\\EOF &&\n +\t\tprotocol=https\n +\t\thost=example.com\n -+\t\tusername=user5\n ++\t\tusername=user-distinct-pass\n +\t\tpassword=pass1\n +\t\tEOF\n +\t\tcheck reject $HELPER <<-\\EOF &&\n +\t\tprotocol=https\n +\t\thost=example.com\n -+\t\tusername=user5\n ++\t\tusername=user-distinct-pass\n +\t\tpassword=pass2\n +\t\tEOF\n +\t\tcheck fill $HELPER <<-\\EOF\n +\t\tprotocol=https\n +\t\thost=example.com\n -+\t\tusername=user5\n ++\t\tusername=user-distinct-pass\n +\t\t--\n +\t\tprotocol=https\n +\t\thost=example.com\n -+\t\tusername=user5\n ++\t\tusername=user-distinct-pass\n +\t\tpassword=pass1\n +\t\tEOF\n +\t'\n 2: e06d80e99a0 ! 2: 42f41b28e6e credential: erase all matching credentials\n @@ Commit message\n \n `credential reject` sends the erase action to each helper, but the\n exact behaviour of erase isn't specified in documentation or tests.\n - Some helpers (such as credential-libsecret) delete all matching\n - credentials, others (such as credential-cache and credential-store)\n - delete at most one matching credential.\n + Some helpers (such as credential-store and credential-libsecret) delete\n + all matching credentials, others (such as credential-cache) delete at\n + most one matching credential.\n \n Test that helpers erase all matching credentials. This behaviour is\n easiest to reason about. Users expect that `echo\n @@ Commit message\n \"url=https://example.com\\nusername=tim\" | git credential reject` erase\n all matching credentials.\n \n - Fix credential-cache and credential-store.\n + Fix credential-cache.\n \n Signed-off-by: M Hickford \n \n @@ builtin/credential-cache--daemon.c: static void serve_one_client(FILE *in, FILE\n \t\t\tfprintf(out, \"username=%s\\n\", e->item.username);\n \t\t\tfprintf(out, \"password=%s\\n\", e->item.password);\n \n - ## builtin/credential-store.c ##\n -@@ builtin/credential-store.c: static int parse_credential_file(const char *fn,\n - \t\t\tfound_credential = 1;\n - \t\t\tif (match_cb) {\n - \t\t\t\tmatch_cb(&entry);\n --\t\t\t\tbreak;\n - \t\t\t}\n - \t\t}\n - \t\telse if (other_cb)\n -\n ## t/lib-credential.sh ##\n @@ t/lib-credential.sh: helper_test_clean() {\n - \treject $1 https example.com user2\n \treject $1 https example.com user4\n - \treject $1 https example.com user5\n -+\treject $1 https example.com user6\n -+\treject $1 https example.com user7\n - \treject $1 https example.com user8\n + \treject $1 https example.com user-distinct-pass\n + \treject $1 https example.com user-overwrite\n ++\treject $1 https example.com user-erase1\n ++\treject $1 https example.com user-erase2\n \treject $1 http path.tld user\n \treject $1 https timeout.tld user\n + \treject $1 https sso.tld\n @@ t/lib-credential.sh: helper_test() {\n \t\tEOF\n \t'\n @@ t/lib-credential.sh: helper_test() {\n +\t\tcheck approve $HELPER <<-\\EOF &&\n +\t\tprotocol=https\n +\t\thost=example.com\n -+\t\tusername=user6\n ++\t\tusername=user-erase1\n +\t\tpassword=pass1\n +\t\tEOF\n +\t\tcheck approve $HELPER <<-\\EOF &&\n +\t\tprotocol=https\n +\t\thost=example.com\n -+\t\tusername=user7\n ++\t\tusername=user-erase2\n +\t\tpassword=pass1\n +\t\tEOF\n +\t\tcheck reject $HELPER <<-\\EOF &&\n\n-- \ngitgitgadget\n"},{"id":"478450","messageId":"91d4b04b5e1e5a9580c0db72b227f0f60ee0dd58.1686856773.git.gitgitgadget@gmail.com","threadId":"59860","inReplyTo":"pull.1525.v4.git.git.1686856773.gitgitgadget@gmail.com","subject":"[PATCH v4 1/2] credential: avoid erasing distinct password","fromName":"M Hickford via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-06-15T19:19:32Z","receivedAt":"2023-06-15T19:19:43Z","isPatch":true,"sender":{"key":"mirth.hickford@gmail.com","avatar":"https://avatars.githubusercontent.com/u/105314?v=4"},"body":"From: M Hickford \n\nTest that credential helpers do not erase a password distinct from the\ninput. Such calls can happen when multiple credential helpers are\nconfigured.\n\nFixes for credential-cache and credential-store.\n\nSigned-off-by: M Hickford \n---\n builtin/credential-cache--daemon.c | 14 +++---\n builtin/credential-store.c | 15 ++++---\n credential.c | 7 +--\n credential.h | 2 +-\n t/lib-credential.sh | 70 ++++++++++++++++++++++++++++++\n 5 files changed, 90 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c\nindex 756c5f02aef..f64dd21d335 100644\n--- a/builtin/credential-cache--daemon.c\n+++ b/builtin/credential-cache--daemon.c\n@@ -33,22 +33,22 @@ static void cache_credential(struct credential *c, int timeout)\n \te->expiration = time(NULL) + timeout;\n }\n \n-static struct credential_cache_entry *lookup_credential(const struct credential *c)\n+static struct credential_cache_entry *lookup_credential(const struct credential *c, int match_password)\n {\n \tint i;\n \tfor (i = 0; i < entries_nr; i++) {\n \t\tstruct credential *e = &entries[i].item;\n-\t\tif (credential_match(c, e))\n+\t\tif (credential_match(c, e, match_password))\n \t\t\treturn &entries[i];\n \t}\n \treturn NULL;\n }\n \n-static void remove_credential(const struct credential *c)\n+static void remove_credential(const struct credential *c, int match_password)\n {\n \tstruct credential_cache_entry *e;\n \n-\te = lookup_credential(c);\n+\te = lookup_credential(c, match_password);\n \tif (e)\n \t\te->expiration = 0;\n }\n@@ -127,7 +127,7 @@ static void serve_one_client(FILE *in, FILE *out)\n \tif (read_request(in, &c, &action, &timeout) < 0)\n \t\t/* ignore error */ ;\n \telse if (!strcmp(action.buf, \"get\")) {\n-\t\tstruct credential_cache_entry *e = lookup_credential(&c);\n+\t\tstruct credential_cache_entry *e = lookup_credential(&c, 0);\n \t\tif (e) {\n \t\t\tfprintf(out, \"username=%s\\n\", e->item.username);\n \t\t\tfprintf(out, \"password=%s\\n\", e->item.password);\n@@ -151,14 +151,14 @@ static void serve_one_client(FILE *in, FILE *out)\n \t\texit(0);\n \t}\n \telse if (!strcmp(action.buf, \"erase\"))\n-\t\tremove_credential(&c);\n+\t\tremove_credential(&c, 1);\n \telse if (!strcmp(action.buf, \"store\")) {\n \t\tif (timeout < 0)\n \t\t\twarning(\"cache client didn't specify a timeout\");\n \t\telse if (!c.username || !c.password)\n \t\t\twarning(\"cache client gave us a partial credential\");\n \t\telse {\n-\t\t\tremove_credential(&c);\n+\t\t\tremove_credential(&c, 0);\n \t\t\tcache_credential(&c, timeout);\n \t\t}\n \t}\ndiff --git a/builtin/credential-store.c b/builtin/credential-store.c\nindex 30c6ccf56c0..0937230bced 100644\n--- a/builtin/credential-store.c\n+++ b/builtin/credential-store.c\n@@ -13,7 +13,8 @@ static struct lock_file credential_lock;\n static int parse_credential_file(const char *fn,\n \t\t\t\t struct credential *c,\n \t\t\t\t void (*match_cb)(struct credential *),\n-\t\t\t\t void (*other_cb)(struct strbuf *))\n+\t\t\t\t void (*other_cb)(struct strbuf *),\n+\t\t\t\t int match_password)\n {\n \tFILE *fh;\n \tstruct strbuf line = STRBUF_INIT;\n@@ -30,7 +31,7 @@ static int parse_credential_file(const char *fn,\n \twhile (strbuf_getline_lf(&line, fh) != EOF) {\n \t\tif (!credential_from_url_gently(&entry, line.buf, 1) &&\n \t\t entry.username && entry.password &&\n-\t\t credential_match(c, &entry)) {\n+\t\t credential_match(c, &entry, match_password)) {\n \t\t\tfound_credential = 1;\n \t\t\tif (match_cb) {\n \t\t\t\tmatch_cb(&entry);\n@@ -60,7 +61,7 @@ static void print_line(struct strbuf *buf)\n }\n \n static void rewrite_credential_file(const char *fn, struct credential *c,\n-\t\t\t\t struct strbuf *extra)\n+\t\t\t\t struct strbuf *extra, int match_password)\n {\n \tint timeout_ms = 1000;\n \n@@ -69,7 +70,7 @@ static void rewrite_credential_file(const char *fn, struct credential *c,\n \t\tdie_errno(_(\"unable to get credential storage lock in %d ms\"), timeout_ms);\n \tif (extra)\n \t\tprint_line(extra);\n-\tparse_credential_file(fn, c, NULL, print_line);\n+\tparse_credential_file(fn, c, NULL, print_line, match_password);\n \tif (commit_lock_file(&credential_lock) < 0)\n \t\tdie_errno(\"unable to write credential store\");\n }\n@@ -91,7 +92,7 @@ static void store_credential_file(const char *fn, struct credential *c)\n \t\t\t\t\tis_rfc3986_reserved_or_unreserved);\n \t}\n \n-\trewrite_credential_file(fn, c, &buf);\n+\trewrite_credential_file(fn, c, &buf, 0);\n \tstrbuf_release(&buf);\n }\n \n@@ -138,7 +139,7 @@ static void remove_credential(const struct string_list *fns, struct credential *\n \t\treturn;\n \tfor_each_string_list_item(fn, fns)\n \t\tif (!access(fn->string, F_OK))\n-\t\t\trewrite_credential_file(fn->string, c, NULL);\n+\t\t\trewrite_credential_file(fn->string, c, NULL, 1);\n }\n \n static void lookup_credential(const struct string_list *fns, struct credential *c)\n@@ -146,7 +147,7 @@ static void lookup_credential(const struct string_list *fns, struct credential *\n \tstruct string_list_item *fn;\n \n \tfor_each_string_list_item(fn, fns)\n-\t\tif (parse_credential_file(fn->string, c, print_entry, NULL))\n+\t\tif (parse_credential_file(fn->string, c, print_entry, NULL, 0))\n \t\t\treturn; /* Found credential */\n }\n \ndiff --git a/credential.c b/credential.c\nindex 023b59d5711..8825c6f1320 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -33,13 +33,14 @@ void credential_clear(struct credential *c)\n }\n \n int credential_match(const struct credential *want,\n-\t\t const struct credential *have)\n+\t\t const struct credential *have, int match_password)\n {\n #define CHECK(x) (!want->x || (have->x && !strcmp(want->x, have->x)))\n \treturn CHECK(protocol) &&\n \t CHECK(host) &&\n \t CHECK(path) &&\n-\t CHECK(username);\n+\t CHECK(username) &&\n+\t (!match_password || CHECK(password));\n #undef CHECK\n }\n \n@@ -102,7 +103,7 @@ static int match_partial_url(const char *url, void *cb)\n \t\twarning(_(\"skipping credential lookup for key: credential.%s\"),\n \t\t\turl);\n \telse\n-\t\tmatches = credential_match(&want, c);\n+\t\tmatches = credential_match(&want, c, 0);\n \tcredential_clear(&want);\n \n \treturn matches;\ndiff --git a/credential.h b/credential.h\nindex b8e2936d1dc..acc41adf548 100644\n--- a/credential.h\n+++ b/credential.h\n@@ -211,6 +211,6 @@ void credential_from_url(struct credential *, const char *url);\n int credential_from_url_gently(struct credential *, const char *url, int quiet);\n \n int credential_match(const struct credential *want,\n-\t\t const struct credential *have);\n+\t\t const struct credential *have, int match_password);\n \n #endif /* CREDENTIAL_H */\ndiff --git a/t/lib-credential.sh b/t/lib-credential.sh\nindex f1ab92ba35c..77baec53b6a 100644\n--- a/t/lib-credential.sh\n+++ b/t/lib-credential.sh\n@@ -44,6 +44,8 @@ helper_test_clean() {\n \treject $1 https example.com user1\n \treject $1 https example.com user2\n \treject $1 https example.com user4\n+\treject $1 https example.com user-distinct-pass\n+\treject $1 https example.com user-overwrite\n \treject $1 http path.tld user\n \treject $1 https timeout.tld user\n \treject $1 https sso.tld\n@@ -167,6 +169,49 @@ helper_test() {\n \t\tEOF\n \t'\n \n+\ttest_expect_success \"helper ($HELPER) overwrites on store\" '\n+\t\tcheck approve $HELPER <<-\\EOF &&\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\tusername=user-overwrite\n+\t\tpassword=pass1\n+\t\tEOF\n+\t\tcheck approve $HELPER <<-\\EOF &&\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\tusername=user-overwrite\n+\t\tpassword=pass2\n+\t\tEOF\n+\t\tcheck fill $HELPER <<-\\EOF &&\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\tusername=user-overwrite\n+\t\t--\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\tusername=user-overwrite\n+\t\tpassword=pass2\n+\t\tEOF\n+\t\tcheck reject $HELPER <<-\\EOF &&\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\tusername=user-overwrite\n+\t\tpassword=pass2\n+\t\tEOF\n+\t\tcheck fill $HELPER <<-\\EOF\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\tusername=user-overwrite\n+\t\t--\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\tusername=user-overwrite\n+\t\tpassword=askpass-password\n+\t\t--\n+\t\taskpass: Password for '\\''https://user-overwrite@example.com'\\'':\n+\t\tEOF\n+\t'\n+\n \ttest_expect_success \"helper ($HELPER) can forget host\" '\n \t\tcheck reject $HELPER <<-\\EOF &&\n \t\tprotocol=https\n@@ -221,6 +266,31 @@ helper_test() {\n \t\tEOF\n \t'\n \n+\ttest_expect_success \"helper ($HELPER) does not erase a password distinct from input\" '\n+\t\tcheck approve $HELPER <<-\\EOF &&\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\tusername=user-distinct-pass\n+\t\tpassword=pass1\n+\t\tEOF\n+\t\tcheck reject $HELPER <<-\\EOF &&\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\tusername=user-distinct-pass\n+\t\tpassword=pass2\n+\t\tEOF\n+\t\tcheck fill $HELPER <<-\\EOF\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\tusername=user-distinct-pass\n+\t\t--\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\tusername=user-distinct-pass\n+\t\tpassword=pass1\n+\t\tEOF\n+\t'\n+\n \ttest_expect_success \"helper ($HELPER) can forget user\" '\n \t\tcheck reject $HELPER <<-\\EOF &&\n \t\tprotocol=https\n-- \ngitgitgadget\n\n"},{"id":"478451","messageId":"42f41b28e6e8738f5f6b425489578bc033b7cbe2.1686856773.git.gitgitgadget@gmail.com","threadId":"59860","inReplyTo":"pull.1525.v4.git.git.1686856773.gitgitgadget@gmail.com","subject":"[PATCH v4 2/2] credential: erase all matching credentials","fromName":"M Hickford via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-06-15T19:19:33Z","receivedAt":"2023-06-15T19:19:50Z","isPatch":true,"sender":{"key":"mirth.hickford@gmail.com","avatar":"https://avatars.githubusercontent.com/u/105314?v=4"},"body":"From: M Hickford \n\n`credential reject` sends the erase action to each helper, but the\nexact behaviour of erase isn't specified in documentation or tests.\nSome helpers (such as credential-store and credential-libsecret) delete\nall matching credentials, others (such as credential-cache) delete at\nmost one matching credential.\n\nTest that helpers erase all matching credentials. This behaviour is\neasiest to reason about. Users expect that `echo\n\"url=https://example.com\" | git credential reject` or `echo\n\"url=https://example.com\\nusername=tim\" | git credential reject` erase\nall matching credentials.\n\nFix credential-cache.\n\nSigned-off-by: M Hickford \n---\n Documentation/git-credential.txt | 2 +-\n Documentation/gitcredentials.txt | 2 +-\n builtin/credential-cache--daemon.c | 15 ++++++++------\n t/lib-credential.sh | 33 ++++++++++++++++++++++++++++++\n 4 files changed, 44 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt\nindex 0e6d9e85ec7..a220afed4f3 100644\n--- a/Documentation/git-credential.txt\n+++ b/Documentation/git-credential.txt\n@@ -39,7 +39,7 @@ for later use.\n \n If the action is `reject`, git-credential will send the description to\n any configured credential helpers, which may erase any stored\n-credential matching the description.\n+credentials matching the description.\n \n If the action is `approve` or `reject`, no output should be emitted.\n \ndiff --git a/Documentation/gitcredentials.txt b/Documentation/gitcredentials.txt\nindex 100f045bb1a..65d652dc40e 100644\n--- a/Documentation/gitcredentials.txt\n+++ b/Documentation/gitcredentials.txt\n@@ -260,7 +260,7 @@ appended to its command line, which is one of:\n \n `erase`::\n \n-\tRemove a matching credential, if any, from the helper's storage.\n+\tRemove matching credentials, if any, from the helper's storage.\n \n The details of the credential will be provided on the helper's stdin\n stream. The exact format is the same as the input/output format of the\ndiff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c\nindex f64dd21d335..dc1cf2d25fc 100644\n--- a/builtin/credential-cache--daemon.c\n+++ b/builtin/credential-cache--daemon.c\n@@ -33,12 +33,12 @@ static void cache_credential(struct credential *c, int timeout)\n \te->expiration = time(NULL) + timeout;\n }\n \n-static struct credential_cache_entry *lookup_credential(const struct credential *c, int match_password)\n+static struct credential_cache_entry *lookup_credential(const struct credential *c)\n {\n \tint i;\n \tfor (i = 0; i < entries_nr; i++) {\n \t\tstruct credential *e = &entries[i].item;\n-\t\tif (credential_match(c, e, match_password))\n+\t\tif (credential_match(c, e, 0))\n \t\t\treturn &entries[i];\n \t}\n \treturn NULL;\n@@ -48,9 +48,12 @@ static void remove_credential(const struct credential *c, int match_password)\n {\n \tstruct credential_cache_entry *e;\n \n-\te = lookup_credential(c, match_password);\n-\tif (e)\n-\t\te->expiration = 0;\n+\tint i;\n+\tfor (i = 0; i < entries_nr; i++) {\n+\t\te = &entries[i];\n+\t\tif (credential_match(c, &e->item, match_password))\n+\t\t\te->expiration = 0;\n+\t}\n }\n \n static timestamp_t check_expirations(void)\n@@ -127,7 +130,7 @@ static void serve_one_client(FILE *in, FILE *out)\n \tif (read_request(in, &c, &action, &timeout) < 0)\n \t\t/* ignore error */ ;\n \telse if (!strcmp(action.buf, \"get\")) {\n-\t\tstruct credential_cache_entry *e = lookup_credential(&c, 0);\n+\t\tstruct credential_cache_entry *e = lookup_credential(&c);\n \t\tif (e) {\n \t\t\tfprintf(out, \"username=%s\\n\", e->item.username);\n \t\t\tfprintf(out, \"password=%s\\n\", e->item.password);\ndiff --git a/t/lib-credential.sh b/t/lib-credential.sh\nindex 77baec53b6a..032b2d8fcc4 100644\n--- a/t/lib-credential.sh\n+++ b/t/lib-credential.sh\n@@ -46,6 +46,8 @@ helper_test_clean() {\n \treject $1 https example.com user4\n \treject $1 https example.com user-distinct-pass\n \treject $1 https example.com user-overwrite\n+\treject $1 https example.com user-erase1\n+\treject $1 https example.com user-erase2\n \treject $1 http path.tld user\n \treject $1 https timeout.tld user\n \treject $1 https sso.tld\n@@ -342,6 +344,37 @@ helper_test() {\n \t\tEOF\n \t'\n \n+\ttest_expect_success \"helper ($HELPER) erases all matching credentials\" '\n+\t\tcheck approve $HELPER <<-\\EOF &&\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\tusername=user-erase1\n+\t\tpassword=pass1\n+\t\tEOF\n+\t\tcheck approve $HELPER <<-\\EOF &&\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\tusername=user-erase2\n+\t\tpassword=pass1\n+\t\tEOF\n+\t\tcheck reject $HELPER <<-\\EOF &&\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\tEOF\n+\t\tcheck fill $HELPER <<-\\EOF\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\t--\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\tusername=askpass-username\n+\t\tpassword=askpass-password\n+\t\t--\n+\t\taskpass: Username for '\\''https://example.com'\\'':\n+\t\taskpass: Password for '\\''https://askpass-username@example.com'\\'':\n+\t\tEOF\n+\t'\n+\n \t: ${GIT_TEST_LONG_CRED_BUFFER:=1024}\n \t# 23 bytes accounts for \"wwwauth[]=basic realm=\" plus NUL\n \tLONG_VALUE_LEN=$((GIT_TEST_LONG_CRED_BUFFER - 23))\n-- \ngitgitgadget\n"},{"id":"478452","messageId":"xmqqh6r8v3es.fsf@gitster.g","threadId":"59860","inReplyTo":"pull.1525.v4.git.git.1686856773.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 0/2] credential: improvements to erase in helpers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-15T21:09:15Z","receivedAt":"2023-06-15T21:09:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"M Hickford via GitGitGadget\" writes:\n\n> M Hickford (2):\n> credential: avoid erasing distinct password\n> credential: erase all matching credentials\n>\n> Documentation/git-credential.txt | 2 +-\n> Documentation/gitcredentials.txt | 2 +-\n> builtin/credential-cache--daemon.c | 17 +++--\n> builtin/credential-store.c | 15 +++--\n> credential.c | 7 +-\n> credential.h | 2 +-\n> t/lib-credential.sh | 103 +++++++++++++++++++++++++++++\n> 7 files changed, 128 insertions(+), 20 deletions(-)\n\nIt is helpful to reviewers to describe/summarize, in your own words,\nwhat changed since the previous version, in the cover letter.\n\nThe range-diff generated for the versions can serve as a good\nsupporting material, and it would help you while writing that\nsummary, but not a substitute for the summary.\n\nThanks.\n\n> base-commit: d7d8841f67f29e6ecbad85a11805c907d0f00d5d\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1525%2Fhickford%2Ferase-test-v4\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1525/hickford/erase-test-v4\n> Pull-Request: https://github.com/git/git/pull/1525\n>\n> Range-diff vs v3:\n>\n> 1: df3c8a15bf8 ! 1: 91d4b04b5e1 credential: avoid erasing distinct password\n> @@ builtin/credential-store.c: static struct lock_file credential_lock;\n> \tFILE *fh;\n> \tstruct strbuf line = STRBUF_INIT;\n> @@ builtin/credential-store.c: static int parse_credential_file(const char *fn,\n> - \n> \twhile (strbuf_getline_lf(&line, fh) != EOF) {\n> \t\tif (!credential_from_url_gently(&entry, line.buf, 1) &&\n> --\t\t entry.username && entry.password &&\n> + \t\t entry.username && entry.password &&\n> -\t\t credential_match(c, &entry)) {\n> -+\t\t\tentry.username && entry.password &&\n> -+\t\t\tcredential_match(c, &entry, match_password)) {\n> ++\t\t credential_match(c, &entry, match_password)) {\n> \t\t\tfound_credential = 1;\n> \t\t\tif (match_cb) {\n> \t\t\t\tmatch_cb(&entry);\n> @@ credential.c: void credential_clear(struct credential *c)\n> {\n> #define CHECK(x) (!want->x || (have->x && !strcmp(want->x, have->x)))\n> \treturn CHECK(protocol) &&\n> --\t CHECK(host) &&\n> --\t CHECK(path) &&\n> + \t CHECK(host) &&\n> + \t CHECK(path) &&\n> -\t CHECK(username);\n> -+\t\tCHECK(host) &&\n> -+\t\tCHECK(path) &&\n> -+\t\tCHECK(username) &&\n> -+\t\t(!match_password || CHECK(password));\n> ++\t CHECK(username) &&\n> ++\t (!match_password || CHECK(password));\n> #undef CHECK\n> }\n> \n> @@ t/lib-credential.sh: helper_test_clean() {\n> \treject $1 https example.com user1\n> \treject $1 https example.com user2\n> \treject $1 https example.com user4\n> -+\treject $1 https example.com user5\n> -+\treject $1 https example.com user8\n> ++\treject $1 https example.com user-distinct-pass\n> ++\treject $1 https example.com user-overwrite\n> \treject $1 http path.tld user\n> \treject $1 https timeout.tld user\n> \treject $1 https sso.tld\n> @@ t/lib-credential.sh: helper_test() {\n> +\t\tcheck approve $HELPER <<-\\EOF &&\n> +\t\tprotocol=https\n> +\t\thost=example.com\n> -+\t\tusername=user8\n> ++\t\tusername=user-overwrite\n> +\t\tpassword=pass1\n> +\t\tEOF\n> +\t\tcheck approve $HELPER <<-\\EOF &&\n> +\t\tprotocol=https\n> +\t\thost=example.com\n> -+\t\tusername=user8\n> ++\t\tusername=user-overwrite\n> +\t\tpassword=pass2\n> +\t\tEOF\n> +\t\tcheck fill $HELPER <<-\\EOF &&\n> +\t\tprotocol=https\n> +\t\thost=example.com\n> -+\t\tusername=user8\n> ++\t\tusername=user-overwrite\n> +\t\t--\n> +\t\tprotocol=https\n> +\t\thost=example.com\n> -+\t\tusername=user8\n> ++\t\tusername=user-overwrite\n> +\t\tpassword=pass2\n> +\t\tEOF\n> +\t\tcheck reject $HELPER <<-\\EOF &&\n> +\t\tprotocol=https\n> +\t\thost=example.com\n> -+\t\tusername=user8\n> ++\t\tusername=user-overwrite\n> +\t\tpassword=pass2\n> +\t\tEOF\n> +\t\tcheck fill $HELPER <<-\\EOF\n> +\t\tprotocol=https\n> +\t\thost=example.com\n> -+\t\tusername=user8\n> ++\t\tusername=user-overwrite\n> +\t\t--\n> +\t\tprotocol=https\n> +\t\thost=example.com\n> -+\t\tusername=user8\n> ++\t\tusername=user-overwrite\n> +\t\tpassword=askpass-password\n> +\t\t--\n> -+\t\taskpass: Password for '\\''https://user8@example.com'\\'':\n> ++\t\taskpass: Password for '\\''https://user-overwrite@example.com'\\'':\n> +\t\tEOF\n> +\t'\n> +\n> @@ t/lib-credential.sh: helper_test() {\n> +\t\tcheck approve $HELPER <<-\\EOF &&\n> +\t\tprotocol=https\n> +\t\thost=example.com\n> -+\t\tusername=user5\n> ++\t\tusername=user-distinct-pass\n> +\t\tpassword=pass1\n> +\t\tEOF\n> +\t\tcheck reject $HELPER <<-\\EOF &&\n> +\t\tprotocol=https\n> +\t\thost=example.com\n> -+\t\tusername=user5\n> ++\t\tusername=user-distinct-pass\n> +\t\tpassword=pass2\n> +\t\tEOF\n> +\t\tcheck fill $HELPER <<-\\EOF\n> +\t\tprotocol=https\n> +\t\thost=example.com\n> -+\t\tusername=user5\n> ++\t\tusername=user-distinct-pass\n> +\t\t--\n> +\t\tprotocol=https\n> +\t\thost=example.com\n> -+\t\tusername=user5\n> ++\t\tusername=user-distinct-pass\n> +\t\tpassword=pass1\n> +\t\tEOF\n> +\t'\n> 2: e06d80e99a0 ! 2: 42f41b28e6e credential: erase all matching credentials\n> @@ Commit message\n> \n> `credential reject` sends the erase action to each helper, but the\n> exact behaviour of erase isn't specified in documentation or tests.\n> - Some helpers (such as credential-libsecret) delete all matching\n> - credentials, others (such as credential-cache and credential-store)\n> - delete at most one matching credential.\n> + Some helpers (such as credential-store and credential-libsecret) delete\n> + all matching credentials, others (such as credential-cache) delete at\n> + most one matching credential.\n> \n> Test that helpers erase all matching credentials. This behaviour is\n> easiest to reason about. Users expect that `echo\n> @@ Commit message\n> \"url=https://example.com\\nusername=tim\" | git credential reject` erase\n> all matching credentials.\n> \n> - Fix credential-cache and credential-store.\n> + Fix credential-cache.\n> \n> Signed-off-by: M Hickford \n> \n> @@ builtin/credential-cache--daemon.c: static void serve_one_client(FILE *in, FILE\n> \t\t\tfprintf(out, \"username=%s\\n\", e->item.username);\n> \t\t\tfprintf(out, \"password=%s\\n\", e->item.password);\n> \n> - ## builtin/credential-store.c ##\n> -@@ builtin/credential-store.c: static int parse_credential_file(const char *fn,\n> - \t\t\tfound_credential = 1;\n> - \t\t\tif (match_cb) {\n> - \t\t\t\tmatch_cb(&entry);\n> --\t\t\t\tbreak;\n> - \t\t\t}\n> - \t\t}\n> - \t\telse if (other_cb)\n> -\n> ## t/lib-credential.sh ##\n> @@ t/lib-credential.sh: helper_test_clean() {\n> - \treject $1 https example.com user2\n> \treject $1 https example.com user4\n> - \treject $1 https example.com user5\n> -+\treject $1 https example.com user6\n> -+\treject $1 https example.com user7\n> - \treject $1 https example.com user8\n> + \treject $1 https example.com user-distinct-pass\n> + \treject $1 https example.com user-overwrite\n> ++\treject $1 https example.com user-erase1\n> ++\treject $1 https example.com user-erase2\n> \treject $1 http path.tld user\n> \treject $1 https timeout.tld user\n> + \treject $1 https sso.tld\n> @@ t/lib-credential.sh: helper_test() {\n> \t\tEOF\n> \t'\n> @@ t/lib-credential.sh: helper_test() {\n> +\t\tcheck approve $HELPER <<-\\EOF &&\n> +\t\tprotocol=https\n> +\t\thost=example.com\n> -+\t\tusername=user6\n> ++\t\tusername=user-erase1\n> +\t\tpassword=pass1\n> +\t\tEOF\n> +\t\tcheck approve $HELPER <<-\\EOF &&\n> +\t\tprotocol=https\n> +\t\thost=example.com\n> -+\t\tusername=user7\n> ++\t\tusername=user-erase2\n> +\t\tpassword=pass1\n> +\t\tEOF\n> +\t\tcheck reject $HELPER <<-\\EOF &&\n"},{"id":"478453","messageId":"20230615212116.GA39325@coredump.intra.peff.net","threadId":"59860","inReplyTo":"xmqqh6r8v3es.fsf@gitster.g","subject":"Re: [PATCH v4 0/2] credential: improvements to erase in helpers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-06-15T21:21:16Z","receivedAt":"2023-06-15T21:21:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 15, 2023 at 02:09:15PM -0700, Junio C Hamano wrote:\n\n> \"M Hickford via GitGitGadget\" writes:\n> \n> > M Hickford (2):\n> > credential: avoid erasing distinct password\n> > credential: erase all matching credentials\n> >\n> > Documentation/git-credential.txt | 2 +-\n> > Documentation/gitcredentials.txt | 2 +-\n> > builtin/credential-cache--daemon.c | 17 +++--\n> > builtin/credential-store.c | 15 +++--\n> > credential.c | 7 +-\n> > credential.h | 2 +-\n> > t/lib-credential.sh | 103 +++++++++++++++++++++++++++++\n> > 7 files changed, 128 insertions(+), 20 deletions(-)\n> \n> It is helpful to reviewers to describe/summarize, in your own words,\n> what changed since the previous version, in the cover letter.\n> \n> The range-diff generated for the versions can serve as a good\n> supporting material, and it would help you while writing that\n> summary, but not a substitute for the summary.\n\nYeah, I agree that would have made reviewing much easier. :)\n\nThat said, I just re-reviewed the patches themselves, and everything now\nlooks good to me.\n\n-Peff\n"},{"id":"478454","messageId":"xmqqcz1wv1f8.fsf@gitster.g","threadId":"59860","inReplyTo":"20230615212116.GA39325@coredump.intra.peff.net","subject":"Re: [PATCH v4 0/2] credential: improvements to erase in helpers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-15T21:52:11Z","receivedAt":"2023-06-15T21:52:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Jun 15, 2023 at 02:09:15PM -0700, Junio C Hamano wrote:\n>\n>> \"M Hickford via GitGitGadget\" writes:\n>> \n>> > M Hickford (2):\n>> > credential: avoid erasing distinct password\n>> > credential: erase all matching credentials\n>> >\n>> > Documentation/git-credential.txt | 2 +-\n>> > Documentation/gitcredentials.txt | 2 +-\n>> > builtin/credential-cache--daemon.c | 17 +++--\n>> > builtin/credential-store.c | 15 +++--\n>> > credential.c | 7 +-\n>> > credential.h | 2 +-\n>> > t/lib-credential.sh | 103 +++++++++++++++++++++++++++++\n>> > 7 files changed, 128 insertions(+), 20 deletions(-)\n>> \n>> It is helpful to reviewers to describe/summarize, in your own words,\n>> what changed since the previous version, in the cover letter.\n>> \n>> The range-diff generated for the versions can serve as a good\n>> supporting material, and it would help you while writing that\n>> summary, but not a substitute for the summary.\n>\n> Yeah, I agree that would have made reviewing much easier. :)\n>\n> That said, I just re-reviewed the patches themselves, and everything now\n> looks good to me.\n\nI missed that you suggested fixing the indentation breakage\nintroduced in the previous round, and the part of the range-diff,\nwhich was unexpected to me because I lacked the context, was\ndistracting enough that I missed other changes X-<. \n\nBut the end result does look good to me, too.\n\nThanks.\n"},{"id":"478471","messageId":"xmqqfs6rqrf7.fsf@gitster.g","threadId":"59860","inReplyTo":"20230615212116.GA39325@coredump.intra.peff.net","subject":"Re: [PATCH v4 0/2] credential: improvements to erase in helpers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-16T16:54:04Z","receivedAt":"2023-06-16T16:56:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> That said, I just re-reviewed the patches themselves, and everything now\n> looks good to me.\n\nThanks for writing and reviewing.\n\nQueued.\n"}]}