{"thread":{"id":"30753","subject":"[PATCH_v1] add git credential login to remote mediawiki","startedAt":"2012-06-09T18:53:48Z","lastAt":"2012-06-10T18:39:21Z","messageCount":6,"participants":["javier.roucher-iglesias@ensimag.imag.fr","Junio C Hamano","Jeff King","Matthieu Moy","roucherj"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"193211","messageId":"1339268028-13991-1-git-send-email-Javier.Roucher-Iglesias@ensimag.imag.fr","threadId":"30753","inReplyTo":null,"subject":"[PATCH_v1] add git credential login to remote mediawiki","fromName":"","fromEmail":"javier.roucher-iglesias@ensimag.imag.fr","sentAt":"2012-06-09T18:53:48Z","receivedAt":"2012-06-09T18:53:48Z","isPatch":false,"sender":{"key":"javier.roucher-iglesias@ensimag.imag.fr","avatar":null},"body":"From: Javier Roucher \n\n\nThis path uses git credential to store the login/password of the mediawiki.\n\n\nSigned-off-by: Pavel Volek \nSigned-off-by: NGUYEN Kim Thuat \nSigned-off-by: ROUCHER IGLESIAS Javier \nSigned-off-by: Matthieu Moy \n---\n contrib/mw-to-git/git-remote-mediawiki | 107 +++++++++++++++++++++++++++++----\n 1 file changed, 95 insertions(+), 12 deletions(-)\n\ndiff --git a/contrib/mw-to-git/git-remote-mediawiki b/contrib/mw-to-git/git-remote-mediawiki\nindex c18bfa1..4b14d78 100755\n--- a/contrib/mw-to-git/git-remote-mediawiki\n+++ b/contrib/mw-to-git/git-remote-mediawiki\n@@ -152,28 +152,111 @@ while () {\n ########################## Functions ##############################\n \n # MediaWiki API instance, created lazily.\n+sub run_credential {\n+\tmy $cre_protocol = \"\";\n+\tmy $cre_host = \"\";\n+\tmy $cre_path = \"\";\n+\tmy $msg = \"\";\n+\tmy $result = \"\";\n+\tmy $op=$_[0];\n+\tif (scalar(@_) == 2) {\n+\t\tif ($_[1] eq (\"store\" || \"cache\")) {\n+\t\t\trun_git(\"config credential.helper \\'$_[1]\\'\");\n+\t\t} else {\n+\t\t\tprint STDERR \"ERROR: run_credential (fill|approve|reject) [store|cache]\\n\";\n+\t\t\texit 1;\n+\t\t}\n+\t}\n+\tmy $parsed = URI->new($url);\n+\t$cre_protocol = $parsed->scheme;\n+\t$cre_host = $parsed->host;\n+\t$cre_path = $parsed->path;\n+\n+ if ($wiki_login ne \"\") {\n+ $msg .= \"username=$wiki_login\\n\";\n+ }\n+ if ($wiki_passwd ne \"\") {\n+ $msg .= \"password=$wiki_passwd\\n\";\n+ }\n+ if ($cre_protocol ne \"\") {\n+ $msg .= \"protocol=$cre_protocol\\n\";\n+ }\n+ if ($cre_host ne \"\") {\n+ $msg .= \"host=$cre_host\\n\";\n+ }\n+ if ($cre_path ne \"\") {\n+ $msg .= \"path=$cre_path\\n\";\n+ }\n+\n+ $msg .= \"\\n\";\n+\n+\tmy $key;\n+\tmy $value;\n+\tmy $Prog = \"git credential $op\";\n+\topen2(*Reader, *Writer, $Prog);\n+\tprint Writer $msg;\n+\tclose (Writer);\n+\n+\tif ($op eq \"fill\") {\n+\t\twhile () {\n+\t\t\tmy ($key, $value) = /([^=]*)=(.*)/;\n+\t\t\t# error if key undef\n+\t\t\tif (not defined $key) {\n+\t\t\t\tprint STDERR \"ERROR reciving reponse git credential fill\\n\";\n+\t\t\t\texit 1;\n+\t\t\t}\n+\t\t\tif ($key eq \"username\") {\n+\t\t\t\t$wiki_login = $value;\n+\t\t\t}\n+\t\t\tif ($key eq \"password\") {\n+\t\t\t\t$wiki_passwd = $value;\n+\t\t\t}\n+\t\t}\n+\t} else {\n+\t\twhile () {\n+\t\t\tprint STDERR \"\\nERROR while running git credential $op:\\n$_\";\n+\t\t}\n+\t}\n+}\n+\n my $mediawiki;\n \n+sub ask_login {\n+\trun_credential(\"fill\",\"store\");\n+\n+\tif (!$mediawiki->login( {\n+\t\tlgname => $wiki_login,\n+\t\tlgpassword => $wiki_passwd,\n+\t\tlgdomain => $wiki_domain,\n+\t\t} )) {\n+\t\t\tprint STDERR \"Failed to log in mediawiki user \\\"$wiki_login\\\" on $url\\n\";\n+\t\t\tprint STDERR \"URL:$wiki_domain $url\\n\";\n+\t\t\tprint STDERR \"(error \" .\n+\t\t\t $mediawiki->{error}->{code} . ': ' .\n+\t\t\t $mediawiki->{error}->{details} . \")\\n\";\n+\t\t\trun_credential(\"reject\");\n+\t#\t\texit 1;\n+\t} else {\n+\t\tprint STDERR \"Logged in with user \\\"$wiki_login\\\".\\n\";\n+\t\trun_credential(\"approve\");\n+\t}\n+}\n+\n sub mw_connect_maybe {\n+\n \tif ($mediawiki) {\n \t return;\n \t}\n \t$mediawiki = MediaWiki::API->new;\n \t$mediawiki->{config}->{api_url} = \"$url/api.php\";\n \tif ($wiki_login) {\n-\t\tif (!$mediawiki->login({\n-\t\t\tlgname => $wiki_login,\n-\t\t\tlgpassword => $wiki_passwd,\n-\t\t\tlgdomain => $wiki_domain,\n-\t\t})) {\n-\t\t\tprint STDERR \"Failed to log in mediawiki user \\\"$wiki_login\\\" on $url\\n\";\n-\t\t\tprint STDERR \"(error \" .\n-\t\t\t $mediawiki->{error}->{code} . ': ' .\n-\t\t\t $mediawiki->{error}->{details} . \")\\n\";\n-\t\t\texit 1;\n-\t\t} else {\n-\t\t\tprint STDERR \"Logged in with user \\\"$wiki_login\\\".\\n\";\n+\t\tif (!$wiki_passwd) {\n+\t\t\t#user knows, password not.\n+\t\t\task_login();\n \t\t}\n+\t} else \t{\n+\t\t#user or password not knows\n+\t\task_login();\n \t}\n }\n \n-- \n1.7.11.rc2.9.ge2c5c96.dirty\n"},{"id":"193228","messageId":"7vvcizac8e.fsf@alter.siamese.dyndns.org","threadId":"30753","inReplyTo":"1339268028-13991-1-git-send-email-Javier.Roucher-Iglesias@ensimag.imag.fr","subject":"Re: [PATCH_v1] add git credential login to remote mediawiki","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-06-10T06:54:09Z","receivedAt":"2012-06-10T06:54:09Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Javier.Roucher-Iglesias@ensimag.imag.fr writes:\n\n> Subject: [PATCH_v1] add git credential login to remote mediawiki\n\nPlease remove \"_\" between \"PATCH\" and \"v1\".\n"},{"id":"193251","messageId":"20120610121827.GB6453@sigill.intra.peff.net","threadId":"30753","inReplyTo":"1339268028-13991-1-git-send-email-Javier.Roucher-Iglesias@ensimag.imag.fr","subject":"Re: [PATCH_v1] add git credential login to remote mediawiki","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-06-10T12:18:28Z","receivedAt":"2012-06-10T12:18:28Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jun 09, 2012 at 08:53:48PM +0200, Javier.Roucher-Iglesias@ensimag.imag.fr wrote:\n\n> diff --git a/contrib/mw-to-git/git-remote-mediawiki b/contrib/mw-to-git/git-remote-mediawiki\n> index c18bfa1..4b14d78 100755\n> --- a/contrib/mw-to-git/git-remote-mediawiki\n> +++ b/contrib/mw-to-git/git-remote-mediawiki\n> @@ -152,28 +152,111 @@ while () {\n> ########################## Functions ##############################\n> \n> # MediaWiki API instance, created lazily.\n> +sub run_credential {\n\nIs there any reason not to add this to perl/Git.pm? I suspect that other\nscripts will want to use it, too (for example, send-email could probably\nuse it for SMTP credentials).\n\n> +\tif (scalar(@_) == 2) {\n> +\t\tif ($_[1] eq (\"store\" || \"cache\")) {\n> +\t\t\trun_git(\"config credential.helper \\'$_[1]\\'\");\n> +\t\t} else {\n> +\t\t\tprint STDERR \"ERROR: run_credential (fill|approve|reject) [store|cache]\\n\";\n> +\t\t\texit 1;\n> +\t\t}\n> +\t}\n\nThis hunk looks wrong. You should never be setting the credential.helper\nconfig; that is the responsibility of the user to set, as they want to\nselect whatever helper is appropriate. Nor do you need to care about\nwhich helpers are in use; the point of git-credential is that it will do\nthat for you.\n\n> +\tmy $parsed = URI->new($url);\n> +\t$cre_protocol = $parsed->scheme;\n> +\t$cre_host = $parsed->host;\n> +\t$cre_path = $parsed->path;\n> +\n> + if ($wiki_login ne \"\") {\n> + $msg .= \"username=$wiki_login\\n\";\n> + }\n> + if ($wiki_passwd ne \"\") {\n> + $msg .= \"password=$wiki_passwd\\n\";\n> + }\n> + if ($cre_protocol ne \"\") {\n> + $msg .= \"protocol=$cre_protocol\\n\";\n> + }\n> + if ($cre_host ne \"\") {\n> + $msg .= \"host=$cre_host\\n\";\n> + }\n> + if ($cre_path ne \"\") {\n> + $msg .= \"path=$cre_path\\n\";\n> + }\n> +\n> + $msg .= \"\\n\";\n\nAll of this could just go away for the \"fill\" case if we allow URLs on\nthe command line (see my previous email if you haven't already). And for\nthe \"approve\" and \"reject\" cases, we could just save the result from\n\"fill\" and feed it back verbatim, as I described in the earlier email.\n\nThen it would be as simple as:\n\n sub fill_credential {\n my $quoted_url = quotemeta(shift);\n\n my $verbatim = `git credential fill $quoted_url`;\n $? and die \"git-credential failed\";\n\n $verbatim =~ /^username=(.*)$/m\n or die \"git-credential did not give us a username\";\n my $username = $1;\n $verbatim =~ /^password=(.*)$/m\n or die \"git-credential did not give us a password\";\n\n return ($username, $password, $verbatim);\n }\n\n sub report_credential {\n my ($type, $verbatim) = @_;\n open(my $fh, '|-', \"git credential $type\");\n print $fh $verbatim;\n }\n\n> +\tmy $key;\n> +\tmy $value;\n> +\tmy $Prog = \"git credential $op\";\n> +\topen2(*Reader, *Writer, $Prog);\n> +\tprint Writer $msg;\n> +\tclose (Writer);\n> +\n> +\tif ($op eq \"fill\") {\n> +\t\twhile () {\n> +\t\t\tmy ($key, $value) = /([^=]*)=(.*)/;\n> +\t\t\t# error if key undef\n> +\t\t\tif (not defined $key) {\n> +\t\t\t\tprint STDERR \"ERROR reciving reponse git credential fill\\n\";\n> +\t\t\t\texit 1;\n> +\t\t\t}\n> +\t\t\tif ($key eq \"username\") {\n> +\t\t\t\t$wiki_login = $value;\n> +\t\t\t}\n> +\t\t\tif ($key eq \"password\") {\n> +\t\t\t\t$wiki_passwd = $value;\n> +\t\t\t}\n> +\t\t}\n> +\t} else {\n> +\t\twhile () {\n> +\t\t\tprint STDERR \"\\nERROR while running git credential $op:\\n$_\";\n> +\t\t}\n> +\t}\n> +}\n\nThis isn't a good way to check for errors. The non-fill actions will\nnever produce output on stdout, and you are not intercepting their\nstderr. Besides which, checking for errors by reading stderr is not a\ngood practice; you should check the return value of the command in $?\nafter it finishes.\n\n-Peff\n"},{"id":"193261","messageId":"vpqaa0bmgnt.fsf@bauges.imag.fr","threadId":"30753","inReplyTo":"20120610121827.GB6453@sigill.intra.peff.net","subject":"Re: [PATCH_v1] add git credential login to remote mediawiki","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2012-06-10T13:37:42Z","receivedAt":"2012-06-10T13:37:42Z","isPatch":false,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Jeff King writes:\n\n> On Sat, Jun 09, 2012 at 08:53:48PM +0200, Javier.Roucher-Iglesias@ensimag.imag.fr wrote:\n>\n>> diff --git a/contrib/mw-to-git/git-remote-mediawiki b/contrib/mw-to-git/git-remote-mediawiki\n>> index c18bfa1..4b14d78 100755\n>> --- a/contrib/mw-to-git/git-remote-mediawiki\n>> +++ b/contrib/mw-to-git/git-remote-mediawiki\n>> @@ -152,28 +152,111 @@ while () {\n>> ########################## Functions ##############################\n>> \n>> # MediaWiki API instance, created lazily.\n>> +sub run_credential {\n>\n> Is there any reason not to add this to perl/Git.pm? I suspect that other\n> scripts will want to use it, too (for example, send-email could probably\n> use it for SMTP credentials).\n\nCurrently, git-remote-mediawiki is a standalone script (doesn't use\nGit.pm). This is good because it makes it trivial to install, but bad in\nthe sense that it may force us (or others) to reinvent the wheel.\n\nUntil now, the wheels we reinvented were very simple (run_git\nessentially), but we may be reaching the point where it makes sense to\nuse and contribute to Git.pm.\n\nUnfortunately, from a non-technical point of view, Javier is\ncontributing this as part of a student project, which ends this week,\nand it's probably not reasonable to introduce such change so late. So,\nI'd keep it here at least for now, and a move to Git.pm could be a\nseparate future topic.\n\n>> +\tif (scalar(@_) == 2) {\n>> +\t\tif ($_[1] eq (\"store\" || \"cache\")) {\n>> +\t\t\trun_git(\"config credential.helper \\'$_[1]\\'\");\n>> +\t\t} else {\n>> +\t\t\tprint STDERR \"ERROR: run_credential (fill|approve|reject) [store|cache]\\n\";\n>> +\t\t\texit 1;\n>> +\t\t}\n>> +\t}\n>\n> This hunk looks wrong. You should never be setting the credential.helper\n> config; that is the responsibility of the user to set, as they want to\n> select whatever helper is appropriate. Nor do you need to care about\n> which helpers are in use; the point of git-credential is that it will do\n> that for you.\n\nAbsolutely.\n\n> sub fill_credential {\n> my $quoted_url = quotemeta(shift);\n>\n> my $verbatim = `git credential fill $quoted_url`;\n> $? and die \"git-credential failed\";\n>\n> $verbatim =~ /^username=(.*)$/m\n> or die \"git-credential did not give us a username\";\n> my $username = $1;\n> $verbatim =~ /^password=(.*)$/m\n> or die \"git-credential did not give us a password\";\n>\n> return ($username, $password, $verbatim);\n> }\n>\n> sub report_credential {\n> my ($type, $verbatim) = @_;\n> open(my $fh, '|-', \"git credential $type\");\n> print $fh $verbatim;\n> }\n\nThat sounds sensible too. We should be careful not to give a password as\nargument (or users of the same machine will be able to find it with e.g.\n\"ps u\"), but your proposal is OK with that.\n\n>> +\t\t\t# error if key undef\n>> +\t\t\tif (not defined $key) {\n>> +\t\t\t\tprint STDERR \"ERROR reciving reponse git credential fill\\n\";\n>> +\t\t\t\texit 1;\n>> +\t\t\t}\n[...]\n>> +\t} else {\n>> +\t\twhile () {\n>> +\t\t\tprint STDERR \"\\nERROR while running git credential $op:\\n$_\";\n>> +\t\t}\n>> +\t}\n>> +}\n>\n> This isn't a good way to check for errors. The non-fill actions will\n> never produce output on stdout, and you are not intercepting their\n> stderr. Besides which, checking for errors by reading stderr is not a\n> good practice; you should check the return value of the command in $?\n> after it finishes.\n\nI think it should do both. In case \"git credential fill\" returns\nsomething that doesn't match the regexp, we don't want perl to error\nwith \"use of undefined value\", but that's just being defensive because\nit shouldn't happen.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"193269","messageId":"f60bde8e22c112d57e1ed967af774b38@telesun.imag.fr","threadId":"30753","inReplyTo":"vpqaa0bmgnt.fsf@bauges.imag.fr","subject":"Re: [PATCH_v1] add git credential login to remote mediawiki","fromName":"roucherj","fromEmail":"roucherj@telesun.imag.fr","sentAt":"2012-06-10T17:36:16Z","receivedAt":"2012-06-10T17:36:16Z","isPatch":false,"sender":{"key":"roucherj@telesun.imag.fr","avatar":null},"body":"On Sun, 10 Jun 2012 15:37:42 +0200, Matthieu Moy wrote:\n> Jeff King writes:\n>\n>> On Sat, Jun 09, 2012 at 08:53:48PM +0200, \n>> Javier.Roucher-Iglesias@ensimag.imag.fr wrote:\n>>\n>>> diff --git a/contrib/mw-to-git/git-remote-mediawiki \n>>> b/contrib/mw-to-git/git-remote-mediawiki\n>>> index c18bfa1..4b14d78 100755\n>>> --- a/contrib/mw-to-git/git-remote-mediawiki\n>>> +++ b/contrib/mw-to-git/git-remote-mediawiki\n>>> @@ -152,28 +152,111 @@ while () {\n>>> ########################## Functions \n>>> ##############################\n>>>\n>>> # MediaWiki API instance, created lazily.\n>>> +sub run_credential {\n>>\n>> Is there any reason not to add this to perl/Git.pm? I suspect that \n>> other\n>> scripts will want to use it, too (for example, send-email could \n>> probably\n>> use it for SMTP credentials).\n>\n> Currently, git-remote-mediawiki is a standalone script (doesn't use\n> Git.pm). This is good because it makes it trivial to install, but bad \n> in\n> the sense that it may force us (or others) to reinvent the wheel.\n>\n> Until now, the wheels we reinvented were very simple (run_git\n> essentially), but we may be reaching the point where it makes sense \n> to\n> use and contribute to Git.pm.\n>\n> Unfortunately, from a non-technical point of view, Javier is\n> contributing this as part of a student project, which ends this week,\n> and it's probably not reasonable to introduce such change so late. \n> So,\n> I'd keep it here at least for now, and a move to Git.pm could be a\n> separate future topic.\n>\n\nThank's for explain my situation.\n\n>>> +\tif (scalar(@_) == 2) {\n>>> +\t\tif ($_[1] eq (\"store\" || \"cache\")) {\n>>> +\t\t\trun_git(\"config credential.helper \\'$_[1]\\'\");\n>>> +\t\t} else {\n>>> +\t\t\tprint STDERR \"ERROR: run_credential (fill|approve|reject) \n>>> [store|cache]\\n\";\n>>> +\t\t\texit 1;\n>>> +\t\t}\n>>> +\t}\n>>\n>> This hunk looks wrong. You should never be setting the \n>> credential.helper\n>> config; that is the responsibility of the user to set, as they want \n>> to\n>> select whatever helper is appropriate. Nor do you need to care about\n>> which helpers are in use; the point of git-credential is that it \n>> will do\n>> that for you.\n>\n> Absolutely.\n>\n\nI have add this with no advance warning, but i will remove it in the \nnext patch.\n\n>> sub fill_credential {\n>> my $quoted_url = quotemeta(shift);\n>>\n>> my $verbatim = `git credential fill $quoted_url`;\n>> $? and die \"git-credential failed\";\n>>\n>> $verbatim =~ /^username=(.*)$/m\n>> or die \"git-credential did not give us a \n>> username\";\n>> my $username = $1;\n>> $verbatim =~ /^password=(.*)$/m\n>> or die \"git-credential did not give us a \n>> password\";\n>>\n>> return ($username, $password, $verbatim);\n>> }\n>>\n>> sub report_credential {\n>> my ($type, $verbatim) = @_;\n>> open(my $fh, '|-', \"git credential $type\");\n>> print $fh $verbatim;\n>> }\n>\n> That sounds sensible too. We should be careful not to give a password \n> as\n> argument (or users of the same machine will be able to find it with \n> e.g.\n> \"ps u\"), but your proposal is OK with that.\n>\n>>> +\t\t\t# error if key undef\n>>> +\t\t\tif (not defined $key) {\n>>> +\t\t\t\tprint STDERR \"ERROR reciving reponse git credential fill\\n\";\n>>> +\t\t\t\texit 1;\n>>> +\t\t\t}\n> [...]\n\nto be change, thanks for the corrections\n\n>>> +\t} else {\n>>> +\t\twhile () {\n>>> +\t\t\tprint STDERR \"\\nERROR while running git credential $op:\\n$_\";\n>>> +\t\t}\n>>> +\t}\n>>> +}\n>>\n>> This isn't a good way to check for errors. The non-fill actions will\n>> never produce output on stdout, and you are not intercepting their\n>> stderr. Besides which, checking for errors by reading stderr is not \n>> a\n>> good practice; you should check the return value of the command in \n>> $?\n>> after it finishes.\n>\n> I think it should do both. In case \"git credential fill\" returns\n> something that doesn't match the regexp, we don't want perl to error\n> with \"use of undefined value\", but that's just being defensive \n> because\n> it shouldn't happen.\n"},{"id":"193276","messageId":"20120610183921.GA8614@sigill.intra.peff.net","threadId":"30753","inReplyTo":"vpqaa0bmgnt.fsf@bauges.imag.fr","subject":"Re: [PATCH_v1] add git credential login to remote mediawiki","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-06-10T18:39:21Z","receivedAt":"2012-06-10T18:39:21Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 10, 2012 at 03:37:42PM +0200, Matthieu Moy wrote:\n\n> > Is there any reason not to add this to perl/Git.pm? I suspect that other\n> > scripts will want to use it, too (for example, send-email could probably\n> > use it for SMTP credentials).\n> \n> Currently, git-remote-mediawiki is a standalone script (doesn't use\n> Git.pm). This is good because it makes it trivial to install, but bad in\n> the sense that it may force us (or others) to reinvent the wheel.\n> \n> Until now, the wheels we reinvented were very simple (run_git\n> essentially), but we may be reaching the point where it makes sense to\n> use and contribute to Git.pm.\n\nYeah, I noticed that. But hopefully since they are at least in the same\ndistribution, it is just a matter of getting the Makefiles right, and\nwill not be an extra burden on the user.\n\n> Unfortunately, from a non-technical point of view, Javier is\n> contributing this as part of a student project, which ends this week,\n> and it's probably not reasonable to introduce such change so late. So,\n> I'd keep it here at least for now, and a move to Git.pm could be a\n> separate future topic.\n\nTotally understood. I review from git's perspective, but it is up to you\nto manage your students' workload. We can migrate the code to Git.pm\nlater (probably as part of a series which actually introduces a second\ncaller).\n\n> > sub fill_credential {\n> > my $quoted_url = quotemeta(shift);\n> >\n> > my $verbatim = `git credential fill $quoted_url`;\n> > $? and die \"git-credential failed\";\n> >\n> > $verbatim =~ /^username=(.*)$/m\n> > or die \"git-credential did not give us a username\";\n> > my $username = $1;\n> > $verbatim =~ /^password=(.*)$/m\n> > or die \"git-credential did not give us a password\";\n> >\n> > return ($username, $password, $verbatim);\n> > }\n> >\n> > sub report_credential {\n> > my ($type, $verbatim) = @_;\n> > open(my $fh, '|-', \"git credential $type\");\n> > print $fh $verbatim;\n> > }\n> \n> That sounds sensible too. We should be careful not to give a password as\n> argument (or users of the same machine will be able to find it with e.g.\n> \"ps u\"), but your proposal is OK with that.\n\nYes, that was intentional (and is the reason why helpers do so much over\nstdin, even though it would reduce their parsing load if they could use\ncommand-line arguments). Unfortunately, there is still one case that\nreveals a password on the command-line: if a caller has a URL that\ncontains an embedded password like:\n\n https://bob:secret@example.com/foo.git\n\nIt's tempting to say \"well, then they don't need to ask git-credential\nat all!\". But the point of handing the whole URL to git-credential is so\nthat the caller doesn't _have_ to do the parsing. So how would it know\nthat the URL contains a password? :)\n\nSo instead of a URL on the command-line, it might make sense to simply\nlet the caller send an extra \"url=\" parameter on stdin (in addition to\nany broken-down parameters, if it also wishes). It's way less convenient\n(you are stuck with open2 from perl, rather than simple backticks), but\nI think we should be cautious due to the security implications.\n\n> >> +\t\t\t# error if key undef\n> >> +\t\t\tif (not defined $key) {\n> >> +\t\t\t\tprint STDERR \"ERROR reciving reponse git credential fill\\n\";\n> >> +\t\t\t\texit 1;\n> >> +\t\t\t}\n> [...]\n> >> +\t} else {\n> >> +\t\twhile () {\n> >> +\t\t\tprint STDERR \"\\nERROR while running git credential $op:\\n$_\";\n> >> +\t\t}\n> >> +\t}\n> >> +}\n> >\n> > This isn't a good way to check for errors. The non-fill actions will\n> > never produce output on stdout, and you are not intercepting their\n> > stderr. Besides which, checking for errors by reading stderr is not a\n> > good practice; you should check the return value of the command in $?\n> > after it finishes.\n> \n> I think it should do both. In case \"git credential fill\" returns\n> something that doesn't match the regexp, we don't want perl to error\n> with \"use of undefined value\", but that's just being defensive because\n> it shouldn't happen.\n\nSorry, I just meant the latter block. It is checking for non-fill\nactions to send any output, which they will never do (yes, it would be\nan error for them to do so, but it is a very unlikely bug, and IMHO not\nreally worth checking).\n\n-Peff\n"}]}