[PATCH/RFC 3/2 v3] Refactor Git::config_*
On Tue, 18 Oct 2011, Junio C Hamano wrote:
Show 25 quoted lines
> 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
Show 20 quoted lines
> 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(-)
Show changes to perl/Git.pm +20 −53
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
