Re: [PATCHv3 4/5] Git.pm: add interface for git credential command

From: Jeff King <peff@peff.net>
Date: Feb 11, 2013, 17:36 UTC
Message-ID: <20130211173632.GJ16402@sigill.intra.peff.net>
In-Reply-To: <xa1tr4kmg4cv.fsf@mina86.com>

On Mon, Feb 11, 2013 at 06:14:24PM +0100, Michal Nazarewicz wrote:

Show 9 quoted lines

> > Should this return a hash reference? It seems like that is how we end up
> > using and passing it elsewhere (since we have to anyway when passing it
> > as a parameter).
> 
> Admittedly I mostly just copied what git-remote-mediawiki did here and
> don't really have any preference either way, even though with this
> function returning a reference the call site would have to become:
> 
>                 %$credential = %{ credential_read $reader };

Oh, right, because Git::credential takes the credential as an in-out parameter rather than just returning it. Which is a bit unusual in perl, but keeps the interface reasonably simple. The alternative would be:

  $cred = Git::credential $cred, sub {
     ...
  }

which is a little less nice.

> Another alternative would be for it to take a reference as an argument,
> possibly an optional one:

I think that is making things more ugly.

> I'd avoid modifying the hash while reading though since I think it's
> best if it's left intact in case of an error.

Agreed.

Show 9 quoted lines

> And of course, if we want to get even more crazy, credential_write could
> accept either reference or a hash, like so:
> 
> +sub credential_write {
> +	my ($self, $writer, @rest) = _maybe_self(@_);
> +	my $credential = @rest == 1 ? $rest[0] : { @rest };
> +	my ($key, $value);
> +	# ...
> +}

Ugh.

> Bottom line is, anything can be coded, but a question is whether it
> makes sense to do so. ;)

Yes, it is probably OK to leave it as-is, then. It is largely a matter of taste, and I will defer to your judgement on that. :)

-Peff