Re: [PATCH v3 2/2] credential: erase all matching credentials
- From
Jeff King <peff@peff.net>
- Date
- Jun 15, 2023, 07:09 UTC
- Message-ID
- <20230615070930.GB1460739@coredump.intra.peff.net>
- In-Reply-To
- <e06d80e99a00c30fb8430afcd457576bb0bf9ce5.1686809004.git.gitgitgadget@gmail.com>
On Thu, Jun 15, 2023 at 06:03:24AM +0000, M Hickford via GitGitGadget wrote:
Show 15 quoted lines
> From: M Hickford <mirth.hickford@gmail.com> > > `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. > > Test that helpers erase all matching credentials. This behaviour is > easiest to reason about. Users expect that `echo > "url=https://example.com" | git credential reject` or `echo > "url=https://example.com\nusername=tim" | git credential reject` erase > all matching credentials. > > Fix credential-cache and credential-store.
This last sentence is out of date now, right? We are only fixing credential-cache.
Show 12 quoted lines
> diff --git a/builtin/credential-store.c b/builtin/credential-store.c
> index f80ff59f18a..06bbfa4dd03 100644
> --- a/builtin/credential-store.c
> +++ b/builtin/credential-store.c
> @@ -35,7 +35,6 @@ static int parse_credential_file(const char *fn,
> found_credential = 1;
> if (match_cb) {
> match_cb(&entry);
> - break;
> }
> }
> else if (other_cb)This hunk is wrong, isn't it? We should not be touching credential-store at all (and losing the "break" means that we'd accidentally print multiple matching entries for "get").
-Peff