[PATCH] git-send-email.perl: expand filename of aliasesfile

[PATCH] git-send-email.perl: expand filename of aliasesfile

[Cord Seele]

Allow config variable sendemail.aliasesfile to contain a tilde, i.e.
make ~/.mutt/aliases work.

Signed-off-by: Cord Seele <cowose@gmail.com>
---
 git-send-email.perl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 98ab33a..b50b35f 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -481,7 +481,7 @@ my %parse_alias = (
 
 if (@alias_files and $aliasfiletype and defined $parse_alias{$aliasfiletype}) {
 	foreach my $file (@alias_files) {
-		open my $fh, '<', $file or die "opening $file: $!\n";
+		open my $fh, '<', glob($file) or die "opening $file: $!\n";
 		$parse_alias{$aliasfiletype}->($fh);
 		close $fh;
 	}
-- 
1.7.6.4.dirty


-- Cord

Re: [PATCH] git-send-email.perl: expand filename of aliasesfile

[Matthieu Moy]

Cord Seele <cowose@googlemail.com> writes:

> -		open my $fh, '<', $file or die "opening $file: $!\n";
> +		open my $fh, '<', glob($file) or die "opening $file: $!\n";

That'd be cleaner to use

git config --path sendemail.aliasesfile

to let Git do the right expansion, in a way consistant with other places
of Git.

In practice, that would imply adding %config_path_settings like the
existing %config_bool_settings, and add config_path to Git.pm (again,
similarly to config_bool).

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

Re: [PATCH] git-send-email.perl: expand filename of aliasesfile

[Cord Seele]

On Wed 28 Sep 2011 15:42:01 +0200, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:

> Cord Seele <cowose@googlemail.com> writes:
> 
> > -		open my $fh, '<', $file or die "opening $file: $!\n";
> > +		open my $fh, '<', glob($file) or die "opening $file: $!\n";
> 
> That'd be cleaner to use
> 
> git config --path sendemail.aliasesfile
> 
> to let Git do the right expansion, in a way consistant with other places
> of Git.

This means to expand it at 'git config' time? Wouldn't it be nicer to
have it expanded when you run 'git send-email'? Then you could move your
~/.gitconfig (that's where I have my aliasesfile configured) between different
accounts and it could still work.

-- Cord

Re: [PATCH] git-send-email.perl: expand filename of aliasesfile

[Matthieu Moy]

Cord Seele <cowose@googlemail.com> writes:

> On Wed 28 Sep 2011 15:42:01 +0200, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:
>> That'd be cleaner to use
>> 
>> git config --path sendemail.aliasesfile
>> 
>> to let Git do the right expansion, in a way consistant with other places
>> of Git.
>
> This means to expand it at 'git config' time?

Yes, but not the "git config" ran to set the value. The one ran
internally by "git send-email" through Git::config(). You may add --get
to my command line above.

> Wouldn't it be nicer to have it expanded when you run 'git
> send-email'? Then you could move your ~/.gitconfig (that's where I
> have my aliasesfile configured) between different accounts and it
> could still work.

That works with my proposal.

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

[PATCH v2] git-send-email: allow filename expansion

[Cord Seele]

OK, here's another try to get it more Git-like.

Filename expansion now works as far as I can tell. But I failed actually
using my mutt aliases (but also with plain v1.7.6.4). Since I'm not
familiar with perl-debugging could please someone more experienced have a look?
Thanks.

-- Cord

[PATCH 1/2] Add Git::config_path()
[PATCH 2/2] use new Git::config_path() for aliasesfile

[PATCH 1/2] Add Git::config_path()

[Cord Seele]

Use --path option when calling 'git config' thus allow for pathname
expansion, e.g. a tilde.

Signed-off-by: Cord Seele <cowose@gmail.com>
---
 perl/Git.pm |   32 ++++++++++++++++++++++++++++++++
 1 files changed, 32 insertions(+), 0 deletions(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index a86ab70..c279bfb 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -627,6 +627,38 @@ sub config_bool {
 	};
 }
 
+
+=item config_path ( VARIABLE )
+
+Retrieve the path configuration C<VARIABLE>. The return value
+is an expanded path or C<undef> if it's not defined.
+
+This currently wraps command('config') so it is not so fast.
+
+=cut
+
+sub config_path {
+	my ($self, $var) = _maybe_self(@_);
+
+	try {
+		my @cmd = ('config', '--path');
+		unshift @cmd, $self if $self;
+		if (wantarray) {
+			return command(@cmd, '--get-all', $var);
+		} else {
+			return command_oneline(@cmd, '--get', $var);
+		}
+	} catch Git::Error::Command with {
+		my $E = shift;
+		if ($E->value() == 1) {
+			# Key not found.
+			return undef;
+		} else {
+			throw $E;
+		}
+	};
+}
+
 =item config_int ( VARIABLE )
 
 Retrieve the integer configuration C<VARIABLE>. The return value
-- 
1.7.6.4

Re: [PATCH 1/2] Add Git::config_path()

[Jakub Narebski]

Cord Seele <cowose@googlemail.com> writes:

> Use --path option when calling 'git config' thus allow for pathname
> expansion, e.g. a tilde.
> 
> Signed-off-by: Cord Seele <cowose@gmail.com>
> ---
>  perl/Git.pm |   32 ++++++++++++++++++++++++++++++++
>  1 files changed, 32 insertions(+), 0 deletions(-)

I think the following minimal test should be squashed in:

---
diff --git a/t/t9700-perl-git.sh b/t/t9700-perl-git.sh
index 3787186..7558f0c 100755
--- a/t/t9700-perl-git.sh
+++ b/t/t9700-perl-git.sh
@@ -43,7 +43,9 @@ test_expect_success \
      git config --add test.booltrue true &&
      git config --add test.boolfalse no &&
      git config --add test.boolother other &&
-     git config --add test.int 2k
+     git config --add test.int 2k &&
+     git config --add test.path "~/foo" &&
+     git config --add test.pathexpanded "$HOME/foo"
      '
 
 # The external test will outputs its own plan
diff --git a/t/t9700/test.pl b/t/t9700/test.pl
index 13ba96e..ce9340c 100755
--- a/t/t9700/test.pl
+++ b/t/t9700/test.pl
@@ -33,6 +33,8 @@ is($r->config_int("test.int"), 2048, "config_int: integer");
 is($r->config_int("test.nonexistent"), undef, "config_int: nonexistent");
 ok($r->config_bool("test.booltrue"), "config_bool: true");
 ok(!$r->config_bool("test.boolfalse"), "config_bool: false");
+is($r->config_path("test.path"), $r->config("test.pathexpanded"),
+   "config_path: ~/foo expansion");
 our $ansi_green = "\x1b[32m";
 is($r->get_color("color.test.slot1", "red"), $ansi_green, "get_color");
 # Cannot test $r->get_colorbool("color.foo")) because we do not

Re: [PATCH 1/2] Add Git::config_path()

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> diff --git a/t/t9700-perl-git.sh b/t/t9700-perl-git.sh
> index 3787186..7558f0c 100755
> --- a/t/t9700-perl-git.sh
> +++ b/t/t9700-perl-git.sh
> @@ -43,7 +43,9 @@ test_expect_success \
>       git config --add test.booltrue true &&
>       git config --add test.boolfalse no &&
>       git config --add test.boolother other &&
> -     git config --add test.int 2k
> +     git config --add test.int 2k &&
> +     git config --add test.path "~/foo" &&
> +     git config --add test.pathexpanded "$HOME/foo"

Given that test-lib.sh sets up the $HOME away from unknown place to ensure
repeatability of tests, I am not sure if this test would ever pass.

Re: [PATCH 1/2] Add Git::config_path()

[Jakub Narebski]

On Mon, 7 Oct 2011, Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > diff --git a/t/t9700-perl-git.sh b/t/t9700-perl-git.sh
> > index 3787186..7558f0c 100755
> > --- a/t/t9700-perl-git.sh
> > +++ b/t/t9700-perl-git.sh
> > @@ -43,7 +43,9 @@ test_expect_success \
> >       git config --add test.booltrue true &&
> >       git config --add test.boolfalse no &&
> >       git config --add test.boolother other &&
> > -     git config --add test.int 2k
> > +     git config --add test.int 2k &&
> > +     git config --add test.path "~/foo" &&
> > +     git config --add test.pathexpanded "$HOME/foo"
> 
> Given that test-lib.sh sets up the $HOME away from unknown place to ensure
> repeatability of tests, I am not sure if this test would ever pass.

Well, it passes.

test-lib.sh sets $HOME to "$TRASH_DIRECTORY", but this value of $HOME
is then later seen by "git config --path ..." run by Git::config_path
in t9700/test.pl

  char *expand_user_path(const char *path)
  [...]
                if (username_len == 0) {
                        const char *home = getenv("HOME");
                        if (!home)
                                goto return_null;
                        strbuf_add(&user_path, home, strlen(home));
                } else {
  [...]

-- 
Jakub Narebski
Poland

Re: [PATCH 1/2] Add Git::config_path()

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

>   char *expand_user_path(const char *path)
>   [...]
>                 if (username_len == 0) {
>                         const char *home = getenv("HOME");
>                         if (!home)
>                                 goto return_null;
>                         strbuf_add(&user_path, home, strlen(home));
>                 } else {
>   [...]

Ahh, Ok. I was afraid getpwnam() codepath might interfere with it.

[PATCH 2/2] use new Git::config_path() for aliasesfile

[Cord Seele]

Signed-off-by: Cord Seele <cowose@gmail.com>
---
 git-send-email.perl |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 98ab33a..f17f7b3 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -225,7 +225,6 @@ my %config_settings = (
     "cccmd" => \$cc_cmd,
     "aliasfiletype" => \$aliasfiletype,
     "bcc" => \@bcclist,
-    "aliasesfile" => \@alias_files,
     "suppresscc" => \@suppress_cc,
     "envelopesender" => \$envelope_sender,
     "multiedit" => \$multiedit,
@@ -234,6 +233,10 @@ my %config_settings = (
     "assume8bitencoding" => \$auto_8bit_encoding,
 );
 
+my %config_path_settings = (
+    "aliasesfile" => \@alias_files,
+);
+
 # Help users prepare for 1.7.0
 sub chain_reply_to {
 	if (defined $chain_reply_to &&
@@ -330,6 +333,11 @@ sub read_config {
 		$$target = Git::config_bool(@repo, "$prefix.$setting") unless (defined $$target);
 	}
 
+	foreach my $setting (keys %config_path_settings) {
+		my $target = $config_path_settings{$setting}->[0];
+		$$target = Git::config_path(@repo, "$prefix.$setting") unless (defined $$target);
+	}
+
 	foreach my $setting (keys %config_settings) {
 		my $target = $config_settings{$setting};
 		next if $setting eq "to" and defined $no_to;
-- 
1.7.6.4

Re: [PATCH 2/2] use new Git::config_path() for aliasesfile

[Junio C Hamano]

I think the addition of "config --path" support is a good idea, but the
resulting code suffers from too many cut&paste cruft across the config*
family of methods.

How about doing a bit of refactoring, perhaps something like this, on top
as a separate patch?

I tried to be careful to still forcing the "one value only" for config_bool
and config_int, but extra sets of eyeballs would be needed.

 perl/Git.pm |   93 +++++++++++++++++-----------------------------------------
 1 files changed, 27 insertions(+), 66 deletions(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index c279bfb..f0a6e92 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -563,22 +563,18 @@ sub wc_chdir {
 }
 
 
-=item config ( VARIABLE )
+=item _config_common ( VARIABLE )
 
-Retrieve the configuration C<VARIABLE> in the same manner as C<config>
-does. In scalar context requires the variable to be set only one time
-(exception is thrown otherwise), in array context returns allows the
-variable to be set multiple times and returns all the values.
-
-This currently wraps command('config') so it is not so fast.
+Common subroutine to implement bulk of what the config* family of methods
+do. This wraps command('config') so it is not so fast.
 
 =cut
 
-sub config {
-	my ($self, $var) = _maybe_self(@_);
-
+sub _config_common {
+	my ($self, $var, $opts) = _maybe_self(@_);
+	
 	try {
-		my @cmd = ('config');
+		my @cmd = ('config', $opts->{'kind'} ? @{$opts->{'kind'}} : ());
 		unshift @cmd, $self if $self;
 		if (wantarray) {
 			return command(@cmd, '--get-all', $var);
@@ -594,6 +590,21 @@ sub config {
 			throw $E;
 		}
 	};
+
+}
+
+=item config ( VARIABLE )
+
+Retrieve the configuration C<VARIABLE> in the same manner as C<config>
+does. In scalar context requires the variable to be set only one time
+(exception is thrown otherwise), in array context returns allows the
+variable to be set multiple times and returns all the values.
+
+=cut
+
+sub config {
+	my ($self, $var) = _maybe_self(@_);
+	return _config_common($self, $var, +{});
 }
 
 
@@ -603,60 +614,24 @@ Retrieve the bool configuration C<VARIABLE>. The return value
 is usable as a boolean in perl (and C<undef> if it's not defined,
 of course).
 
-This currently wraps command('config') so it is not so fast.
-
 =cut
 
 sub config_bool {
 	my ($self, $var) = _maybe_self(@_);
-
-	try {
-		my @cmd = ('config', '--bool', '--get', $var);
-		unshift @cmd, $self if $self;
-		my $val = command_oneline(@cmd);
-		return undef unless defined $val;
-		return $val eq 'true';
-	} catch Git::Error::Command with {
-		my $E = shift;
-		if ($E->value() == 1) {
-			# Key not found.
-			return undef;
-		} else {
-			throw $E;
-		}
-	};
+	my $val = scalar _config_common($self, $var, {'kind' => '--bool'});
+	return (defined $val && $val eq 'true');
 }
 
-
 =item config_path ( VARIABLE )
 
 Retrieve the path configuration C<VARIABLE>. The return value
 is an expanded path or C<undef> if it's not defined.
 
-This currently wraps command('config') so it is not so fast.
-
 =cut
 
 sub config_path {
 	my ($self, $var) = _maybe_self(@_);
-
-	try {
-		my @cmd = ('config', '--path');
-		unshift @cmd, $self if $self;
-		if (wantarray) {
-			return command(@cmd, '--get-all', $var);
-		} else {
-			return command_oneline(@cmd, '--get', $var);
-		}
-	} catch Git::Error::Command with {
-		my $E = shift;
-		if ($E->value() == 1) {
-			# Key not found.
-			return undef;
-		} else {
-			throw $E;
-		}
-	};
+	return _config_common($self, $var, +{'kind' => '--path'});
 }
 
 =item config_int ( VARIABLE )
@@ -667,26 +642,12 @@ or 'g' in the config file will cause the value to be multiplied
 by 1024, 1048576 (1024^2), or 1073741824 (1024^3) prior to output.
 It would return C<undef> if configuration variable is not defined,
 
-This currently wraps command('config') so it is not so fast.
-
 =cut
 
 sub config_int {
 	my ($self, $var) = _maybe_self(@_);
-
-	try {
-		my @cmd = ('config', '--int', '--get', $var);
-		unshift @cmd, $self if $self;
-		return command_oneline(@cmd);
-	} catch Git::Error::Command with {
-		my $E = shift;
-		if ($E->value() == 1) {
-			# Key not found.
-			return undef;
-		} else {
-			throw $E;
-		}
-	};
+	my $val = scalar _config_common($self, $var, +{'kind' => '--int'});
+	return $val;
 }
 
 =item get_colorbool ( NAME )

Re: [PATCH 2/2] use new Git::config_path() for aliasesfile

[Cord Seele]

On Fri 30 Sep 2011 12:55:45 -0700, Junio C Hamano <gitster@pobox.com> wrote:

> I think the addition of "config --path" support is a good idea, but the
> resulting code suffers from too many cut&paste cruft across the config*
> family of methods.
> 
> How about doing a bit of refactoring, perhaps something like this, on top
> as a separate patch?

Sound very reasonable to me - unfortunately it's beyond my perl-scope to be of
much help here.

-- Cord

Re: [PATCH 2/2] use new Git::config_path() for aliasesfile

[Jakub Narebski]

On Fri, 30 Sep 2011, Junio C Hamano wrote:

> I think the addition of "config --path" support is a good idea, but the
> resulting code suffers from too many cut&paste cruft across the config*
> family of methods.
> 
> How about doing a bit of refactoring, perhaps something like this, on top
> as a separate patch?

This is a good idea, in my opinion.
 
> I tried to be careful to still forcing the "one value only" for config_bool
> and config_int, but extra sets of eyeballs would be needed.

We do have tests for that, have we?
 
[...]
> +Common subroutine to implement bulk of what the config* family of methods
> +do. This wraps command('config') so it is not so fast.

BTW. I wonder if it wouldn't be a good idea to restart work on Git::Config
with lazy/eager loading of the whole config, like git_get_project_config()
and git_parse_project_config() subroutines from gitweb do it - running 
"git config -z -l".  Though then --int/--bool/--path conversion would have
to be implemented in Perl...
  
>  =cut
>  
> -sub config {
> -	my ($self, $var) = _maybe_self(@_);
> -
> +sub _config_common {
> +	my ($self, $var, $opts) = _maybe_self(@_);
> +	
>  	try {
> -		my @cmd = ('config');
> +		my @cmd = ('config', $opts->{'kind'} ? @{$opts->{'kind'}} : ());

How it is supposed to work?

First, you probably want to check for "exists $opts->{'kind'}", or even
"defined $opts->{'kind'}".

Second, "@{$opts->{'kind'}}" assumes that $opts->{'kind'} is an array
reference (something we didn't check)... and it isn't.


BTW. why do you use hashref?  Do you plan for the future to pass more
options that 'kind'?

>  		unshift @cmd, $self if $self;
>  		if (wantarray) {
>  			return command(@cmd, '--get-all', $var);
> @@ -594,6 +590,21 @@ sub config {
>  			throw $E;
>  		}
>  	};
> +
> +}
> +
> +=item config ( VARIABLE )
> +
> +Retrieve the configuration C<VARIABLE> in the same manner as C<config>
> +does. In scalar context requires the variable to be set only one time
> +(exception is thrown otherwise), in array context returns allows the
> +variable to be set multiple times and returns all the values.
> +
> +=cut
> +
> +sub config {
> +	my ($self, $var) = _maybe_self(@_);
> +	return _config_common($self, $var, +{});

No need to pass an empty hash.  Perl has a feature called autovivification:
when an undefined variable is dereferenced, it gets silently upgraded to
an array or hash reference (depending of the type of the dereferencing).

It is un-Perlish to do so.

>  sub config_bool {
>  	my ($self, $var) = _maybe_self(@_);
[...]
> +	my $val = scalar _config_common($self, $var, {'kind' => '--bool'});
> +	return (defined $val && $val eq 'true');
>  }
[...]

>  sub config_path {
>  	my ($self, $var) = _maybe_self(@_);
[...]
> +	return _config_common($self, $var, +{'kind' => '--path'});
>  }

Why the difference between {'kind' => '--bool'} and +{'kind' => '--path'}?

> -This currently wraps command('config') so it is not so fast.
> -

Shouldn't this be mentioned somewhat, even indirectly?

-- 
Jakub Narebski
Poland

Re: [PATCH 2/2] use new Git::config_path() for aliasesfile

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> On Fri, 30 Sep 2011, Junio C Hamano wrote:
>
>> I think the addition of "config --path" support is a good idea, but the
>> resulting code suffers from too many cut&paste cruft across the config*
>> family of methods.
>> 
>> How about doing a bit of refactoring, perhaps something like this, on top
>> as a separate patch?
>
> This is a good idea, in my opinion.

Thanks.

>> I tried to be careful to still forcing the "one value only" for config_bool
>> and config_int, but extra sets of eyeballs would be needed.
>
> We do have tests for that, have we?

Perhaps, but I consider Perl "other peoples' problem" ;-) so...

> BTW. why do you use hashref?  Do you plan for the future to pass more
> options that 'kind'?

I don't, but other people might, and I didn't want to place undue burden
on them.

[PATCH/RFC 3/2] Refactor Git::config_*

[Jakub Narebski]

From: Junio C Hamano <gitster@pobox.com>

There is, especially with addition of Git::config_path(), much code
repetition in the Git::config_* family of subroutines.  Refactor
common parts of Git::config(), Git::config_bool(), Git::config_int()
and Git::config_path() into _config_common() subroutine.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This is version which has fixed style to be more Perl-ish, and which
actually works (i.e. t9700 passes).

I have also moved _config_common() after commands that use it, just like
it is done with other "private" methods (methods with names starting with
'_'), and excluded this private detail of implementation from docs.

 perl/Git.pm |   85 ++++++++++++++---------------------------------------------
 1 files changed, 20 insertions(+), 65 deletions(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index c279bfb..d6df2e8 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -562,7 +562,6 @@ sub wc_chdir {
 	$self->{opts}->{WorkingSubdir} = $subdir;
 }
 
-
 =item config ( VARIABLE )
 
 Retrieve the configuration C<VARIABLE> in the same manner as C<config>
@@ -570,30 +569,11 @@ does. In scalar context requires the variable to be set only one time
 (exception is thrown otherwise), in array context returns allows the
 variable to be set multiple times and returns all the values.
 
-This currently wraps command('config') so it is not so fast.
-
 =cut
 
 sub config {
 	my ($self, $var) = _maybe_self(@_);
-
-	try {
-		my @cmd = ('config');
-		unshift @cmd, $self if $self;
-		if (wantarray) {
-			return command(@cmd, '--get-all', $var);
-		} else {
-			return command_oneline(@cmd, '--get', $var);
-		}
-	} catch Git::Error::Command with {
-		my $E = shift;
-		if ($E->value() == 1) {
-			# Key not found.
-			return;
-		} else {
-			throw $E;
-		}
-	};
+	return _config_common($self, $var);
 }
 
 
@@ -603,60 +583,24 @@ Retrieve the bool configuration C<VARIABLE>. The return value
 is usable as a boolean in perl (and C<undef> if it's not defined,
 of course).
 
-This currently wraps command('config') so it is not so fast.
-
 =cut
 
 sub config_bool {
 	my ($self, $var) = _maybe_self(@_);
-
-	try {
-		my @cmd = ('config', '--bool', '--get', $var);
-		unshift @cmd, $self if $self;
-		my $val = command_oneline(@cmd);
-		return undef unless defined $val;
-		return $val eq 'true';
-	} catch Git::Error::Command with {
-		my $E = shift;
-		if ($E->value() == 1) {
-			# Key not found.
-			return undef;
-		} else {
-			throw $E;
-		}
-	};
+	my $val = scalar _config_common($self, $var, {'kind' => '--bool'});
+	return (defined $val && $val eq 'true');
 }
 
-
 =item config_path ( VARIABLE )
 
 Retrieve the path configuration C<VARIABLE>. The return value
 is an expanded path or C<undef> if it's not defined.
 
-This currently wraps command('config') so it is not so fast.
-
 =cut
 
 sub config_path {
 	my ($self, $var) = _maybe_self(@_);
-
-	try {
-		my @cmd = ('config', '--path');
-		unshift @cmd, $self if $self;
-		if (wantarray) {
-			return command(@cmd, '--get-all', $var);
-		} else {
-			return command_oneline(@cmd, '--get', $var);
-		}
-	} catch Git::Error::Command with {
-		my $E = shift;
-		if ($E->value() == 1) {
-			# Key not found.
-			return undef;
-		} else {
-			throw $E;
-		}
-	};
+	return _config_common($self, $var, {'kind' => '--path'});
 }
 
 =item config_int ( VARIABLE )
@@ -667,28 +611,39 @@ or 'g' in the config file will cause the value to be multiplied
 by 1024, 1048576 (1024^2), or 1073741824 (1024^3) prior to output.
 It would return C<undef> if configuration variable is not defined,
 
-This currently wraps command('config') so it is not so fast.
-
 =cut
 
 sub config_int {
 	my ($self, $var) = _maybe_self(@_);
+	return scalar _config_common($self, $var, {'kind' => '--int'});
+}
+
+# Common subroutine to implement bulk of what the config* family of methods
+# do. This wraps command('config') so it is not so fast.
+sub _config_common {
+	my ($self, $var, $opts) = _maybe_self(@_);
 
 	try {
-		my @cmd = ('config', '--int', '--get', $var);
+		my @cmd = ('config', $opts->{'kind'} ? $opts->{'kind'} : ());
 		unshift @cmd, $self if $self;
-		return command_oneline(@cmd);
+		if (wantarray) {
+			return command(@cmd, '--get-all', $var);
+		} else {
+			return command_oneline(@cmd, '--get', $var);
+		}
 	} catch Git::Error::Command with {
 		my $E = shift;
 		if ($E->value() == 1) {
 			# Key not found.
-			return undef;
+			return;
 		} else {
 			throw $E;
 		}
 	};
+
 }
 
+
 =item get_colorbool ( NAME )
 
 Finds if color should be used for NAMEd operation from the configuration,
-- 
1.7.6

Re: [PATCH/RFC 3/2] Refactor Git::config_*

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> This is version which has fixed style to be more Perl-ish, and which
> actually works (i.e. t9700 passes).
>
> I have also moved _config_common() after commands that use it, just like
> it is done with other "private" methods (methods with names starting with
> '_'), and excluded this private detail of implementation from docs.

It seems that this breaks many tests in t9xxx series for me, especially
the 9100 series that cover git-svn.

Re: [PATCH/RFC 3/2] Refactor Git::config_*

[Jakub Narebski]

On Mon, 17 Oct 2011, Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > This is version which has fixed style to be more Perl-ish, and which
> > actually works (i.e. t9700 passes).
> >
> > I have also moved _config_common() after commands that use it, just like
> > it is done with other "private" methods (methods with names starting with
> > '_'), and excluded this private detail of implementation from docs.
> 
> It seems that this breaks many tests in t9xxx series for me, especially
> the 9100 series that cover git-svn.

Sorry about that, I have ran only t9700-perl-git.sh, which runs all right.

The problem was with duplicated _maybe_self(@_), which should be run only
once.  t9700-perl-git.sh passed because it uses only (recommended) object
form, and not procedural form like git-svn.

The following amend fixes the issue for me:

-- >8 --
diff --git i/perl/Git.pm w/perl/Git.pm
index 8e52290..32f6533 100644
--- i/perl/Git.pm
+++ w/perl/Git.pm
@@ -653,7 +653,7 @@ sub config_int {
 # Common subroutine to implement bulk of what the config* family of methods
 # do. This wraps command('config') so it is not so fast.
 sub _config_common {
-	my ($self, $var, $opts) = _maybe_self(@_);
+	my ($self, $var, $opts) = @_;
 
 	try {
 		my @cmd = ('config', $opts->{'kind'} ? $opts->{'kind'} : ());
-- 8< --

I'll resend amended commit.
-- 
Jakub Narebski
Poland

Re: [PATCH/RFC 3/2] Refactor Git::config_*

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> The problem was with duplicated _maybe_self(@_), which should be run only
> once.  t9700-perl-git.sh passed because it uses only (recommended) object
> form, and not procedural form like git-svn.

Thanks.

I have a suspicion that a more standard implementation of _maybe_self()
would instead unshift a dummy singleton Git() instance instead of undef to
lift such a limitation, but perhaps the code relies on undef'ness of self
when called as a vanilla subroutine.

[PATCH/RFC 3/2 (fixed)] Refactor Git::config_*

[Jakub Narebski]

From: Junio C Hamano <gitster@pobox.com>

There is, especially with addition of Git::config_path(), much code
repetition in the Git::config_* family of subroutines.

Refactor common parts of Git::config(), Git::config_bool(),
Git::config_int() and Git::config_path() into _config_common()
helper method, reducing code duplication.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
Jakub Narebski wrote:
>
> I'll resend amended commit.

Here it is.

 perl/Git.pm |   74 ++++++++++++++--------------------------------------------
 1 files changed, 18 insertions(+), 56 deletions(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index c775b4f..8e1e2fd 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -576,24 +576,7 @@ This currently wraps command('config') so it is not so fast.
 
 sub config {
 	my ($self, $var) = _maybe_self(@_);
-
-	try {
-		my @cmd = ('config');
-		unshift @cmd, $self if $self;
-		if (wantarray) {
-			return command(@cmd, '--get-all', $var);
-		} else {
-			return command_oneline(@cmd, '--get', $var);
-		}
-	} catch Git::Error::Command with {
-		my $E = shift;
-		if ($E->value() == 1) {
-			# Key not found.
-			return;
-		} else {
-			throw $E;
-		}
-	};
+	return _config_common($self, $var);
 }
 
 
@@ -609,25 +592,10 @@ This currently wraps command('config') so it is not so fast.
 
 sub config_bool {
 	my ($self, $var) = _maybe_self(@_);
-
-	try {
-		my @cmd = ('config', '--bool', '--get', $var);
-		unshift @cmd, $self if $self;
-		my $val = command_oneline(@cmd);
-		return undef unless defined $val;
-		return $val eq 'true';
-	} catch Git::Error::Command with {
-		my $E = shift;
-		if ($E->value() == 1) {
-			# Key not found.
-			return undef;
-		} else {
-			throw $E;
-		}
-	};
+	my $val = scalar _config_common($self, $var, {'kind' => '--bool'});
+	return (defined $val && $val eq 'true');
 }
 
-
 =item config_path ( VARIABLE )
 
 Retrieve the path configuration C<VARIABLE>. The return value
@@ -639,24 +607,7 @@ This currently wraps command('config') so it is not so fast.
 
 sub config_path {
 	my ($self, $var) = _maybe_self(@_);
-
-	try {
-		my @cmd = ('config', '--path');
-		unshift @cmd, $self if $self;
-		if (wantarray) {
-			return command(@cmd, '--get-all', $var);
-		} else {
-			return command_oneline(@cmd, '--get', $var);
-		}
-	} catch Git::Error::Command with {
-		my $E = shift;
-		if ($E->value() == 1) {
-			# Key not found.
-			return undef;
-		} else {
-			throw $E;
-		}
-	};
+	return _config_common($self, $var, {'kind' => '--path'});
 }
 
 
@@ -705,16 +656,27 @@ This currently wraps command('config') so it is not so fast.
 
 sub config_int {
 	my ($self, $var) = _maybe_self(@_);
+	return scalar _config_common($self, $var, {'kind' => '--int'});
+}
+
+# Common subroutine to implement bulk of what the config* family of methods
+# do. This curently wraps command('config') so it is not so fast.
+sub _config_common {
+	my ($self, $var, $opts) = @_;
 
 	try {
-		my @cmd = ('config', '--int', '--get', $var);
+		my @cmd = ('config', $opts->{'kind'} ? $opts->{'kind'} : ());
 		unshift @cmd, $self if $self;
-		return command_oneline(@cmd);
+		if (wantarray) {
+			return command(@cmd, '--get-all', $var);
+		} else {
+			return command_oneline(@cmd, '--get', $var);
+		}
 	} catch Git::Error::Command with {
 		my $E = shift;
 		if ($E->value() == 1) {
 			# Key not found.
-			return undef;
+			return;
 		} else {
 			throw $E;
 		}
-- 
1.7.6

Re: [PATCH/RFC 3/2 (fixed)] Refactor Git::config_*

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> From: Junio C Hamano <gitster@pobox.com>
>
> There is, especially with addition of Git::config_path(), much code
> repetition in the Git::config_* family of subroutines.
>
> Refactor common parts of Git::config(), Git::config_bool(),
> Git::config_int() and Git::config_path() into _config_common()
> helper method, reducing code duplication.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> ---
> Jakub Narebski wrote:
>>
>> I'll resend amended commit.
>
> Here it is.

Well, this breaks t9001 and I ended up spending an hour and half figuring
out why. Admittedly, I was doing something else on the side, so it is not
like the whole 90 minutes is the billable hour for reviewing this patch,
but as far as the Git project is concerned, I didn't have any other branch
checked out in my working tree, so that whole time was what ended up
costing.

The real problem was here.

> @@ -609,25 +592,10 @@ This currently wraps command('config') so it is not so fast.
>  
>  sub config_bool {
>  	my ($self, $var) = _maybe_self(@_);
> -
> -	try {
> -		my @cmd = ('config', '--bool', '--get', $var);
> -		unshift @cmd, $self if $self;
> -		my $val = command_oneline(@cmd);
> -		return undef unless defined $val;
> -		return $val eq 'true';
> ...
> -	};
> +	my $val = scalar _config_common($self, $var, {'kind' => '--bool'});
> +	return (defined $val && $val eq 'true');
>  }

Can you spot the difference?

This is the reason why I do not particularly like a rewrite for the sake
of rewriting.

[PATCH/RFC 3/2 v3] Refactor Git::config_*

[Jakub Narebski]

On Tue, 18 Oct 2011, Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> > From: Junio C Hamano <gitster@pobox.com>
> >
> > There is, especially with addition of Git::config_path(), much code
> > repetition in the Git::config_* family of subroutines.
> >
> > Refactor common parts of Git::config(), Git::config_bool(),
> > Git::config_int() and Git::config_path() into _config_common()
> > helper method, reducing code duplication.
> >
> > Signed-off-by: Junio C Hamano <gitster@pobox.com>
> > Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> > ---
> > Jakub Narebski wrote:
> > >
> > > I'll resend amended commit.
> >
> > Here it is.
> 
> Well, this breaks t9001 and I ended up spending an hour and half figuring
> out why. Admittedly, I was doing something else on the side, so it is not
> like the whole 90 minutes is the billable hour for reviewing this patch,
> but as far as the Git project is concerned, I didn't have any other branch
> checked out in my working tree, so that whole time was what ended up
> costing.

I'm sorry about that.  I have checked t9700-perl-git.sh and 
t9100-git-svn-basic.sh that the version before fixup had problems with,
but for some reason I had many spurious test failures, so I have not run
the full test suite (well, it would be enough to run those that require
Git.pm).

Nb. I still have:

  Test Summary Report
  -------------------
  t1402-check-ref-format.sh                        (Wstat: 256 Tests: 93 Failed: 1)
    Failed test:  31
    Non-zero exit status: 1
  t4034-diff-words.sh                              (Wstat: 256 Tests: 34 Failed: 2)
    Failed tests:  21, 25
    Non-zero exit status: 1
 
> The real problem was here.
> 
> > @@ -609,25 +592,10 @@ This currently wraps command('config') so it is not so fast.
> >  
> >  sub config_bool {
> >  	my ($self, $var) = _maybe_self(@_);
> > -
> > -	try {
> > -		my @cmd = ('config', '--bool', '--get', $var);
> > -		unshift @cmd, $self if $self;
> > -		my $val = command_oneline(@cmd);
> > -		return undef unless defined $val;
> > -		return $val eq 'true';
> > ...
> > -	};
> > +	my $val = scalar _config_common($self, $var, {'kind' => '--bool'});
> > +	return (defined $val && $val eq 'true');
> >  }
> 
> Can you spot the difference?

Damn.

I really should have done refactoring from scratch myself, instead of basing
it on "how about that" throwaway patch.

Fixed now in the version below.
 
> This is the reason why I do not particularly like a rewrite for the sake
> of rewriting.

The goal was to reduce code duplication to _avoid_ errors.

What I have noticed is that there is slight difference between original
Git::config_path and the one after refactoring.  In the version before
this patch the error catching part of config_path() looks like this:

        } catch Git::Error::Command with {
                my $E = shift;
                if ($E->value() == 1) {
                        # Key not found.
                        return undef;
                } else {
                        throw $E;
                }
        };

while after this patch (and in config()) it looks like this:

        } catch Git::Error::Command with {
                my $E = shift;
                if ($E->value() == 1) {
                        # Key not found.
                        return;
                } else {
                        throw $E;
                }
        };

I am not sure which one is right, but I suspect the latter.
Cord?

-- >8 ------------ >8 -------------- >8 --
From: Junio C Hamano <gitster@pobox.com>

There is, especially with addition of Git::config_path(), much code
repetition in the Git::config_* family of subroutines.

Refactor common parts of Git::config(), Git::config_bool(),
Git::config_int() and Git::config_path() into _config_common()
helper method, reducing code duplication.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
 perl/Git.pm |   73 ++++++++++++++++------------------------------------------
 1 files changed, 20 insertions(+), 53 deletions(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index c279bfb..b7035ad 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -577,23 +577,7 @@ This currently wraps command('config') so it is not so fast.
 sub config {
 	my ($self, $var) = _maybe_self(@_);
 
-	try {
-		my @cmd = ('config');
-		unshift @cmd, $self if $self;
-		if (wantarray) {
-			return command(@cmd, '--get-all', $var);
-		} else {
-			return command_oneline(@cmd, '--get', $var);
-		}
-	} catch Git::Error::Command with {
-		my $E = shift;
-		if ($E->value() == 1) {
-			# Key not found.
-			return;
-		} else {
-			throw $E;
-		}
-	};
+	return _config_common($self, $var);
 }
 
 
@@ -610,24 +594,11 @@ This currently wraps command('config') so it is not so fast.
 sub config_bool {
 	my ($self, $var) = _maybe_self(@_);
 
-	try {
-		my @cmd = ('config', '--bool', '--get', $var);
-		unshift @cmd, $self if $self;
-		my $val = command_oneline(@cmd);
-		return undef unless defined $val;
-		return $val eq 'true';
-	} catch Git::Error::Command with {
-		my $E = shift;
-		if ($E->value() == 1) {
-			# Key not found.
-			return undef;
-		} else {
-			throw $E;
-		}
-	};
+	my $val = scalar _config_common($self, $var, {'kind' => '--bool'});
+	return undef unless defined $val;
+	return $val eq 'true';
 }
 
-
 =item config_path ( VARIABLE )
 
 Retrieve the path configuration C<VARIABLE>. The return value
@@ -640,23 +611,7 @@ This currently wraps command('config') so it is not so fast.
 sub config_path {
 	my ($self, $var) = _maybe_self(@_);
 
-	try {
-		my @cmd = ('config', '--path');
-		unshift @cmd, $self if $self;
-		if (wantarray) {
-			return command(@cmd, '--get-all', $var);
-		} else {
-			return command_oneline(@cmd, '--get', $var);
-		}
-	} catch Git::Error::Command with {
-		my $E = shift;
-		if ($E->value() == 1) {
-			# Key not found.
-			return undef;
-		} else {
-			throw $E;
-		}
-	};
+	return _config_common($self, $var, {'kind' => '--path'});
 }
 
 =item config_int ( VARIABLE )
@@ -674,15 +629,27 @@ This currently wraps command('config') so it is not so fast.
 sub config_int {
 	my ($self, $var) = _maybe_self(@_);
 
+	return scalar _config_common($self, $var, {'kind' => '--int'});
+}
+
+# Common subroutine to implement bulk of what the config* family of methods
+# do. This curently wraps command('config') so it is not so fast.
+sub _config_common {
+	my ($self, $var, $opts) = @_;
+
 	try {
-		my @cmd = ('config', '--int', '--get', $var);
+		my @cmd = ('config', $opts->{'kind'} ? $opts->{'kind'} : ());
 		unshift @cmd, $self if $self;
-		return command_oneline(@cmd);
+		if (wantarray) {
+			return command(@cmd, '--get-all', $var);
+		} else {
+			return command_oneline(@cmd, '--get', $var);
+		}
 	} catch Git::Error::Command with {
 		my $E = shift;
 		if ($E->value() == 1) {
 			# Key not found.
-			return undef;
+			return;
 		} else {
 			throw $E;
 		}
-- 
1.7.6

Re: [PATCH/RFC 3/2 v3] Refactor Git::config_*

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

>> Well, this breaks t9001 and I ended up spending an hour and half figuring
>> out why. Admittedly, I was doing something else on the side, so it is not
>> like the whole 90 minutes is the billable hour for reviewing this patch,
>> but as far as the Git project is concerned, I didn't have any other branch
>> checked out in my working tree, so that whole time was what ended up
>> costing.
>
> I'm sorry about that.

No need to be sorry. After all you just re-sent a throw-away patch with
minor tweaks, but unfortunately a tricky Perl script sometimes needs line
by line inspection with fine toothed comb to avoid regression.

>> The real problem was here.
>> ...
>> > -		my $val = command_oneline(@cmd);
>> > -		return undef unless defined $val;
>> > -		return $val eq 'true';
>> > ...
>> > -	};
>> > +	my $val = scalar _config_common($self, $var, {'kind' => '--bool'});
>> > +	return (defined $val && $val eq 'true');
>> >  }
>> 
>> Can you spot the difference?
>
> Damn.

For people following from the sideline, the difference that bit us was
that

    return $val eq 'true';

yields "" (not undef) for false, but some callers care the distinction
between undef kind of false and other kinds of false. It is not really the
fault of the language per-se, but still... 

>> This is the reason why I do not particularly like a rewrite for the sake
>> of rewriting.
>
> The goal was to reduce code duplication to _avoid_ errors.

That is exactly why I said I do not like a rewrite for such purpose.

Once the end result of smaller code is done correctly, it would help
avoiding future errors, but people tend to think unconsciously that the
change to reduce code duplication is much safer than adding new code.
Upon receiving such a patch, without knowing that it was not done with
enough attention to detail with fine toothed comb, it is me who ends up
spending nontrivial amount of time fixing the breakage. These "clean-ups"
are not cheap.

> What I have noticed is that there is slight difference between original
> Git::config_path and the one after refactoring.  In the version before
> this patch the error catching part of config_path() looks like this:
> ...
>                         return undef;
> ...
> while after this patch (and in config()) it looks like this:
> ...
>                         return;
> ...

> I am not sure which one is right, but I suspect the latter.

This is Perl---"return;" returns undef, so there is no right or wrong.

Having said that, I tend to prefer being explicit so that people not so
familiar with the language do not have to waste time wondering about such
differences. Especially where it matters, like this case where some
callers may care about different kinds of falsehood.

That is another reason I tend to hate the kind of "this makes it more
Perl-ish" changes, as they tend to force readers to spend extra brain
cells to see what is going on. I'd rather spell things more explicit,
especially when the distinction matters.

I've already pushed out the fixed one as 6942a3d (libperl-git: refactor
Git::config_*, 2011-10-18), with this bit:

    - ...
    -               my $val = command_oneline(@cmd);
    -               return undef unless defined $val;
    +       # Do not rewrite this as return (defined $val && $val eq 'true')
    +       # as some callers do care what kind of falsehood they receive.
    +       if (!defined $val) {
    +               return undef;
    +       } else {
                    return $val eq 'true';
      ...

Re: [PATCH/RFC 3/2 v3] Refactor Git::config_*

[Jakub Narebski]

Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
>>> The real problem was here.
>>> ...
>>>> -		my $val = command_oneline(@cmd);
>>>> -		return undef unless defined $val;
>>>> -		return $val eq 'true';
>>>> ...
>>>> -	};
>>>> +	my $val = scalar _config_common($self, $var, {'kind' => '--bool'});
>>>> +	return (defined $val && $val eq 'true');
>>>>  }
>>> 
>>> Can you spot the difference?
>>
>> Damn.
> 
> For people following from the sideline, the difference that bit us was
> that
> 
>     return $val eq 'true';
> 
> yields "" (not undef) for false, but some callers care the distinction
> between undef kind of false and other kinds of false. It is not really the
> fault of the language per-se, but still... 

And Git::config* family of commands return 'undef' if key was not found.
So Git::config_bool() can return true, false or undef, and the last part
is important. 
 
>> What I have noticed is that there is slight difference between original
>> Git::config_path and the one after refactoring.  In the version before
>> this patch the error catching part of config_path() looks like this:
>> ...
>>                         return undef;
>> ...
>> while after this patch (and in config()) it looks like this:
>> ...
>>                         return;
>> ...
>> 
>> I am not sure which one is right, but I suspect the latter.
> 
> This is Perl---"return;" returns undef, so there is no right or wrong.

Actually this is not true: "return;" returns undef in scalar context,
and empty list in list context.  This means that result always evaluates
to false...
 
> Having said that, I tend to prefer being explicit so that people not so
> familiar with the language do not have to waste time wondering about such
> differences. Especially where it matters, like this case where some
> callers may care about different kinds of falsehood.
> 
> That is another reason I tend to hate the kind of "this makes it more
> Perl-ish" changes, as they tend to force readers to spend extra brain
> cells to see what is going on. I'd rather spell things more explicit,
> especially when the distinction matters.

...and that is why "return;" is more Perl-ish, and usually safer.

There might be exceptions where we want to return one-element list
containing single undef element in list context, but I guess they
are exceedingly rare.
 
> I've already pushed out the fixed one as 6942a3d (libperl-git: refactor
> Git::config_*, 2011-10-18), with this bit:
> 
>     - ...
>     -               my $val = command_oneline(@cmd);
>     -               return undef unless defined $val;
>     +       # Do not rewrite this as return (defined $val && $val eq 'true')
>     +       # as some callers do care what kind of falsehood they receive.
>     +       if (!defined $val) {
>     +               return undef;
>     +       } else {
>                     return $val eq 'true';
>       ...

I like mine better... ;-))))

-- 
Jakub Narebski
Poland