Re: [PATCH] gitweb: make feature_blame return a list

Matt Kraai [off-list ref] writes:

My Perl may be getting rusty, but does the above make any difference?
How?

On Monday 15 December 2008 23:20, Junio C Hamano wrote:

Matt Kraai [off-list ref] writes:

quoted

The feature defaults are expected to be a list, but feature_blame was
returning a scalar.  This change makes it consistent with the other
boolean feature subroutines.

Signed-off-by: Matt Kraai <redacted>
---
 gitweb/gitweb.perl |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 6eb370d..145e712 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl

@@ -367,12 +367,12 @@ sub feature_blame {
     my ($val) = git_get_project_config('blame', '--bool');
 
     if ($val eq 'true') {
-            return 1;
+            return (1);
     } elsif ($val eq 'false') {
-            return 0;
+            return (0);
     }
 
-    return $_[0];
+    return ($_[0]);
 }

My Perl may be getting rusty, but does the above make any difference?
How?

It's formally more correct and makes the blame feature fit with the
general feature framework, although the feature works correctly even
without the (). I was actually going to send a similar patch myself, 
having missed it during the cleanup and get/check splitup patchset.

Matt's patch gets my Ack, for what it's worth.


-- 
Giuseppe "Oblomov" Bilotta

On Mon, Dec 15, 2008 at 02:20:03PM -0800, Junio C Hamano wrote:

Matt Kraai [off-list ref] writes:

quoted

--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl

@@ -367,12 +367,12 @@ sub feature_blame {
 	my ($val) = git_get_project_config('blame', '--bool');
 
 	if ($val eq 'true') {
-		return 1;
+		return (1);
 	} elsif ($val eq 'false') {
-		return 0;
+		return (0);
 	}
 
-	return $_[0];
+	return ($_[0]);
 }

My Perl may be getting rusty, but does the above make any difference?

I'm still relatively new to Perl, but at least in my limited testing,
I couldn't generate a case in which these different constructs had
different results.

I made this change so that all of the boolean feature subroutines
would have the same body, modulo the name of the option; that way,
replacing them with a single routine only involves making the option
name a parameter.

If you'd like me to resubmit my second patch, I'm happy to do so.
Just let me know whether you prefer the resulting function to wrap its
return values in parentheses (as is currently done by feature_grep and
feature_pickaxe) or not.

-- 
Matt                                                 http://ftbfs.org/