git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Jakub Narebski <jnareb@gmail.com>
Date
Oct 18, 2011, 22:09 UTC
Message-ID
<201110190009.40470.jnareb@gmail.com>
In-Reply-To
<7vty76f57d.fsf@alter.siamese.dyndns.org>
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(-)
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
Previous: Junio C HamanoNext: Junio C Hamano
Message 22 of 24 in “git-send-email.perl: expand filename of aliasesfile”
  1. git-send-email.perl: expand filename of aliasesfileCord Seele, Sep 28, 2011
  2. Matthieu MoySep 28, 2011
  3. Cord SeeleSep 28, 2011
  4. Matthieu MoySep 28, 2011
  5. git-send-email: allow filename expansionCord Seele, Sep 30, 2011
  6. 1/2 Add Git::config_path()Cord Seele, Sep 30, 2011
  7. Jakub NarebskiOct 7, 2011
  8. Junio C HamanoOct 7, 2011
  9. Jakub NarebskiOct 7, 2011
  10. Junio C HamanoOct 7, 2011
  11. 2/2 use new Git::config_path() for aliasesfileCord Seele, Sep 30, 2011
  12. Junio C HamanoSep 30, 2011
  13. Cord SeeleSep 30, 2011
  14. Jakub NarebskiSep 30, 2011
  15. Junio C HamanoSep 30, 2011
  16. 3/2 Refactor Git::config_*Jakub Narebski, Oct 7, 2011
  17. Junio C HamanoOct 17, 2011
  18. Jakub NarebskiOct 17, 2011
  19. Junio C HamanoOct 17, 2011
  20. 3/2 Refactor Git::config_*Jakub Narebski, Oct 18, 2011
  21. Junio C HamanoOct 18, 2011
  22. 3/2 Refactor Git::config_*Jakub Narebski, Oct 18, 2011
  23. Junio C HamanoOct 18, 2011
  24. Jakub NarebskiOct 19, 2011

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.