Re: [PATCH 03/13] introduce credentials API

From: Jeff King <peff@peff.net>
Date: Nov 29, 2011, 21:14 UTC
Message-ID: <20111129211411.GC1793@sigill.intra.peff.net>
In-Reply-To: <7vmxbeu91d.fsf@alter.siamese.dyndns.org>

On Tue, Nov 29, 2011 at 09:34:54AM -0800, Junio C Hamano wrote:

Show 6 quoted lines

> >> > +`credential_fill`::
> >> > +
> >> > +	Attempt to fill the username and password fields of the passed
> >> > +	credential struct, first consulting storage helpers, then asking
> >> > +	the user. Guarantees that the username and password fields will
> >> > +	be filled afterwards (or die() will be called).

Show 10 quoted lines

> Immensely, at least to me. From the perspective of a user of the API, a
> call to credential_fill() is to "fill in the credential" in the sense that
> "call the function to fill in the credential" but I find it easier to
> understand if it were explained to me as "ask the API to fill in the
> credential, which may involve helpers to interact with the user--the point
> of the API is that the caller does not care how it is done".  Same for the
> reject/accept calls---the example makes it clear that they are to tell the
> decision to reject/accept made by the application to the credential API,
> and it is up to the API layer what it does using that decision (like
> removing the cached and now stale password).

Ahh, I see your confusion now. It is not that the description is necessarily lacking any content, but that the tense of the sentences is misleading. I'll fix that.

> The above example is a bit too simplistic and misleading, though. You
> would call reject only on authentication failure (do not trash stored and
> good password upon network being unreachable temporarily or the server
> being overloaded).

Good point. I'll fix that and add the example to the documentation.

Show 12 quoted lines

> > So one possible rule would be:
> >
> >   1. If it starts with "!", clip off the "!" and hand it to the shell.
> >
> >   2. Otherwise, if is_absolute_path(), hand it to the shell directly.
> >
> >   3. Otherwise, prepend "git credential-" and hand it to the shell.
> >
> > I think that is slightly less confusing than the "first word is alnum"
> > thing.
> 
> Simpler and easier to explain. Good ;-)

OK, I'll implement that, then.

Show 6 quoted lines

> > How do you feel about the "values cannot contain a newline" requirement?
> 
> In the context of asking username, password, or passphrase, I think "LF is
> the end of the line and you cannot have that byte in your response" is
> perfectly reasonable. I've yet to find a way to use LF in a passphrase to
> unlock my Gnome keychain ;-).

The potential issue is that other values get that, too. So if you have a URL with "\n" in the path, it cannot be transmitted verbatim. We can url-encode, of course, but I didn't want the helpers to have to deal with quoting issues.

Show 5 quoted lines

> >> Two style nits.
> >
> > I'm supposed to guess? ;P
> 
> Sorry, but you guessed right.

OK, will fix.

-Peff