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

[roucherj <roucherj@xxxxxxxxxxxxxxx>]

On Sun, 10 Jun 2012 15:37:42 +0200, Matthieu Moy wrote:
> Jeff King <peff@xxxxxxxx> writes:
>
> > On Sat, Jun 09, 2012 at 08:53:48PM +0200, 
> > Javier.Roucher-Iglesias@xxxxxxxxxxxxxxx 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.

--
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