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

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Jun 7, 2012, 17:13 UTC
Message-ID: <vpqvcj32ggt.fsf@bauges.imag.fr>
In-Reply-To: <1339079743-31068-1-git-send-email-Javier.Roucher-Iglesias@ensimag.imag.fr>

> 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)

Show 6 quoted lines

>  .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.

> index 0000000..a6e1d0a
> --- /dev/null
> +++ b/Documentation/git-credential.txt

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/