[0/3] Correct credential helper discrepancies handling input

Message

John Passaro via GitGitGadget Sept. 22, 2022, 4:59 p.m. UTC

Whilst working in the credential helper and auth space I happened to notice
that the behaviour of some of the credential helpers in contrib/credential/
is not consistent.

Specifically both the git-credential-wincred and git-credential-netrc
helpers die when they receive unknown credential property keys, which is
contrary to the behaviour of all the other in-tree helpers including:
git-credential-cache, -store, -libsecret, -gnome-keyring, -osxkeychain.

Also update the git-credential-osxkeychain helper to include a comment
making it's behaviour explicit in ignoring unknown keys, as per other
helpers.

Matthew John Cheetham (3):
  wincred: ignore unknown lines (do not die)
  netrc: ignore unknown lines (do not die)
  osxkeychain: clarify that we ignore unknown lines

 contrib/credential/netrc/git-credential-netrc.perl         | 5 ++++-
 .../credential/osxkeychain/git-credential-osxkeychain.c    | 5 +++++
 contrib/credential/wincred/git-credential-wincred.c        | 7 +++++--
 3 files changed, 14 insertions(+), 3 deletions(-)


base-commit: dd3f6c4cae7e3b15ce984dce8593ff7569650e24
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1363%2Fmjcheetham%2Fcredhelper-fixes-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1363/mjcheetham/credhelper-fixes-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1363

Comments

Derrick Stolee Sept. 22, 2022, 6:31 p.m. UTC | #1

On 9/22/2022 12:59 PM, Matthew John Cheetham via GitGitGadget wrote:
> Whilst working in the credential helper and auth space I happened to notice
> that the behaviour of some of the credential helpers in contrib/credential/
> is not consistent.
> 
> Specifically both the git-credential-wincred and git-credential-netrc
> helpers die when they receive unknown credential property keys, which is
> contrary to the behaviour of all the other in-tree helpers including:
> git-credential-cache, -store, -libsecret, -gnome-keyring, -osxkeychain.
> 
> Also update the git-credential-osxkeychain helper to include a comment
> making it's behaviour explicit in ignoring unknown keys, as per other
> helpers.

Thanks for sending this patches separate from your earlier RFC. I
think these patches speak for themselves and are good to go.

Thanks,
-Stolee

Jeff King Sept. 22, 2022, 10:11 p.m. UTC | #2

On Thu, Sep 22, 2022 at 04:59:31PM +0000, Matthew John Cheetham via GitGitGadget wrote:

> Whilst working in the credential helper and auth space I happened to notice
> that the behaviour of some of the credential helpers in contrib/credential/
> is not consistent.
> 
> Specifically both the git-credential-wincred and git-credential-netrc
> helpers die when they receive unknown credential property keys, which is
> contrary to the behaviour of all the other in-tree helpers including:
> git-credential-cache, -store, -libsecret, -gnome-keyring, -osxkeychain.
> 
> Also update the git-credential-osxkeychain helper to include a comment
> making it's behaviour explicit in ignoring unknown keys, as per other
> helpers.

Thanks, I think these are all sensible changes. The new behavior matches
the original vision of the helper protocol. But more importantly, it
brings them in line with all of the _other_ existing helpers. Which is
no coincidence, since the designer of the protocol wrote most of them. ;)

I haven't had a chance to look carefully at the rest of your larger
series yet, but I think these are a nice standalone improvement
regardless of what happens there.

-Peff