Re: [PATCH 4/4] mw-to-git: use git-credential's URL parser
On Wed, Jul 18, 2012 at 02:37:27PM +0200, Matthieu Moy wrote:
Show 10 quoted lines
> Jeff King <peff@peff.net> writes:
>
> > I started with a version that did that, but there are two complications:
> >
> > 1. credential_write needs to know that the 'url' field must come
> > first, as it overwrites the other fields. So we end up
> > special-casing it either way.
>
> Right, I didn't think of that. But you'd have to special-case it only
> within credential_run, and not for the caller.
Yeah. It would look like this:
diff --git a/contrib/mw-to-git/git-remote-mediawiki b/contrib/mw-to-git/git-remote-mediawiki
index c9ac416..e1392b0 100755
--- a/contrib/mw-to-git/git-remote-mediawiki
+++ b/contrib/mw-to-git/git-remote-mediawiki
@@ -190,9 +190,11 @@ sub credential_write {
sub credential_run {
my $op = shift;
my $credential = shift;
- my $url = shift;
my $pid = open2(my $reader, my $writer, "git credential $op");
- print $writer "url=$url\n" if defined $url;
+ if (exists $credential->{url}) {
+ print $writer "url=$credential->{url}\n";
+ delete $credential->{url};
+ }
credential_write($credential, $writer);
print $writer "\n";
close($writer);
which is still kind of ugly. We could also push it down into
credential_write, like:
diff --git a/contrib/mw-to-git/git-remote-mediawiki b/contrib/mw-to-git/git-remote-mediawiki
index c9ac416..0a821fd 100755
--- a/contrib/mw-to-git/git-remote-mediawiki
+++ b/contrib/mw-to-git/git-remote-mediawiki
@@ -180,8 +180,9 @@ sub credential_read {
sub credential_write {
my $credential = shift;
my $writer = shift;
+ print $writer "url=$credential->{url}\n" if exists $credential->{url};
while (my ($key, $value) = each(%$credential) ) {
- if (length $value) {
+ if (length $value && $key ne 'url') {
print $writer "$key=$value\n";
}
}
@@ -190,9 +191,7 @@ sub credential_write {
sub credential_run {
my $op = shift;
my $credential = shift;
- my $url = shift;
my $pid = open2(my $reader, my $writer, "git credential $op");
- print $writer "url=$url\n" if defined $url;
credential_write($credential, $writer);
print $writer "\n";
close($writer);
which is probably the least gross. I was originally hesitant because the
issue (2) I brought up, but...
> > 2. Git hands us back the broken-down version, which we add to the
> > credential.
>
> We don't add it, but already replace the whole structure. This is
> somehow needed because "git credential fill" can remove fields from the
> structure (the path attribute is removed with
> credential.useHttpPath=false). So, this point doesn't seem problematic.
Hmph. I considered that we might do it and even checked, but I somehow
read the code wrong (I think I was thrown off by the pass-by-reference
to credential_run, but of course it overwrites it inside that function).
So since that is a non-issue, I think the second diff I provided above
is a bit nicer.
-Peff
Jeff King <peff@peff.net>