Matthieu Moy <Matthieu.Moy@xxxxxxx> writes: > From: Javier Roucher Iglesias <Javier.Roucher-Iglesias@xxxxxxxxxxxxxxx> > > The credential API is in C, and not available to scripting languages. > Expose the functionalities of the API by wrapping them into a new > plumbing command "git credentials". > > In other words, replace the internal "test-credential" by an official Git > command. > > Most documentation writen by: Jeff King <peff@xxxxxxxx> > Signed-off-by: Pavel Volek <Pavel.Volek@xxxxxxxxxxxxxxx> > Signed-off-by: Kim Thuat Nguyen <Kim-Thuat.Nguyen@xxxxxxxxxxxxxxx> > Signed-off-by: Javier Roucher Iglesias <Javier.Roucher-Iglesias@xxxxxxxxxxxxxxx> > Signed-off-by: Matthieu Moy <Matthieu.Moy@xxxxxxx> > --- > .gitignore | 2 +- > Documentation/git-credential.txt | 126 ++++++++++++++++++++++++++++ > Documentation/technical/api-credentials.txt | 39 +-------- > Makefile | 2 +- > builtin.h | 1 + > test-credential.c => builtin/credential.c | 20 ++--- Nice ;-) > diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt > new file mode 100644 > index 0000000..b64ac30 > --- /dev/null > +++ b/Documentation/git-credential.txt > @@ -0,0 +1,126 @@ > ... > +DESCRIPTION > +----------- > +... Very clearly written. > +TYPICAL WORKFLOW > +---------------- > + > +An application using git-credential will typically follow this > +workflow: Again, who does what in what order is very clearly described below. One minor question I had was "is this _workflow_?". "An application using ... typically follow this workflow:" might want to be reworded to "An application uses `git credential` like this:" or something like that. I am not sure about the section title, though. > + 1. Generate a credential description based on the context. > ++ > +For example, if we want a password for > +`https://example.com/foo.git`, we might generate the following > +credential description (don't forget the blank line at the end): Add "; it tells `git credential` that the application finished feeding all the infomation it has" or something after "blank line at the end" to justify why the user must not forget it. > + > + protocol=https > + host=example.com > + path=foo.git I also thought that we discussed optionally removing the burden of parsing the incoming URL (e.g. https://exmaple.com/foo.git) into its components by giving them a way to feed a single line url=https://example.com/foo.git in place of the above three? Perhaps it will come as an enhancement in a later patch in the series? > + 2. Ask git-credential to give us a username and password for this > + description. This is done by running `git credential fill`, > + feeding the description from step (1) to its stdin. The username > + and password will be produced on stdout, like: > + > + username=bob > + password=secr3t I may be ahead of myself, but the following untold paragraph came to my mind while I was reading this: If the "git credential" knew about the password, this step may not have involved the user actually typing this password (the user may have typed a password to unlock the keychain instead, or no user interaction was done if the keychain was already unlocked) before it returned "password=secr3t". > + 3. Try to use the credential (e.g., by accessing the URL with the > + username and password from step (2)). OK. Drop "Try to use" and just say "Use"; it's clearer. Similarly, s/by accessing/access/. > + 4. Report on the success or failure of the password. If the > + credential allowed the operation to complete successfully, then > + it can be marked with an "approve" action. If the credential was > + rejected during the operation, use the "reject" action. In either > + case, `git credential` should be fed with the credential > + description obtained from step (2) together with the ones already > + provided in step (1). A question that came to my mind after the first sentence was "Why should I report it? What benefit am I getting, if I successfully accessed the resource I wanted to in the previous step already?" 4. Tell the git credential if the credential was good, so that git credential can ask the user the correct password upon the next invocation, if the one returned in step 2. did not work (e.g. a bad password came from a keychain), by using "approve" to tell it that it was good, or "reject" to tell it that it was bad. In either case, ... The term "credential" is used here without definition (I think you meant a <username, password> pair with the word). I think it makes the description shorter to say "credential" instead of "username and password", but then we would want to define the term upfront and use it throughout the document. E.g. the end of step 2. would read like this if we did so: ... to its standard input. The credential will be produced on the standard output, like so: username=bob password=secr3t Oh, another thing. Please avoid using "stdin", etc., when you are not discussing variables in the program text but you are referring to them as mechanism names, and instead spell it out. > +[[IOFMT]] > +INPUT/OUTPUT FORMAT > +------------------- > + > +`git credential` reads and/or writes (depending on the action used) > +credential information in its standard input/output. These information > +can correspond either to keys from which `git credential` will obtain > +the login/password information (e.g. host, protocol, path), or to the > +actual credential data to be obtained (login/password). Shouldn't "keys from which ..." be "keys for which ..."? You ask for a data item from somebody who has the data _for_ a key, or you query a data item from a database _with_ a key. The data source could be human in this context, so I'd prefer not get my brain queried with a key (asking me politely for data for a key is OK ;-). > +The credential is split into a set of named attributes. > +Attributes are provided to the helper, one per line. Each attribute is > ... I didn't have any issue with the remainder of the paragraph. > diff --git a/Documentation/technical/api-credentials.txt b/Documentation/technical/api-credentials.txt > index adb6f0c..5977b58 100644 > --- a/Documentation/technical/api-credentials.txt > +++ b/Documentation/technical/api-credentials.txt > @@ -241,42 +241,9 @@ appended to its command line, which is one of: > Remove a matching credential, if any, from the helper's storage. > > The details of the credential will be provided on the helper's stdin > +stream. The exact format is the same as the input/output format of the > +`git credential` plumbing command (see the section `INPUT/OUTPUT > +FORMAT` in linkgit:git-credential[7] for a detailed specification). OK. > diff --git a/test-credential.c b/builtin/credential.c > similarity index 63% > rename from test-credential.c > rename to builtin/credential.c > index dee200e..4147314 100644 > --- a/test-credential.c > +++ b/builtin/credential.c > @@ -1,21 +1,18 @@ > -#include "cache.h" > +#include <stdio.h> > #include "credential.h" > -#include "string-list.h" > +#include "builtin.h" I understand that you do not want the entire cache.h, but could you include "git-compat-util.h" instead? We try to absorb platform dependencies in that header file and avoid including system headers directly to C sources. > static const char usage_msg[] = > -"test-credential <fill|approve|reject> [helper...]"; > + "credential <fill|approve|reject>"; > > -int main(int argc, const char **argv) > +int cmd_credential (int argc, const char **argv, const char *prefix) Style. > diff --git a/git.c b/git.c > index d232de9..660c926 100644 > --- a/git.c > +++ b/git.c > @@ -353,6 +353,7 @@ static void handle_internal_command(int argc, const char **argv) > { "commit-tree", cmd_commit_tree, RUN_SETUP }, > { "config", cmd_config, RUN_SETUP_GENTLY }, > { "count-objects", cmd_count_objects, RUN_SETUP }, > + { "credential", cmd_credential, RUN_SETUP_GENTLY }, Good. > diff --git a/t/lib-credential.sh b/t/lib-credential.sh > index 4a37cd7..7c4826e 100755 > --- a/t/lib-credential.sh > +++ b/t/lib-credential.sh > @@ -4,10 +4,20 @@ > # stdout and stderr should be provided on stdin, > # separated by "--". > check() { > + credential_opts= > + credential_cmd=$1 > + shift > + for arg in "$@"; do > + credential_opts="$credential_opts -c credential.helper='$arg'" > + done > read_chunk >stdin && > read_chunk >expect-stdout && > read_chunk >expect-stderr && > - test-credential "$@" <stdin >stdout 2>stderr && > + if ! eval "git $credential_opts credential $credential_cmd <stdin >stdout 2>stderr"; then > + echo "git credential failed with code $?" && > + cat stderr && > + false > + fi && > test_cmp expect-stdout stdout && > test_cmp expect-stderr stderr > } > @@ -41,7 +51,7 @@ reject() { > echo protocol=$2 > echo host=$3 > echo username=$4 > - ) | test-credential reject $1 > + ) | git -c credential.helper=$1 credential reject > } > > helper_test() { Nice. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html