Re: [PATCH_v1] add git credential login to remote mediawiki

[roucherj <roucherj@telesun.imag.fr>]

On Sun, 10 Jun 2012 15:37:42 +0200, Matthieu Moy wrote:
> Jeff King <peff@peff.net> writes:
>
>> On Sat, Jun 09, 2012 at 08:53:48PM +0200, 
>> Javier.Roucher-Iglesias@ensimag.imag.fr wrote:
>>
>>> diff --git a/contrib/mw-to-git/git-remote-mediawiki 
>>> b/contrib/mw-to-git/git-remote-mediawiki
>>> index c18bfa1..4b14d78 100755
>>> --- a/contrib/mw-to-git/git-remote-mediawiki
>>> +++ b/contrib/mw-to-git/git-remote-mediawiki
>>> @@ -152,28 +152,111 @@ while (<STDIN>) {
>>>  ########################## Functions 
>>> ##############################
>>>
>>>  # MediaWiki API instance, created lazily.
>>> +sub run_credential {
>>
>> Is there any reason not to add this to perl/Git.pm? I suspect that 
>> other
>> scripts will want to use it, too (for example, send-email could 
>> probably
>> use it for SMTP credentials).
>
> Currently, git-remote-mediawiki is a standalone script (doesn't use
> Git.pm). This is good because it makes it trivial to install, but bad 
> in
> the sense that it may force us (or others) to reinvent the wheel.
>
> Until now, the wheels we reinvented were very simple (run_git
> essentially), but we may be reaching the point where it makes sense 
> to
> use and contribute to Git.pm.
>
> Unfortunately, from a non-technical point of view, Javier is
> contributing this as part of a student project, which ends this week,
> and it's probably not reasonable to introduce such change so late. 
> So,
> I'd keep it here at least for now, and a move to Git.pm could be a
> separate future topic.
>

Thank's for explain my situation.

>>> +	if (scalar(@_) == 2) {
>>> +		if ($_[1] eq ("store" || "cache")) {
>>> +			run_git("config credential.helper \'$_[1]\'");
>>> +		} else {
>>> +			print STDERR "ERROR: run_credential (fill|approve|reject) 
>>> [store|cache]\n";
>>> +			exit 1;
>>> +		}
>>> +	}
>>
>> This hunk looks wrong. You should never be setting the 
>> credential.helper
>> config; that is the responsibility of the user to set, as they want 
>> to
>> select whatever helper is appropriate. Nor do you need to care about
>> which helpers are in use; the point of git-credential is that it 
>> will do
>> that for you.
>
> Absolutely.
>

I have add this with no advance warning, but i will remove it in the 
next patch.

>>   sub fill_credential {
>>           my $quoted_url = quotemeta(shift);
>>
>>           my $verbatim = `git credential fill $quoted_url`;
>>           $? and die "git-credential failed";
>>
>>           $verbatim =~ /^username=(.*)$/m
>>                   or die "git-credential did not give us a 
>> username";
>>           my $username = $1;
>>           $verbatim =~ /^password=(.*)$/m
>>                   or die "git-credential did not give us a 
>> password";
>>
>>           return ($username, $password, $verbatim);
>>   }
>>
>>   sub report_credential {
>>           my ($type, $verbatim) = @_;
>>           open(my $fh, '|-', "git credential $type");
>>           print $fh $verbatim;
>>   }
>
> That sounds sensible too. We should be careful not to give a password 
> as
> argument (or users of the same machine will be able to find it with 
> e.g.
> "ps u"), but your proposal is OK with that.
>
>>> +			# error if key undef
>>> +			if (not defined $key) {
>>> +				print STDERR "ERROR reciving reponse git credential fill\n";
>>> +				exit 1;
>>> +			}
> [...]

to be change, thanks for the corrections

>>> +	} else {
>>> +		while (<Reader>) {
>>> +			print STDERR "\nERROR while running git credential $op:\n$_";
>>> +		}
>>> +	}
>>> +}
>>
>> This isn't a good way to check for errors. The non-fill actions will
>> never produce output on stdout, and you are not intercepting their
>> stderr. Besides which, checking for errors by reading stderr is not 
>> a
>> good practice; you should check the return value of the command in 
>> $?
>> after it finishes.
>
> I think it should do both. In case "git credential fill" returns
> something that doesn't match the regexp, we don't want perl to error
> with "use of undefined value", but that's just being defensive 
> because
> it shouldn't happen.