[PATCH 0/4] mediawiki/credential cleanups

[PATCH 0/4] mediawiki/credential cleanups

[Jeff King]

Hi Matthieu,

Your initial credential plumbing series went by while I was on vacation,
and I finally got a chance to look at it in more detail. Overall it
looks very good; thank you (and your students) for working on it. It was
especially nice to have a test harness with complete instructions. :)

I came up with a few commits on top. Patches 1 and 2 are cleanups that should
hopefully be non-controversial. Patch 3 is the "url=" attribute we
discussed earlier, and then patch 4 makes use of it in mw-to-git.

  [1/4]: docs/credential: minor clarity fixups
  [2/4]: mw-to-git: check blank credential attributes via length
  [3/4]: credential: convert "url" attribute into its parsed subparts
  [4/4]: mw-to-git: use git-credential's URL parser

 Documentation/git-credential.txt       | 22 ++++++++++++++++------
 contrib/mw-to-git/git-remote-mediawiki | 34 +++++-----------------------------
 credential.c                           |  2 ++
 3 files changed, 23 insertions(+), 35 deletions(-)

-Peff

[PATCH 1/4] docs/credential: minor clarity fixups

[Jeff King]

The text in git-credential(1) was copied from
technical/api-credentials, so it still talks about the
input/output format as coming from git to the helper. Since
the surrounding text already indicates that this format is
used for reading and writing with git credential, we can
just remove the extraneous confusing bits.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-credential.txt | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt
index a81684e..afd5365 100644
--- a/Documentation/git-credential.txt
+++ b/Documentation/git-credential.txt
@@ -102,22 +102,20 @@ INPUT/OUTPUT FORMAT
 -------------------
 
 `git credential` reads and/or writes (depending on the action used)
-credential information in its standard input/output. These information
+credential information in its standard input/output. This information
 can correspond either to keys for which `git credential` will obtain
 the login/password information (e.g. host, protocol, path), or to the
 actual credential data to be obtained (login/password).
 
-The credential is split into a set of named attributes.
-Attributes are provided to the helper, one per line. Each attribute is
+The credential is split into a set of named attributes, with one
+attribute per line. Each attribute is
 specified by a key-value pair, separated by an `=` (equals) sign,
 followed by a newline. The key may contain any bytes except `=`,
 newline, or NUL. The value may contain any bytes except newline or NUL.
 In both cases, all bytes are treated as-is (i.e., there is no quoting,
 and one cannot transmit a value with newline or NUL in it). The list of
 attributes is terminated by a blank line or end-of-file.
-Git will send the following attributes (but may not send all of
-them for a given credential; for example, a `host` attribute makes no
-sense when dealing with a non-network protocol):
+Git understands the following attributes:
 
 `protocol`::
 
-- 
1.7.10.5.40.g059818d

[PATCH 2/4] mw-to-git: check blank credential attributes via length

[Jeff King]

When writing a credential to git-credential, we omit fields
that do not have a true value. This will skip empty or
undefined fields (which we want), but will also accidentally
skip usernames or passwords which happen to have a non-true
value (e.g., "0"). Be more careful by checking for non-zero
length.

Signed-off-by: Jeff King <peff@peff.net>
---
 contrib/mw-to-git/git-remote-mediawiki | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/mw-to-git/git-remote-mediawiki b/contrib/mw-to-git/git-remote-mediawiki
index accd70a..b06f27b 100755
--- a/contrib/mw-to-git/git-remote-mediawiki
+++ b/contrib/mw-to-git/git-remote-mediawiki
@@ -207,7 +207,7 @@ sub credential_write {
 	my $credential = shift;
 	my $writer = shift;
 	while (my ($key, $value) = each(%$credential) ) {
-		if ($value) {
+		if (length $value) {
 			print $writer "$key=$value\n";
 		}
 	}
-- 
1.7.10.5.40.g059818d

[PATCH 3/4] credential: convert "url" attribute into its parsed subparts

[Jeff King]

The git-credential command requires that you feed it a
broken-down credential, which means that the client needs to
parse a URL itself. Since we have our own URL-parsing
routines, we can easily allow the caller to just give us the
URL as-is, saving them some code.

Signed-off-by: Jeff King <peff@peff.net>
---
The implementation turned out to be delightfully simple. I stopped short
of adding an "ident" command to git-credential where you could do
something like:

  $ echo https://user@example.com | git credential ident
  protocol=https
  host=example.com
  username=user

since I had no use for it, but it would obviously be an easy one-liner
to write (it's just "fill" without the actual fill call).

 Documentation/git-credential.txt | 12 ++++++++++++
 credential.c                     |  2 ++
 2 files changed, 14 insertions(+)

diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt
index afd5365..53adee3 100644
--- a/Documentation/git-credential.txt
+++ b/Documentation/git-credential.txt
@@ -140,3 +140,15 @@ Git understands the following attributes:
 `password`::
 
 	The credential's password, if we are asking it to be stored.
+
+`url`::
+
+	When this special attribute is read by `git credential`, the
+	value is parsed as a URL and treated as if its constituent parts
+	were read (e.g., `url=https://example.com` would behave as if
+	`protocol=https` and `host=example.com` had been provided). This
+	can help callers avoid parsing URLs themselves.  Note that any
+	components which are missing from the URL (e.g., there is no
+	username in the example above) will be set to empty; if you want
+	to provide a URL and override some attributes, provide the URL
+	attribute first, followed by any overrides.
diff --git a/credential.c b/credential.c
index 2c40007..e54753c 100644
--- a/credential.c
+++ b/credential.c
@@ -172,6 +172,8 @@ int credential_read(struct credential *c, FILE *fp)
 		} else if (!strcmp(key, "path")) {
 			free(c->path);
 			c->path = xstrdup(value);
+		} else if (!strcmp(key, "url")) {
+			credential_from_url(c, value);
 		}
 		/*
 		 * Ignore other lines; we don't know what they mean, but
-- 
1.7.10.5.40.g059818d

Re: [PATCH 3/4] credential: convert "url" attribute into its parsed subparts

[Matthieu Moy]

Jeff King <peff@peff.net> writes:

>   $ echo https://user@example.com | git credential ident
>   protocol=https
>   host=example.com
>   username=user
>
> since I had no use for it, but it would obviously be an easy one-liner
> to write (it's just "fill" without the actual fill call).

I was thinking the same, except I would have spelled it "git credential
parse" (but ident is fine too). On the perl side, that would allow
getting a credential hash very simply (but it was already simple in
perl, and made useless by your code).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH 3/4] credential: convert "url" attribute into its parsed subparts

[Jeff King]

On Wed, Jul 18, 2012 at 02:24:01PM +0200, Matthieu Moy wrote:

> Jeff King <peff@peff.net> writes:
> 
> >   $ echo https://user@example.com | git credential ident
> >   protocol=https
> >   host=example.com
> >   username=user
> >
> > since I had no use for it, but it would obviously be an easy one-liner
> > to write (it's just "fill" without the actual fill call).
> 
> I was thinking the same, except I would have spelled it "git credential
> parse" (but ident is fine too). On the perl side, that would allow
> getting a credential hash very simply (but it was already simple in
> perl, and made useless by your code).

I wanted to give it some name that meant "pass-through" rather than just
parse, in case we add more magic attributes later. I meant "ident" to be
like "mathematical identity", but in the context of a credential tool,
it is probably somewhat ambiguous. :)

Anyway, the fact that we can just do the parsing as part of the "fill"
means we don't need it for now, so I'll leave it until somebody really
cares.

-Peff

[PATCH 4/4] mw-to-git: use git-credential's URL parser

[Jeff King]

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 <peff@peff.net>
---
 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 (<STDIN>) {
 
 ## 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

Re: [PATCH 4/4] mw-to-git: use git-credential's URL parser

[Matthieu Moy]

Jeff King <peff@peff.net> 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@imag.fr>

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH 4/4] mw-to-git: use git-credential's URL parser

[Jeff King]

On Wed, Jul 18, 2012 at 02:24:13PM +0200, Matthieu Moy wrote:

> Jeff King <peff@peff.net> 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 <Matthieu.Moy@imag.fr>

Thanks.

-Peff

Re: [PATCH 4/4] mw-to-git: use git-credential's URL parser

[Matthieu Moy]

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.

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

Re: [PATCH 4/4] mw-to-git: use git-credential's URL parser

[Jeff King]

On Wed, Jul 18, 2012 at 02:37:27PM +0200, Matthieu Moy wrote:

> 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

[PATCHv2 4/4] mw-to-git: use git-credential's URL parser

[Jeff King]

On Wed, Jul 18, 2012 at 08:57:41AM -0400, Jeff King wrote:

> So since that is a non-issue, I think the second diff I provided above
> is a bit nicer.

And here is a replacement patch 4/4.

-- >8 --
Subject: 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 <peff@peff.net>
---
 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..38afa76 100755
--- a/contrib/mw-to-git/git-remote-mediawiki
+++ b/contrib/mw-to-git/git-remote-mediawiki
@@ -163,32 +163,6 @@ while (<STDIN>) {
 
 ## 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;
@@ -206,8 +180,10 @@ sub credential_read {
 sub credential_write {
 	my $credential = shift;
 	my $writer = shift;
+	# url overwrites other fields, so it must come first
+	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";
 		}
 	}
@@ -246,7 +222,7 @@ 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 = (url => $url);
 		$credential{username} = $wiki_login;
 		$credential{password} = $wiki_passwd;
 		credential_run("fill", \%credential);
-- 
1.7.10.5.40.g059818d

Re: [PATCHv2 4/4] mw-to-git: use git-credential's URL parser

[Matthieu Moy]

Jeff King <peff@peff.net> writes:

> On Wed, Jul 18, 2012 at 08:57:41AM -0400, Jeff King wrote:
>
>> So since that is a non-issue, I think the second diff I provided above
>> is a bit nicer.
>
> And here is a replacement patch 4/4.
>
> -- >8 --
> Subject: mw-to-git: use git-credential's URL parser

Perfect, thanks.

(and you've been faster than me about the comment ;-) )

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH 4/4] mw-to-git: use git-credential's URL parser

[Matthieu Moy]

Jeff King <peff@peff.net> 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/