From: roucherj <roucherj@telesun.imag.fr>
Date: Sun, 10 Jun 2012 17:41:17 GMT
Subject: Re: [PATCH_v1] add 'git credential' plumbing command
Message-ID: <38899eac92a1ea5c17b98f5e7bd5d948@telesun.imag.fr>
In-Reply-To: <20120609215236.Horde.J-h4cnwdC4BP06mEUeqxRlA@webmail.minatec.grenoble-inp.fr>

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.