Re: [PATCH 03/13] introduce credentials API

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

quoted

quoted

quoted

+`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).

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.

quoted

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.

quoted

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.

quoted

quoted

Two style nits.

I'm supposed to guess? ;P

Sorry, but you guessed right.

OK, will fix.

-Peff