Re: [PATCH 4/4] mw-to-git: use git-credential's URL parser
Jeff King writes: > So since that is a non-issue, I think the second diff I provided above > is a bit nicer. I like this one best too. It just lacks a comment saying why it should be displayed first ;-). -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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: > Jeff King 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] mw-to-git: use git-credential's URL parser
Jeff King 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. > 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. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] mw-to-git: use git-credential's URL parser
On Wed, Jul 18, 2012 at 02:24:13PM +0200, Matthieu Moy wrote: > Jeff King writes: > > > @@ -216,7 +190,9 @@ 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); > > @@ -246,10 +222,10 @@ sub mw_connect_maybe { > > $mediawiki = MediaWiki::API->new; > > $mediawiki->{config}->{api_url} = "$url/api.php"; > > if ($wiki_login) { > > - my %credential = credential_from_url($url); > > + my %credential; > > $credential{username} = $wiki_login; > > $credential{password} = $wiki_passwd; > > - credential_run("fill", \%credential); > > + credential_run("fill", \%credential, $url); > > my $request = {lgname => $credential{username}, > >lgpassword => $credential{password}, > >lgdomain => $wiki_domain}; > > I would find it more elegant not to special-case the URL field, and just > > my %credential = ('url' => $url); > > but I'm fine with your version too. 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. 2. Git hands us back the broken-down version, which we add to the credential. So it is superfluous to keep sending 'url' for the other, non-fill interactions. I don't think it's technically wrong, but it also seemed inelegant. Having "credential_write" delete the 'url' field seemed even more inelegant. > Other than that, and for the 4 patches of the serie: > > Reviewed-by: Matthieu Moy Thanks. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] mw-to-git: use git-credential's URL parser
Jeff King writes: > @@ -216,7 +190,9 @@ 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); > @@ -246,10 +222,10 @@ sub mw_connect_maybe { > $mediawiki = MediaWiki::API->new; > $mediawiki->{config}->{api_url} = "$url/api.php"; > if ($wiki_login) { > - my %credential = credential_from_url($url); > + my %credential; > $credential{username} = $wiki_login; > $credential{password} = $wiki_passwd; > - credential_run("fill", \%credential); > + credential_run("fill", \%credential, $url); > my $request = {lgname => $credential{username}, > lgpassword => $credential{password}, > lgdomain => $wiki_domain}; I would find it more elegant not to special-case the URL field, and just my %credential = ('url' => $url); but I'm fine with your version too. Other than that, and for the 4 patches of the serie: Reviewed-by: Matthieu Moy -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/4] mw-to-git: use git-credential's URL parser
We can just feed our URL straight to git-credential and it will parse it for us, saving us some code. Signed-off-by: Jeff King --- contrib/mw-to-git/git-remote-mediawiki | 32 1 file changed, 4 insertions(+), 28 deletions(-) diff --git a/contrib/mw-to-git/git-remote-mediawiki b/contrib/mw-to-git/git-remote-mediawiki index b06f27b..c9ac416 100755 --- a/contrib/mw-to-git/git-remote-mediawiki +++ b/contrib/mw-to-git/git-remote-mediawiki @@ -163,32 +163,6 @@ while () { ## credential API management (generic functions) -sub credential_from_url { - my $url = shift; - my $parsed = URI->new($url); - my %credential; - - if ($parsed->scheme) { - $credential{protocol} = $parsed->scheme; - } - if ($parsed->host) { - $credential{host} = $parsed->host; - } - if ($parsed->path) { - $credential{path} = $parsed->path; - } - if ($parsed->userinfo) { - if ($parsed->userinfo =~ /([^:]*):(.*)/) { - $credential{username} = $1; - $credential{password} = $2; - } else { - $credential{username} = $parsed->userinfo; - } - } - - return %credential; -} - sub credential_read { my %credential; my $reader = shift; @@ -216,7 +190,9 @@ 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); @@ -246,10 +222,10 @@ sub mw_connect_maybe { $mediawiki = MediaWiki::API->new; $mediawiki->{config}->{api_url} = "$url/api.php"; if ($wiki_login) { - my %credential = credential_from_url($url); + my %credential; $credential{username} = $wiki_login; $credential{password} = $wiki_passwd; - credential_run("fill", \%credential); + credential_run("fill", \%credential, $url); my $request = {lgname => $credential{username}, lgpassword => $credential{password}, lgdomain => $wiki_domain}; -- 1.7.10.5.40.g059818d -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html