Re: [PATCH/RFC] credentials helpers+remote helpers

Subject: Re: [PATCH/RFC] credentials helpers+remote helpers

This is not a good title. The one you have below (which I suggested
off-list) should be the first line of the patch.

Javier.Roucher-Iglesias@ensimag.imag.fr writes:

Add "git credential" plumbing command

(I mean this one)

 .gitignore                       |  1 +
 Documentation/git-credential.txt | 70 ++++++++++++++++++++++++++++++++++++++++
 Makefile                         |  1 +
 builtin.h                        |  1 +
 builtin/credential.c             | 40 +++++++++++++++++++++++
 git.c                            |  1 +

No tests?

Since this new command is essentially a copy-paste of test-credential.c,
it would indeed make sense to replace test-credential calls in the
scripts by calls to this "git credential" command.

This file is not valid asciidoc. Please, run "make doc" and see if the
generated HTML is right. Currently, it says

    SUBDIR ../
make[1]: `GIT-VERSION-FILE' is up to date.
    ASCIIDOC git-credential.html
asciidoc: ERROR: git-credential.txt: line 69: [blockdef-listing] missing closing delimiter
make: *** [git-credential.html] Error 1

(didn't I already mention it off-list?)

+------------------
+git credential [fill|approve|reject]
+
+------------------

[...]

+static const char usage_msg[] =
+"credential <fill|approve|reject> [helper...]";

Which one is right?

I already suggested several times that you get rid of this "helper"
argument (inherited from test-credential), or that you give it a better
API (e.g. --helper HELPER, but not positional argument). If this
argument is usefull, then keep it but it should be documented properly.

+Git-credential permits to save username, password, host, path and protocol.
+When you invoke git-credential, you can ask for a password, using the command
+'git credential fill'.

Who is "you" here? This "you" is the developer writing a script using
"git credential", while this one :

+-If it is not stored, git-credential will ask you to enter 
+the password:
+		
+		> Password for '[http|https]admin@localhost':

... is the user of the script. Please be more precise, e.g. "a script
can ask ...", "the user will be prompted for a password".

+Then if password is correct, you can store using command

if _the_ password ... you can store _it_ using ...

+If the password is refused, you can delete using command

delete _it_

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/