"M Hickford via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes: > M Hickford (2): > credential: avoid erasing distinct password > credential: erase all matching credentials > > Documentation/git-credential.txt | 2 +- > Documentation/gitcredentials.txt | 2 +- > builtin/credential-cache--daemon.c | 17 +++-- > builtin/credential-store.c | 15 +++-- > credential.c | 7 +- > credential.h | 2 +- > t/lib-credential.sh | 103 +++++++++++++++++++++++++++++ > 7 files changed, 128 insertions(+), 20 deletions(-) It is helpful to reviewers to describe/summarize, in your own words, what changed since the previous version, in the cover letter. The range-diff generated for the versions can serve as a good supporting material, and it would help you while writing that summary, but not a substitute for the summary. Thanks. > base-commit: d7d8841f67f29e6ecbad85a11805c907d0f00d5d > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1525%2Fhickford%2Ferase-test-v4 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1525/hickford/erase-test-v4 > Pull-Request: https://github.com/git/git/pull/1525 > > Range-diff vs v3: > > 1: df3c8a15bf8 ! 1: 91d4b04b5e1 credential: avoid erasing distinct password > @@ builtin/credential-store.c: static struct lock_file credential_lock; > FILE *fh; > struct strbuf line = STRBUF_INIT; > @@ builtin/credential-store.c: static int parse_credential_file(const char *fn, > - > while (strbuf_getline_lf(&line, fh) != EOF) { > if (!credential_from_url_gently(&entry, line.buf, 1) && > -- entry.username && entry.password && > + entry.username && entry.password && > - credential_match(c, &entry)) { > -+ entry.username && entry.password && > -+ credential_match(c, &entry, match_password)) { > ++ credential_match(c, &entry, match_password)) { > found_credential = 1; > if (match_cb) { > match_cb(&entry); > @@ credential.c: void credential_clear(struct credential *c) > { > #define CHECK(x) (!want->x || (have->x && !strcmp(want->x, have->x))) > return CHECK(protocol) && > -- CHECK(host) && > -- CHECK(path) && > + CHECK(host) && > + CHECK(path) && > - CHECK(username); > -+ CHECK(host) && > -+ CHECK(path) && > -+ CHECK(username) && > -+ (!match_password || CHECK(password)); > ++ CHECK(username) && > ++ (!match_password || CHECK(password)); > #undef CHECK > } > > @@ t/lib-credential.sh: helper_test_clean() { > reject $1 https example.com user1 > reject $1 https example.com user2 > reject $1 https example.com user4 > -+ reject $1 https example.com user5 > -+ reject $1 https example.com user8 > ++ reject $1 https example.com user-distinct-pass > ++ reject $1 https example.com user-overwrite > reject $1 http path.tld user > reject $1 https timeout.tld user > reject $1 https sso.tld > @@ t/lib-credential.sh: helper_test() { > + check approve $HELPER <<-\EOF && > + protocol=https > + host=example.com > -+ username=user8 > ++ username=user-overwrite > + password=pass1 > + EOF > + check approve $HELPER <<-\EOF && > + protocol=https > + host=example.com > -+ username=user8 > ++ username=user-overwrite > + password=pass2 > + EOF > + check fill $HELPER <<-\EOF && > + protocol=https > + host=example.com > -+ username=user8 > ++ username=user-overwrite > + -- > + protocol=https > + host=example.com > -+ username=user8 > ++ username=user-overwrite > + password=pass2 > + EOF > + check reject $HELPER <<-\EOF && > + protocol=https > + host=example.com > -+ username=user8 > ++ username=user-overwrite > + password=pass2 > + EOF > + check fill $HELPER <<-\EOF > + protocol=https > + host=example.com > -+ username=user8 > ++ username=user-overwrite > + -- > + protocol=https > + host=example.com > -+ username=user8 > ++ username=user-overwrite > + password=askpass-password > + -- > -+ askpass: Password for '\''https://user8@xxxxxxxxxxx'\'': > ++ askpass: Password for '\''https://user-overwrite@xxxxxxxxxxx'\'': > + EOF > + ' > + > @@ t/lib-credential.sh: helper_test() { > + check approve $HELPER <<-\EOF && > + protocol=https > + host=example.com > -+ username=user5 > ++ username=user-distinct-pass > + password=pass1 > + EOF > + check reject $HELPER <<-\EOF && > + protocol=https > + host=example.com > -+ username=user5 > ++ username=user-distinct-pass > + password=pass2 > + EOF > + check fill $HELPER <<-\EOF > + protocol=https > + host=example.com > -+ username=user5 > ++ username=user-distinct-pass > + -- > + protocol=https > + host=example.com > -+ username=user5 > ++ username=user-distinct-pass > + password=pass1 > + EOF > + ' > 2: e06d80e99a0 ! 2: 42f41b28e6e credential: erase all matching credentials > @@ Commit message > > `credential reject` sends the erase action to each helper, but the > exact behaviour of erase isn't specified in documentation or tests. > - Some helpers (such as credential-libsecret) delete all matching > - credentials, others (such as credential-cache and credential-store) > - delete at most one matching credential. > + Some helpers (such as credential-store and credential-libsecret) delete > + all matching credentials, others (such as credential-cache) delete at > + most one matching credential. > > Test that helpers erase all matching credentials. This behaviour is > easiest to reason about. Users expect that `echo > @@ Commit message > "url=https://example.com\nusername=tim"; | git credential reject` erase > all matching credentials. > > - Fix credential-cache and credential-store. > + Fix credential-cache. > > Signed-off-by: M Hickford <mirth.hickford@xxxxxxxxx> > > @@ builtin/credential-cache--daemon.c: static void serve_one_client(FILE *in, FILE > fprintf(out, "username=%s\n", e->item.username); > fprintf(out, "password=%s\n", e->item.password); > > - ## builtin/credential-store.c ## > -@@ builtin/credential-store.c: static int parse_credential_file(const char *fn, > - found_credential = 1; > - if (match_cb) { > - match_cb(&entry); > -- break; > - } > - } > - else if (other_cb) > - > ## t/lib-credential.sh ## > @@ t/lib-credential.sh: helper_test_clean() { > - reject $1 https example.com user2 > reject $1 https example.com user4 > - reject $1 https example.com user5 > -+ reject $1 https example.com user6 > -+ reject $1 https example.com user7 > - reject $1 https example.com user8 > + reject $1 https example.com user-distinct-pass > + reject $1 https example.com user-overwrite > ++ reject $1 https example.com user-erase1 > ++ reject $1 https example.com user-erase2 > reject $1 http path.tld user > reject $1 https timeout.tld user > + reject $1 https sso.tld > @@ t/lib-credential.sh: helper_test() { > EOF > ' > @@ t/lib-credential.sh: helper_test() { > + check approve $HELPER <<-\EOF && > + protocol=https > + host=example.com > -+ username=user6 > ++ username=user-erase1 > + password=pass1 > + EOF > + check approve $HELPER <<-\EOF && > + protocol=https > + host=example.com > -+ username=user7 > ++ username=user-erase2 > + password=pass1 > + EOF > + check reject $HELPER <<-\EOF &&
