Re: [PATCH] credential-osxkeychain: support more protocols

From: Xidorn Quan <quanxunzhen@gmail.com>
Date: May 27, 2013, 13:57 UTC
Message-ID: <CAMdq69_=21de4FvGkp0bFtZjNOttkwXOxjvhM0yZi5gyj0-V9g@mail.gmail.com>
In-Reply-To: <CAEBDL5W4sLB0R1ZOspb-yQzmyTCE7Y1HeC2KZ69F8R28fJY7_A@mail.gmail.com>

On Mon, May 27, 2013 at 6:27 PM, John Szakmeister <john@szakmeister.net> wrote:

Show 37 quoted lines

>
> On Mon, May 27, 2013 at 3:57 AM, Xidorn Quan <quanxunzhen@gmail.com> wrote:
> > Add protocol ftp, smtp, and ssh for credential-osxkeychain.
> > ---
> >  contrib/credential/osxkeychain/git-credential-osxkeychain.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
> > index 3940202..4ddcfb3 100644
> > --- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c
> > +++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
> > @@ -127,10 +127,16 @@ static void read_credential(void)
> >                 *v++ = '\0';
> >
> >                 if (!strcmp(buf, "protocol")) {
> > -                       if (!strcmp(v, "https"))
> > +                       if (!strcmp(v, "ftp"))
> > +                               protocol = kSecProtocolTypeFTP;
> > +                       else if (!strcmp(v, "https"))
> >                                 protocol = kSecProtocolTypeHTTPS;
> >                         else if (!strcmp(v, "http"))
> >                                 protocol = kSecProtocolTypeHTTP;
> > +                       else if (!strcmp(v, "smtp"))
> > +                               protocol = kSecProtocolTypeSMTP;
> > +                       else if (!strcmp(v, "ssh"))
> > +                               protocol = kSecProtocolTypeSSH;
> >                         else /* we don't yet handle other protocols */
> >                                 exit(0);
>
> This looks pretty good, except the last one raises a question.  I'm
> using Mac OS X, and ssh already interacts with keychain to get my SSH
> key password.  Is this mainly for password logins via SSH?  Assuming
> that's the case:
>
> Signed-off-by: John Szakmeister <john@szakmeister.net>
>
> -John

I thought that SSH password logins can benefit from it, but I just found that it is wrong because it seems that SSH client is responsible for authenticating. Consequently, supporting SSH here is useless. I will remove that lines and send this patch again.

Since it is the first time I submit a patch to git, I am not very familiar with the convention here. Should I send the modified patch to the maintainer directly? And what information should I append to my patch before it can get merged?

-- Xidorn Quan