# [PATCH] gitweb: make remote_heads config setting work. 6 messages from 2012-11-05 to 2012-11-21. Participants: Phil Pennock, Jeff King, Junio C Hamano. Thread: https://gitlist.dev/t/32026 ## Phil Pennock, 2012-11-05 23:50 Subject: [PATCH] gitweb: make remote_heads config setting work. Message-ID: <20121105235047.GA78156@redoubt.spodhuis.org> URL: https://gitlist.dev/e/20121105235047.GA78156%40redoubt.spodhuis.org ``` 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 " --- gitweb/gitweb.perl | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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, 2012-11-08 16:56 Subject: Re: [PATCH] gitweb: make remote_heads config setting work. Message-ID: <20121108165648.GG15560@sigill.intra.peff.net> URL: https://gitlist.dev/e/20121108165648.GG15560%40sigill.intra.peff.net In-Reply-To: <20121105235047.GA78156@redoubt.spodhuis.org> ``` On Mon, Nov 05, 2012 at 06:50:47PM -0500, Phil Pennock wrote: > 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, 2012-11-09 04:40 Subject: Re: [PATCH] gitweb: make remote_heads config setting work. Message-ID: <7vk3tvqthw.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vk3tvqthw.fsf%40alter.siamese.dyndns.org In-Reply-To: <20121105235047.GA78156@redoubt.spodhuis.org> ``` Phil Pennock writes: > @@ -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, 2012-11-09 16:37 Subject: Re: [PATCH] gitweb: make remote_heads config setting work. Message-ID: <20121109163710.GD19725@sigill.intra.peff.net> URL: https://gitlist.dev/e/20121109163710.GD19725%40sigill.intra.peff.net In-Reply-To: <7vk3tvqthw.fsf@alter.siamese.dyndns.org> ``` 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. -Peff ``` ## Junio C Hamano, 2012-11-20 22:21 Subject: Re* [PATCH] gitweb: make remote_heads config setting work. Message-ID: <7vpq37rk3v.fsf_-_@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vpq37rk3v.fsf_-_%40alter.siamese.dyndns.org In-Reply-To: <20121109163710.GD19725@sigill.intra.peff.net> ``` Jeff King writes: > 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 --- gitweb/gitweb.perl | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) 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, 2012-11-21 19:31 Subject: Re: Re* [PATCH] gitweb: make remote_heads config setting work. Message-ID: <20121121193115.GB16280@sigill.intra.peff.net> URL: https://gitlist.dev/e/20121121193115.GB16280%40sigill.intra.peff.net In-Reply-To: <7vpq37rk3v.fsf_-_@alter.siamese.dyndns.org> ``` On Tue, Nov 20, 2012 at 02:21:40PM -0800, Junio C Hamano wrote: > > 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 ```