* [PATCH_v1] add 'git credential' plumbing command
@ 2012-06-09 18:45 Javier.Roucher-Iglesias
2012-06-09 19:52 ` konglu
` (3 more replies)
0 siblings, 4 replies; 15+ messages in thread
From: Javier.Roucher-Iglesias @ 2012-06-09 18:45 UTC (permalink / raw)
To: git
Cc: Javier Roucher, Pavel Volek, NGUYEN Kim Thuat,
ROUCHER IGLESIAS Javier, Matthieu Moy
From: Javier Roucher <jroucher@gmail.com>
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".
Signed-off-by: Pavel Volek <Pavel.Volek@ensimag.imag.fr>
Signed-off-by: NGUYEN Kim Thuat <Kim-Thuat.Nguyen@ensimag.imag.fr>
Signed-off-by: ROUCHER IGLESIAS Javier <roucherj@ensimag.imag.fr>
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
.gitignore | 1 +
Documentation/git-credential.txt | 74 ++++++++++++++++++++++++++++++++++++++++
Makefile | 1 +
builtin.h | 1 +
builtin/credential.c | 40 ++++++++++++++++++++++
git.c | 1 +
6 files changed, 118 insertions(+)
create mode 100644 Documentation/git-credential.txt
create mode 100644 builtin/credential.c
diff --git a/.gitignore b/.gitignore
index bf66648..7d1d86e 100644
--- a/.gitignore
+++ b/.gitignore
@@ -31,6 +31,7 @@
/git-commit-tree
/git-config
/git-count-objects
+/git-credential
/git-credential-cache
/git-credential-cache--daemon
/git-credential-store
diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt
new file mode 100644
index 0000000..ead23b5
--- /dev/null
+++ b/Documentation/git-credential.txt
@@ -0,0 +1,74 @@
+git-credential(7)
+=================
+
+NAME
+----
+git-credential - Providing and strore user credentials to git
+
+SYNOPSIS
+--------
+------------------
+git credential <fill|approve|reject>
+
+------------------
+
+DESCRIPTION
+-----------
+
+Git-credential permits to the user of the script to save:
+username, password, host, path and protocol. When the user of script
+invoke git-credential, the script can ask for a password, using the command
+'git credential fill'.
+Taking data from the standard input, the program treats each line as a
+separate data item, and the end of series of data item is signalled by a
+blank line.
+
+ username=admin\n
+ protocol=[http|https]\n
+ host=localhost\n
+ path=/dir\n\n
+
+-If git-credential system have the password already stored
+git-credential will answer with by STDOUT:
+
+ username=admin
+ password=*****
+
+-If it is not stored, the user will be prompt for a password:
+
+ > Password for '[http|https]admin@localhost':
+
+
+Then if the password is correct, (note: is not git credential
+how decides if password is correct or not. Is the external system
+that have to authenticate the user) it can be stored using command
+'git crendential approve' by providing the structure, by STDIN.
+
+ username=admin
+ password=*****
+ protocol=[http|https]
+ host=localhost
+ path=/dir
+
+If the password is refused, it can be deleted using command
+'git credential reject' by providing the same structure.
+
+
+REQUESTING CREDENTIALS
+----------------------
+
+1. The 'git credential fill' makes the structure,
+with this structure it will be able to save your
+credentials, and if the credential is allready stored,
+it will fill the password.
+
+ username=foo
+ password=****
+ protocol=[http|https]
+ localhost=url
+ path=/direction
+
+2. Then 'git credential approve' to store them.
+
+3. Otherwise, if the credential is not correct you can do
+ 'git credential reject' to delete the credential.
diff --git a/Makefile b/Makefile
index 4592f1f..3f53da8 100644
--- a/Makefile
+++ b/Makefile
@@ -827,6 +827,7 @@ BUILTIN_OBJS += builtin/commit-tree.o
BUILTIN_OBJS += builtin/commit.o
BUILTIN_OBJS += builtin/config.o
BUILTIN_OBJS += builtin/count-objects.o
+BUILTIN_OBJS += builtin/credential.o
BUILTIN_OBJS += builtin/describe.o
BUILTIN_OBJS += builtin/diff-files.o
BUILTIN_OBJS += builtin/diff-index.o
diff --git a/builtin.h b/builtin.h
index 338f540..48feddc 100644
--- a/builtin.h
+++ b/builtin.h
@@ -66,6 +66,7 @@ extern int cmd_commit(int argc, const char **argv, const char *prefix);
extern int cmd_commit_tree(int argc, const char **argv, const char *prefix);
extern int cmd_config(int argc, const char **argv, const char *prefix);
extern int cmd_count_objects(int argc, const char **argv, const char *prefix);
+extern int cmd_credential(int argc, const char **argv, const char *prefix);
extern int cmd_describe(int argc, const char **argv, const char *prefix);
extern int cmd_diff_files(int argc, const char **argv, const char *prefix);
extern int cmd_diff_index(int argc, const char **argv, const char *prefix);
diff --git a/builtin/credential.c b/builtin/credential.c
new file mode 100644
index 0000000..9f00885
--- /dev/null
+++ b/builtin/credential.c
@@ -0,0 +1,40 @@
+#include <stdio.h>
+#include "cache.h"
+#include "credential.h"
+#include "string-list.h"
+
+static const char usage_msg[] =
+"credential <fill|approve|reject>";
+
+void cmd_credential (int argc, char **argv, const char *prefix){
+ const char *op;
+ struct credential c = CREDENTIAL_INIT;
+ int i;
+
+ op = argv[1];
+ if (!op)
+ usage(usage_msg);
+
+ for (i = 2; i < argc; i++)
+ string_list_append(&c.helpers, argv[i]);
+
+ if (credential_read(&c, stdin) < 0)
+ die("unable to read credential from stdin");
+
+ if (!strcmp(op, "fill")) {
+ credential_fill(&c);
+ if (c.username)
+ printf("username=%s\n", c.username);
+ if (c.password)
+ printf("password=%s\n", c.password);
+ }
+ else if (!strcmp(op, "approve")) {
+ credential_approve(&c);
+ }
+ else if (!strcmp(op, "reject")) {
+ credential_reject(&c);
+ }
+ else
+ usage(usage_msg);
+}
+
diff --git a/git.c b/git.c
index d232de9..7cbd7d8 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_count_objects, RUN_SETUP },
{ "describe", cmd_describe, RUN_SETUP },
{ "diff", cmd_diff },
{ "diff-files", cmd_diff_files, RUN_SETUP | NEED_WORK_TREE },
--
1.7.11.rc2.9.ge2c5c96.dirty
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH_v1] add 'git credential' plumbing command
2012-06-09 18:45 [PATCH_v1] add 'git credential' plumbing command Javier.Roucher-Iglesias
@ 2012-06-09 19:52 ` konglu
2012-06-10 17:41 ` roucherj
2012-06-10 6:53 ` Junio C Hamano
` (2 subsequent siblings)
3 siblings, 1 reply; 15+ messages in thread
From: konglu @ 2012-06-09 19:52 UTC (permalink / raw)
To: Javier.Roucher-Iglesias
Cc: git, Javier Roucher, Pavel Volek, NGUYEN Kim Thuat,
ROUCHER IGLESIAS Javier, Matthieu Moy
Javier.Roucher-Iglesias@ensimag.imag.fr a écrit :
> +git-credential - Providing and strore user credentials to git
s/Providing/Provides/ & s/strore/store
> +-If git-credential system have the password already stored
> +git-credential will answer with by STDOUT:
s/have/has/
> +Then if the password is correct, (note: is not git credential
> +how decides if password is correct or not. Is the external system
> +that have to authenticate the user) it can be stored using command
> +'git crendential approve' by providing the structure, by STDIN.
Wouldn't the note be "it's not git credential that decides if the password is
correct or not. That part is done by the external system" ?
> +1. The 'git credential fill' makes the structure,
> +with this structure it will be able to save your
> +credentials, and if the credential is allready stored,
> +it will fill the password.
s/allready/already/
> +void cmd_credential (int argc, char **argv, const char *prefix){
> + const char *op;
> + struct credential c = CREDENTIAL_INIT;
> + int i;
> +
> + op = argv[1];
> + if (!op)
> + usage(usage_msg);
> +
> + for (i = 2; i < argc; i++)
> + string_list_append(&c.helpers, argv[i]);
> +
> + if (credential_read(&c, stdin) < 0)
> + die("unable to read credential from stdin");
> +
> + if (!strcmp(op, "fill")) {
> + credential_fill(&c);
> + if (c.username)
> + printf("username=%s\n", c.username);
> + if (c.password)
> + printf("password=%s\n", c.password);
> + }
> + else if (!strcmp(op, "approve")) {
> + credential_approve(&c);
> + }
> + else if (!strcmp(op, "reject")) {
> + credential_reject(&c);
> + }
> + else
> + usage(usage_msg);
Braces for the last "else" part. In general, the structure should be
if (...) {
/*code*/
} else if (...) {
/*code*/
} else {
/*code*/
}
If juste one block needs brances, all the other "else if"/"else" part
need it too.
BTW, please be aware of the white spaces (here mostly in the doc) :).
Lucien Kong.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH_v1] add 'git credential' plumbing command
2012-06-09 18:45 [PATCH_v1] add 'git credential' plumbing command Javier.Roucher-Iglesias
2012-06-09 19:52 ` konglu
@ 2012-06-10 6:53 ` Junio C Hamano
2012-06-10 13:16 ` Matthieu Moy
2012-06-10 11:56 ` Jeff King
2012-06-10 13:18 ` Matthieu Moy
3 siblings, 1 reply; 15+ messages in thread
From: Junio C Hamano @ 2012-06-10 6:53 UTC (permalink / raw)
To: Javier.Roucher-Iglesias
Cc: git, Javier Roucher, Pavel Volek, NGUYEN Kim Thuat,
ROUCHER IGLESIAS Javier, Matthieu Moy
Javier.Roucher-Iglesias@ensimag.imag.fr writes:
> +Git-credential permits to the user of the script to save:
> +username, password, host, path and protocol.
The above sounds like saying "A filesystem allows you to save
pathname and contents". While it may not be _wrong_ per-se, usually
you would think of a filesystem as something you store contents in;
pathname is primarily used as a key to find the contents (i.e. you
do not store it in the filesystem).
Isn't the credential mechanism for storing password for <user,
protocol, host, path> tuple (i.e. the four-tuple is used as a
look-up key)?
> diff --git a/builtin/credential.c b/builtin/credential.c
> new file mode 100644
> index 0000000..9f00885
> --- /dev/null
> +++ b/builtin/credential.c
> @@ -0,0 +1,40 @@
> +#include <stdio.h>
> +#include "cache.h"
> +#include "credential.h"
> +#include "string-list.h"
> +
> +static const char usage_msg[] =
> +"credential <fill|approve|reject>";
> +
> +void cmd_credential (int argc, char **argv, const char *prefix){
Style:
void cmd_credential(int argc, char **argv, const char *prefix)
{
> diff --git a/git.c b/git.c
> index d232de9..7cbd7d8 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_count_objects, RUN_SETUP },
Does "git credential" need to have a git repository (i.e. run in a
git repository or in a working tree that is controlled by one)? A
scripted Porcelain you would write using "git credential" may want
to implement something like "git clone" or "git ls-remote" where you
do not have to be in an existing repository.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH_v1] add 'git credential' plumbing command
2012-06-09 18:45 [PATCH_v1] add 'git credential' plumbing command Javier.Roucher-Iglesias
2012-06-09 19:52 ` konglu
2012-06-10 6:53 ` Junio C Hamano
@ 2012-06-10 11:56 ` Jeff King
2012-06-11 15:34 ` Junio C Hamano
2012-06-11 18:02 ` Matthieu Moy
2012-06-10 13:18 ` Matthieu Moy
3 siblings, 2 replies; 15+ messages in thread
From: Jeff King @ 2012-06-10 11:56 UTC (permalink / raw)
To: Javier.Roucher-Iglesias
Cc: git, Javier Roucher, Pavel Volek, NGUYEN Kim Thuat,
ROUCHER IGLESIAS Javier, Matthieu Moy
On Sat, Jun 09, 2012 at 08:45:02PM +0200, Javier.Roucher-Iglesias@ensimag.imag.fr wrote:
> +DESCRIPTION
> +-----------
> +
> +Git-credential permits to the user of the script to save:
> +username, password, host, path and protocol. When the user of script
> +invoke git-credential, the script can ask for a password, using the command
> +'git credential fill'.
I wonder if this description starts off a bit too literal, and should
start with the big picture. Tell people what it is generally, how it
fits into git, and in what circumstances they would want to run it.
Like:
Git has an internal interface for storing and retrieving credentials
from system-specific helpers, as well as prompting the user for
usernames and passwords. The git-credential command exposes this
interface to scripts which may want to retrieve, store, or prompt for
credentials in the same manner as git. The design of this scriptable
interface models the internal C API; see
link:technical/api-credentials.txt[the git credential API] for more
background on the concepts.
> +Taking data from the standard input, the program treats each line as a
> +separate data item, and the end of series of data item is signalled by a
> +blank line.
> +
> + username=admin\n
> + protocol=[http|https]\n
> + host=localhost\n
> + path=/dir\n\n
It's nice to have an example like this, but there's much detail missing
in how the format is specified. However, this format is already
documented in the "helpers" section of api-credentials.txt, so it
probably makes sense to refer to that document.
> +-If git-credential system have the password already stored
> +git-credential will answer with by STDOUT:
> +
> + username=admin
> + password=*****
> +
> +-If it is not stored, the user will be prompt for a password:
> +
> + > Password for '[http|https]admin@localhost':
I'd rather see this broken up into a reference specification and
separate examples. Then the reference part can be very specific about
what happens and the behavior in each case. For example:
git-credential takes an "action" option on the command-line (one of
"fill", "approve", or "reject") and reads a credential description
on stdin. The format of the description is the same as that given to
credential helpers; see link:technical/api-credential.txt[the git
credential API] for a specification.
If the action is "fill", git-credential will attempt to add "username"
and "password" fields to the description by reading config files, by
contacting any configured credential helpers, or by prompting the
user. The username and password fields of the credential description
are then printed to stdout.
If the action is "approve", git-credential will send the description
to any configured credential helpers, which may store the credential
for later use.
If the action is "reject", git-credential will send the description to
any configured credential helpers, which may erase any stored
credential matching the description.
And now that we've introduced the actions and said what they've done, we
can go on to describe the expected workflow, which is a good place to
put examples:
An application using git-credential will typically follow this
workflow:
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:
protocol=https
host=example.com
path=foo.git
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
3. Try to use the credential (e.g., by accessing the URL with the
username and password from step (2)).
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 from step (1) concatenated with the attempted
credential (i.e., the output you got in step (2)).
Note in the above proposed documentation I was trying to describe the
behavior of the command in your patch. It does not reflect changes which
I will propose to the command's behavior below. :)
> diff --git a/builtin/credential.c b/builtin/credential.c
> new file mode 100644
> index 0000000..9f00885
> --- /dev/null
> +++ b/builtin/credential.c
> @@ -0,0 +1,40 @@
> +#include <stdio.h>
> +#include "cache.h"
> +#include "credential.h"
> +#include "string-list.h"
> +
> +static const char usage_msg[] =
> +"credential <fill|approve|reject>";
> +
> +void cmd_credential (int argc, char **argv, const char *prefix){
> + const char *op;
> + struct credential c = CREDENTIAL_INIT;
> + int i;
> +
> + op = argv[1];
> + if (!op)
> + usage(usage_msg);
> +
> + for (i = 2; i < argc; i++)
> + string_list_append(&c.helpers, argv[i]);
So arguments past the first one become helpers that we try, regardless
of the credential.helper config settings. Is there a reason for this to
be in the user-facing command?
I assume this got copied by looking at test-credential. I'd be OK with
including this feature in git-credential (and converting our test
scripts to use it, so we can drop test-credential entirely). But
probably it should not soak up all of the command-line arguments, and
instead should be a hidden option like:
git credential --helper=cache fill
That will give us more flexibility later down the road.
> + if (credential_read(&c, stdin) < 0)
> + die("unable to read credential from stdin");
I think we want to provide a straight credential-reader like this,
because it is the most flexible form of describing a credential. But I
suspect many callers will have a URL, and we are creating extra work for
them to break it down into components themselves.
Perhaps it would be simpler to accept a URL on the command line, and
also provide a --stdin option for callers that want to feed it directly.
So:
git credential fill https://example.com/foo.git
would be identical to:
git credential --stdin fill <<\EOF
protocol=https
host=example.com
path=foo.git
EOF
> + if (!strcmp(op, "fill")) {
> + credential_fill(&c);
> + if (c.username)
> + printf("username=%s\n", c.username);
> + if (c.password)
> + printf("password=%s\n", c.password);
> + }
I am tempted to suggest that this actually output the _whole_
credential, not just the username and password. Coupled with the above
behavior, you would get:
$ git credential fill https://example.com/foo.git
protocol=https
host=example.com
path=foo.git
username=bob
password=secr3t
which happens to be exactly what you want to feed back to the "approve"
and "reject" actions (and it is not really any harder to parse).
We _could_ get by with allowing:
git credential --stdin approve https://example.com/foo.git <<\EOF
username=bob
password=secr3t
EOF
and having it combine the URL on the command-line with the entries on
stdin (and indeed, I think that is the only sane thing to do when
--stdin and a URL are both given).
But that implies that there will never be any extra attributes that are
relevant to the approve/reject that are not in the URL (e.g., elsewhere
there has been talk about helpers being able to add arbitrary fields to
the descriptions in order to communicate with each other). So it may be
that we want to encourage callers to save the result of "fill" and feed
it back to "approve" or "reject" verbatim.
-Peff
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH_v1] add 'git credential' plumbing command
2012-06-10 6:53 ` Junio C Hamano
@ 2012-06-10 13:16 ` Matthieu Moy
2012-06-10 20:32 ` Jonathan Nieder
0 siblings, 1 reply; 15+ messages in thread
From: Matthieu Moy @ 2012-06-10 13:16 UTC (permalink / raw)
To: Junio C Hamano
Cc: Javier.Roucher-Iglesias, git, Javier Roucher, Pavel Volek,
NGUYEN Kim Thuat, ROUCHER IGLESIAS Javier
Junio C Hamano <gitster@pobox.com> writes:
>> diff --git a/git.c b/git.c
>> index d232de9..7cbd7d8 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_count_objects, RUN_SETUP },
>
> Does "git credential" need to have a git repository (i.e. run in a
> git repository or in a working tree that is controlled by one)?
It shouldn't (hence, should use RUN_SETUP_GENTLY).
> A scripted Porcelain you would write using "git credential" may want
> to implement something like "git clone" or "git ls-remote" where you
> do not have to be in an existing repository.
("git clone" might not be the best example, as the authentication is
usually done in the "git fetch" part of "git clone", but never mind)
Actually, "git credential" has very little to do with Git, and could
even be used in a git-unrelated script.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH_v1] add 'git credential' plumbing command
2012-06-09 18:45 [PATCH_v1] add 'git credential' plumbing command Javier.Roucher-Iglesias
` (2 preceding siblings ...)
2012-06-10 11:56 ` Jeff King
@ 2012-06-10 13:18 ` Matthieu Moy
3 siblings, 0 replies; 15+ messages in thread
From: Matthieu Moy @ 2012-06-10 13:18 UTC (permalink / raw)
To: Javier.Roucher-Iglesias
Cc: git, Javier Roucher, Pavel Volek, NGUYEN Kim Thuat,
ROUCHER IGLESIAS Javier
Javier.Roucher-Iglesias@ensimag.imag.fr writes:
> +static const char usage_msg[] =
> +"credential <fill|approve|reject>";
[...]
> + for (i = 2; i < argc; i++)
> + string_list_append(&c.helpers, argv[i]);
This helpers argument is still there, and is now totally undocumented. I
shouldn't have to repeat that so many times.
> --- 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_count_objects, RUN_SETUP },
Your "git credential" runs cmd_count_objects. A little bit of testing
should have found that (my remarks about lack of testing still apply).
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH_v1] add 'git credential' plumbing command
2012-06-09 19:52 ` konglu
@ 2012-06-10 17:41 ` roucherj
2012-06-10 18:12 ` Matthieu Moy
0 siblings, 1 reply; 15+ messages in thread
From: roucherj @ 2012-06-10 17:41 UTC (permalink / raw)
To: konglu
Cc: Javier.Roucher-Iglesias, git, Javier Roucher, Pavel Volek,
NGUYEN Kim Thuat, ROUCHER IGLESIAS Javier, Matthieu Moy
On Sat, 09 Jun 2012 21:52:36 +0200, konglu@minatec.inpg.fr wrote:
> Javier.Roucher-Iglesias@ensimag.imag.fr a écrit :
>
>> +git-credential - Providing and strore user credentials to git
>
> s/Providing/Provides/ & s/strore/store
>
>> +-If git-credential system have the password already stored
>> +git-credential will answer with by STDOUT:
>
> s/have/has/
>
>> +Then if the password is correct, (note: is not git credential
>> +how decides if password is correct or not. Is the external system
>> +that have to authenticate the user) it can be stored using command
>> +'git crendential approve' by providing the structure, by STDIN.
>
> Wouldn't the note be "it's not git credential that decides if the
> password is
> correct or not. That part is done by the external system" ?
>
>> +1. The 'git credential fill' makes the structure,
>> +with this structure it will be able to save your
>> +credentials, and if the credential is allready stored,
>> +it will fill the password.
>
> s/allready/already/
>
Thank's for the english writting corrections
it will be change it for the next patch
>> +void cmd_credential (int argc, char **argv, const char *prefix){
>> + const char *op;
>> + struct credential c = CREDENTIAL_INIT;
>> + int i;
>> +
>> + op = argv[1];
>> + if (!op)
>> + usage(usage_msg);
>> +
>> + for (i = 2; i < argc; i++)
>> + string_list_append(&c.helpers, argv[i]);
>> +
>> + if (credential_read(&c, stdin) < 0)
>> + die("unable to read credential from stdin");
>> +
>> + if (!strcmp(op, "fill")) {
>> + credential_fill(&c);
>> + if (c.username)
>> + printf("username=%s\n", c.username);
>> + if (c.password)
>> + printf("password=%s\n", c.password);
>> + }
>> + else if (!strcmp(op, "approve")) {
>> + credential_approve(&c);
>> + }
>> + else if (!strcmp(op, "reject")) {
>> + credential_reject(&c);
>> + }
>> + else
>> + usage(usage_msg);
>
> Braces for the last "else" part. In general, the structure should be
>
> if (...) {
> /*code*/
> } else if (...) {
> /*code*/
> } else {
> /*code*/
> }
>
> If juste one block needs brances, all the other "else if"/"else" part
> need it too.
>
> BTW, please be aware of the white spaces (here mostly in the doc) :).
>
> Lucien Kong.
I will remove brances.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH_v1] add 'git credential' plumbing command
2012-06-10 17:41 ` roucherj
@ 2012-06-10 18:12 ` Matthieu Moy
0 siblings, 0 replies; 15+ messages in thread
From: Matthieu Moy @ 2012-06-10 18:12 UTC (permalink / raw)
To: roucherj
Cc: konglu, Javier.Roucher-Iglesias, git, Javier Roucher,
Pavel Volek, NGUYEN Kim Thuat, ROUCHER IGLESIAS Javier
roucherj <roucherj@telesun.imag.fr> writes:
> On Sat, 09 Jun 2012 21:52:36 +0200, konglu@minatec.inpg.fr wrote:
>>> +void cmd_credential (int argc, char **argv, const char *prefix){
>>> + const char *op;
>>> + struct credential c = CREDENTIAL_INIT;
>>> + int i;
>>> +
>>> + op = argv[1];
>>> + if (!op)
>>> + usage(usage_msg);
>>> +
>>> + for (i = 2; i < argc; i++)
>>> + string_list_append(&c.helpers, argv[i]);
>>> +
>>> + if (credential_read(&c, stdin) < 0)
>>> + die("unable to read credential from stdin");
>>> +
>>> + if (!strcmp(op, "fill")) {
>>> + credential_fill(&c);
>>> + if (c.username)
>>> + printf("username=%s\n", c.username);
>>> + if (c.password)
>>> + printf("password=%s\n", c.password);
>>> + }
>>> + else if (!strcmp(op, "approve")) {
>>> + credential_approve(&c);
>>> + }
>>> + else if (!strcmp(op, "reject")) {
>>> + credential_reject(&c);
>>> + }
>>> + else
>>> + usage(usage_msg);
>>
>> Braces for the last "else" part. In general, the structure should be
>>
>> if (...) {
>> /*code*/
>> } else if (...) {
>> /*code*/
>> } else {
>> /*code*/
>> }
>>
>> If juste one block needs brances, all the other "else if"/"else" part
>> need it too.
>>
>> BTW, please be aware of the white spaces (here mostly in the doc) :).
>>
>> Lucien Kong.
>
> I will remove brances.
The remark was about adding them, not removing them. There's one branch
of the if/else if/ with several instructions, so we usually put braces
everywhere.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH_v1] add 'git credential' plumbing command
2012-06-10 13:16 ` Matthieu Moy
@ 2012-06-10 20:32 ` Jonathan Nieder
2012-06-10 21:02 ` Jonathan Nieder
0 siblings, 1 reply; 15+ messages in thread
From: Jonathan Nieder @ 2012-06-10 20:32 UTC (permalink / raw)
To: Matthieu Moy
Cc: Junio C Hamano, Javier.Roucher-Iglesias, git, Javier Roucher,
Pavel Volek, NGUYEN Kim Thuat, ROUCHER IGLESIAS Javier
Matthieu Moy wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>> Does "git credential" need to have a git repository (i.e. run in a
>> git repository or in a working tree that is controlled by one)?
>
> It shouldn't (hence, should use RUN_SETUP_GENTLY).
Rather, that means it should use 0:
{ "credential", cmd_credential },
[...]
> Actually, "git credential" has very little to do with Git, and could
> even be used in a git-unrelated script.
I suspect it would be simplest to make it a non-builtin. There are
some examples to take inspiration from in the PROGRAM_OBJS variable of
the Makefile.
Hope that helps,
Jonathan
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH_v1] add 'git credential' plumbing command
2012-06-10 20:32 ` Jonathan Nieder
@ 2012-06-10 21:02 ` Jonathan Nieder
2012-06-11 15:35 ` Junio C Hamano
0 siblings, 1 reply; 15+ messages in thread
From: Jonathan Nieder @ 2012-06-10 21:02 UTC (permalink / raw)
To: Matthieu Moy
Cc: Junio C Hamano, Javier.Roucher-Iglesias, git, Javier Roucher,
Pavel Volek, NGUYEN Kim Thuat, ROUCHER IGLESIAS Javier
Jonathan Nieder wrote:
> Matthieu Moy wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>>> Does "git credential" need to have a git repository (i.e. run in a
>>> git repository or in a working tree that is controlled by one)?
>>
>> It shouldn't (hence, should use RUN_SETUP_GENTLY).
>
> Rather, that means it should use 0:
>
> { "credential", cmd_credential },
... and it turns out I'm talking nonsense. RUN_SETUP_GENTLY would
be a sensible choice indeed, to allow the command to discover the
current repository and read .git/config from there indeed.
Sorry for the confusion,
Jonathan
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH_v1] add 'git credential' plumbing command
2012-06-10 11:56 ` Jeff King
@ 2012-06-11 15:34 ` Junio C Hamano
2012-06-11 15:45 ` Jeff King
2012-06-11 18:02 ` Matthieu Moy
1 sibling, 1 reply; 15+ messages in thread
From: Junio C Hamano @ 2012-06-11 15:34 UTC (permalink / raw)
To: Jeff King
Cc: Javier.Roucher-Iglesias, git, Javier Roucher, Pavel Volek,
NGUYEN Kim Thuat, ROUCHER IGLESIAS Javier, Matthieu Moy
Jeff King <peff@peff.net> writes:
> Perhaps it would be simpler to accept a URL on the command line, and
> also provide a --stdin option for callers that want to feed it directly.
> So:
>
> git credential fill https://example.com/foo.git
>
> would be identical to:
>
> git credential --stdin fill <<\EOF
> protocol=https
> host=example.com
> path=foo.git
> EOF
and "git credential fill https://peff@example.com/foo.git" would be
identical to the latter one with user=peff already filled in?
> I am tempted to suggest that this actually output the _whole_
> credential, not just the username and password. Coupled with the above
> behavior, you would get:
>
> $ git credential fill https://example.com/foo.git
> protocol=https
> host=example.com
> path=foo.git
> username=bob
> password=secr3t
>
> which happens to be exactly what you want to feed back to the "approve"
> and "reject" actions (and it is not really any harder to parse).
>
> We _could_ get by with allowing:
>
> git credential --stdin approve https://example.com/foo.git <<\EOF
> username=bob
> password=secr3t
> EOF
>
> and having it combine the URL on the command-line with the entries on
> stdin (and indeed, I think that is the only sane thing to do when
> --stdin and a URL are both given).
All good suggestions ;-).
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH_v1] add 'git credential' plumbing command
2012-06-10 21:02 ` Jonathan Nieder
@ 2012-06-11 15:35 ` Junio C Hamano
0 siblings, 0 replies; 15+ messages in thread
From: Junio C Hamano @ 2012-06-11 15:35 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Matthieu Moy, Javier.Roucher-Iglesias, git, Javier Roucher,
Pavel Volek, NGUYEN Kim Thuat, ROUCHER IGLESIAS Javier
Jonathan Nieder <jrnieder@gmail.com> writes:
> Jonathan Nieder wrote:
>> Matthieu Moy wrote:
>>> Junio C Hamano <gitster@pobox.com> writes:
>
>>>> Does "git credential" need to have a git repository (i.e. run in a
>>>> git repository or in a working tree that is controlled by one)?
>>>
>>> It shouldn't (hence, should use RUN_SETUP_GENTLY).
>>
>> Rather, that means it should use 0:
>>
>> { "credential", cmd_credential },
>
> ... and it turns out I'm talking nonsense. RUN_SETUP_GENTLY would
> be a sensible choice indeed, to allow the command to discover the
> current repository and read .git/config from there indeed.
Yeah, I think "it is OK to be called outside a repository, but if
inside we read and honor the configuration" is a sensible behaviour
for this command.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH_v1] add 'git credential' plumbing command
2012-06-11 15:34 ` Junio C Hamano
@ 2012-06-11 15:45 ` Jeff King
0 siblings, 0 replies; 15+ messages in thread
From: Jeff King @ 2012-06-11 15:45 UTC (permalink / raw)
To: Junio C Hamano
Cc: Javier.Roucher-Iglesias, git, Javier Roucher, Pavel Volek,
NGUYEN Kim Thuat, ROUCHER IGLESIAS Javier, Matthieu Moy
On Mon, Jun 11, 2012 at 08:34:55AM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > Perhaps it would be simpler to accept a URL on the command line, and
> > also provide a --stdin option for callers that want to feed it directly.
> > So:
> >
> > git credential fill https://example.com/foo.git
> >
> > would be identical to:
> >
> > git credential --stdin fill <<\EOF
> > protocol=https
> > host=example.com
> > path=foo.git
> > EOF
>
> and "git credential fill https://peff@example.com/foo.git" would be
> identical to the latter one with user=peff already filled in?
Exactly. Though see my other response to Matthieu, which notes that:
git credential fill https://peff:supersecret@example.com/foo.git
is problematic. :(
Probably it should be spelled:
git credential fill <<\EOF
url=https://peff:supersecret@example.com/foo.git
EOF
and then:
git credential fill <<\EOF
url=https://peff:supersecret@example.com/foo.git
username=junio
password=othersecret
would do what you expect (break the URL out into its components, and
then override particular fields).
-Peff
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH_v1] add 'git credential' plumbing command
2012-06-10 11:56 ` Jeff King
2012-06-11 15:34 ` Junio C Hamano
@ 2012-06-11 18:02 ` Matthieu Moy
2012-06-11 18:18 ` Jeff King
1 sibling, 1 reply; 15+ messages in thread
From: Matthieu Moy @ 2012-06-11 18:02 UTC (permalink / raw)
To: Jeff King
Cc: Javier.Roucher-Iglesias, git, Javier Roucher, Pavel Volek,
NGUYEN Kim Thuat, ROUCHER IGLESIAS Javier
Jeff King <peff@peff.net> writes:
> It's nice to have an example like this, but there's much detail missing
> in how the format is specified. However, this format is already
> documented in the "helpers" section of api-credentials.txt, so it
> probably makes sense to refer to that document.
I'd do it the other way around. api-credentials.txt is in technical/,
while the document we're writing will end-up in a man page, which cannot
link to technical/.
So, it makes more sense to move the format specification to
git-credential.txt, and link to it from api-credentials.txt (now that we
have a nice way to link to manpages from technical/ ;-) ).
> I assume this got copied by looking at test-credential. I'd be OK with
> including this feature in git-credential (and converting our test
> scripts to use it, so we can drop test-credential entirely). But
> probably it should not soak up all of the command-line arguments, and
> instead should be a hidden option like:
>
> git credential --helper=cache fill
>
> That will give us more flexibility later down the road.
Actually, this should already be possible with
git -c credential.helper=cache credential fill
I suspect that this feature will never be used outside tests, and if so,
I don't think it deserves a command-line option.
> I am tempted to suggest that this actually output the _whole_
> credential, not just the username and password. Coupled with the above
> behavior, you would get:
>
> $ git credential fill https://example.com/foo.git
> protocol=https
> host=example.com
> path=foo.git
> username=bob
> password=secr3t
>
> which happens to be exactly what you want to feed back to the "approve"
> and "reject" actions (and it is not really any harder to parse).
I like that, yes.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH_v1] add 'git credential' plumbing command
2012-06-11 18:02 ` Matthieu Moy
@ 2012-06-11 18:18 ` Jeff King
0 siblings, 0 replies; 15+ messages in thread
From: Jeff King @ 2012-06-11 18:18 UTC (permalink / raw)
To: Matthieu Moy
Cc: Javier.Roucher-Iglesias, git, Javier Roucher, Pavel Volek,
NGUYEN Kim Thuat, ROUCHER IGLESIAS Javier
On Mon, Jun 11, 2012 at 08:02:06PM +0200, Matthieu Moy wrote:
> Jeff King <peff@peff.net> writes:
>
> > It's nice to have an example like this, but there's much detail missing
> > in how the format is specified. However, this format is already
> > documented in the "helpers" section of api-credentials.txt, so it
> > probably makes sense to refer to that document.
>
> I'd do it the other way around. api-credentials.txt is in technical/,
> while the document we're writing will end-up in a man page, which cannot
> link to technical/.
We do so already in a few places:
$ cd Documentation && git grep 'link:technical/'
git.txt:link:technical/api-index.html[GIT API documentation].
gitcredentials.txt:link:technical/api-credentials.html[credentials API] for details.
user-manual.txt:found in link:technical/pack-format.txt[technical/pack-format.txt].
I think the rationale is that you would have the HTML documentation
installed into /usr/share/doc/git-doc or similar, and asciidoc does
correctly generate footnote references from those links. However, I
would be fine with putting the meat of it into git-credential, and
having api-credentials refer back to it. It's easier on the user that
way.
> > git credential --helper=cache fill
> >
> > That will give us more flexibility later down the road.
>
> Actually, this should already be possible with
>
> git -c credential.helper=cache credential fill
>
> I suspect that this feature will never be used outside tests, and if so,
> I don't think it deserves a command-line option.
Yeah, that is even better. The tests could also use test_config. I
would pick whichever of the two is more convenient for a particular
test.
-Peff
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2012-06-11 18:18 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-06-09 18:45 [PATCH_v1] add 'git credential' plumbing command Javier.Roucher-Iglesias
2012-06-09 19:52 ` konglu
2012-06-10 17:41 ` roucherj
2012-06-10 18:12 ` Matthieu Moy
2012-06-10 6:53 ` Junio C Hamano
2012-06-10 13:16 ` Matthieu Moy
2012-06-10 20:32 ` Jonathan Nieder
2012-06-10 21:02 ` Jonathan Nieder
2012-06-11 15:35 ` Junio C Hamano
2012-06-10 11:56 ` Jeff King
2012-06-11 15:34 ` Junio C Hamano
2012-06-11 15:45 ` Jeff King
2012-06-11 18:02 ` Matthieu Moy
2012-06-11 18:18 ` Jeff King
2012-06-10 13:18 ` Matthieu Moy
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).