threads / patch / 32026

patchgitweb: make remote_heads config setting work.

Subject: [PATCH] gitweb: make remote_heads config setting work.

## tl;dr

6 messages between Nov 5, 2012 and Nov 21, 2012. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Phil Pennock· Nov 5, 2012, 23:50 UTC · lore

Git configuration items can not contain underscores in their name; the 'remote_heads' feature can not be enabled on a per-repository basis with that name.

This changes the git-config option to be `gitweb.remoteheads` but does not change the gitweb.conf option, to avoid backwards compatibility issues. We strip underscores from keys before looking through git-config output for them.

Signed-off-by: Phil Pennock <phil@apcera.com>"
---
 gitweb/gitweb.perl |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Show changes to gitweb/gitweb.perl +2 −1

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 10ed9e5..f2144c8 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -541,7 +541,7 @@ our %feature = (
 	# $feature{'remote_heads'}{'default'} = [1];
 	# To have project specific config enable override in $GITWEB_CONFIG
 	# $feature{'remote_heads'}{'override'} = 1;
-	# and in project config gitweb.remote_heads = 0|1;
+	# and in project config gitweb.remoteheads = 0|1;
 	'remote_heads' => {
 		'sub' => sub { feature_bool('remote_heads', @_) },
 		'override' => 0,
@@ -2702,6 +2702,7 @@ sub git_get_project_config {
 		$key = lc($key);
 	}
 	$key =~ s/^gitweb\.//;
+	$key =~ s/_//g;
 	return if ($key =~ m/\W/);
 
 	# type sanity check

-- 
1.7.10.3

Jeff King· Nov 8, 2012, 16:56 UTC · re: Phil Pennock · lore

Re: [PATCH] gitweb: make remote_heads config setting work.

On Mon, Nov 05, 2012 at 06:50:47PM -0500, Phil Pennock wrote:

Show 8 quoted lines

> Git configuration items can not contain underscores in their name; the
> 'remote_heads' feature can not be enabled on a per-repository basis with
> that name.
> 
> This changes the git-config option to be `gitweb.remoteheads` but does
> not change the gitweb.conf option, to avoid backwards compatibility
> issues.  We strip underscores from keys before looking through
> git-config output for them.

Makes sense. Thanks for considering the backwards compatibility angle. Hopefully we can avoid adding names with underscores in the future, but I think the mapping of "remote_head -> remotehead" is obvious enough that we do not need to worry about deprecating and replacing the old name.

-Peff

Junio C Hamano· Nov 9, 2012, 04:40 UTC · re: Phil Pennock · lore

Re: [PATCH] gitweb: make remote_heads config setting work.

Phil Pennock <phil@apcera.com> writes:

Show 8 quoted lines

> @@ -2702,6 +2702,7 @@ sub git_get_project_config {
>  		$key = lc($key);
>  	}
>  	$key =~ s/^gitweb\.//;
> +	$key =~ s/_//g;
>  	return if ($key =~ m/\W/);
>  
>  	# type sanity check

The idea to strip "_" from "remote_heads" to create "remoteheads" is fine, but I am not sure if the implementation is good.

Looking at the code before this part:

	if (my ($hi, $mi, $lo) = ($key =~ /^([^.]*)\.(.*)\.([^.]*)$/)) {
		$key = join(".", lc($hi), $mi, lc($lo));
	} else {
		$key = lc($key);
	}
	$key =~ s/^gitweb\.//;
	return if ($key =~ m/\W/);

the new code is munding the $hi and $mi parts, while the mistaken configuration this patch is trying to correct is about the $lo part, and possibly the $hi part, but never the $mi part.

It is expected that $hi will always be gitweb, and I suspect that there may not be any "per something" configuration variable (e.g. "gitweb.master.blame" may be used to override the default value given by "gitweb.blame" only for the master branch), that uses $mi part, so it might not matter to munge the entire $key like this patch does with the current code.

But that makes it even more important to get this part right _now_; otherwise, it is likely that this new code will introduce a bug to a future patch that adds "per something" configuration support.

Jeff King· Nov 9, 2012, 16:37 UTC · re: Junio C Hamano · lore

Re: [PATCH] gitweb: make remote_heads config setting work.

On Thu, Nov 08, 2012 at 08:40:11PM -0800, Junio C Hamano wrote:

Show 13 quoted lines

> Looking at the code before this part:
> 
> 	if (my ($hi, $mi, $lo) = ($key =~ /^([^.]*)\.(.*)\.([^.]*)$/)) {
> 		$key = join(".", lc($hi), $mi, lc($lo));
> 	} else {
> 		$key = lc($key);
> 	}
> 	$key =~ s/^gitweb\.//;
> 	return if ($key =~ m/\W/);
> 
> the new code is munding the $hi and $mi parts, while the mistaken
> configuration this patch is trying to correct is about the $lo part,
> and possibly the $hi part, but never the $mi part.

Good catch. I think the "return" in the existing code suffers from the same problem: it will bail on non-word characters in the $mi part, but that part should allow arbitrary characters.

-Peff

Junio C Hamano· Nov 20, 2012, 22:21 UTC · re: Jeff King · lore

Re* [PATCH] gitweb: make remote_heads config setting work.

Jeff King <peff@peff.net> writes:

Show 19 quoted lines

> On Thu, Nov 08, 2012 at 08:40:11PM -0800, Junio C Hamano wrote:
>
>> Looking at the code before this part:
>> 
>> 	if (my ($hi, $mi, $lo) = ($key =~ /^([^.]*)\.(.*)\.([^.]*)$/)) {
>> 		$key = join(".", lc($hi), $mi, lc($lo));
>> 	} else {
>> 		$key = lc($key);
>> 	}
>> 	$key =~ s/^gitweb\.//;
>> 	return if ($key =~ m/\W/);
>> 
>> the new code is munding the $hi and $mi parts, while the mistaken
>> configuration this patch is trying to correct is about the $lo part,
>> and possibly the $hi part, but never the $mi part.
>
> Good catch. I think the "return" in the existing code suffers from the
> same problem: it will bail on non-word characters in the $mi part, but
> that part should allow arbitrary characters.

I am tired of keeping the "expecting reroll" entries and having to worry about them, so let's do this

-- >8 --
Subject: [squash] gitweb: make remote_heads config setting work

Only the top-level and bottom-level names are case insensitive and spelled without "_". Protect future support of subsection names from name mangling.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 gitweb/gitweb.perl | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Show changes to diff +4 −2

diff --git c/gitweb/gitweb.perl w/gitweb/gitweb.perl
index f2144c8..c421fa4 100755
--- c/gitweb/gitweb.perl
+++ w/gitweb/gitweb.perl
@@ -2697,13 +2697,15 @@ sub git_get_project_config {
 	# only subsection, if exists, is case sensitive,
 	# and not lowercased by 'git config -z -l'
 	if (my ($hi, $mi, $lo) = ($key =~ /^([^.]*)\.(.*)\.([^.]*)$/)) {
+		$lo =~ s/_//g;
 		$key = join(".", lc($hi), $mi, lc($lo));
+		return if ($lo =~ /\W/ || $hi =~ /\W/);
 	} else {
 		$key = lc($key);
+		$key =~ s/_//g;
+		return if ($key =~ /\W/);
 	}
 	$key =~ s/^gitweb\.//;
-	$key =~ s/_//g;
-	return if ($key =~ m/\W/);
 
 	# type sanity check
 	if (defined $type) {

Jeff King· Nov 21, 2012, 19:31 UTC · re: Junio C Hamano · lore

Re: Re* [PATCH] gitweb: make remote_heads config setting work.

On Tue, Nov 20, 2012 at 02:21:40PM -0800, Junio C Hamano wrote:

Show 13 quoted lines

> > Good catch. I think the "return" in the existing code suffers from the
> > same problem: it will bail on non-word characters in the $mi part, but
> > that part should allow arbitrary characters.
> 
> I am tired of keeping the "expecting reroll" entries and having to
> worry about them, so let's do this
> 
> -- >8 --
> Subject: [squash] gitweb: make remote_heads config setting work
> 
> Only the top-level and bottom-level names are case insensitive and
> spelled without "_".  Protect future support of subsection names
> from name mangling.

I think the end result is fine, though you are really fixing a bug here (the /\W/ check is moved up), and then also putting the remote_heads fix (s/_//g) into the right place. IOW, if you are going to squash, you should probably note the bug-fix part in the commit message explicitly.

-Peff