* [PATCH] gitweb: make remote_heads config setting work. @ 2012-11-05 23:50 Phil Pennock 2012-11-08 16:56 ` Jeff King 2012-11-09 4:40 ` Junio C Hamano 0 siblings, 2 replies; 6+ messages in thread From: Phil Pennock @ 2012-11-05 23:50 UTC (permalink / raw) To: git, gitster 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(-) 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] gitweb: make remote_heads config setting work. 2012-11-05 23:50 [PATCH] gitweb: make remote_heads config setting work Phil Pennock @ 2012-11-08 16:56 ` Jeff King 2012-11-09 4:40 ` Junio C Hamano 1 sibling, 0 replies; 6+ messages in thread From: Jeff King @ 2012-11-08 16:56 UTC (permalink / raw) To: Phil Pennock; +Cc: git, gitster 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gitweb: make remote_heads config setting work. 2012-11-05 23:50 [PATCH] gitweb: make remote_heads config setting work Phil Pennock 2012-11-08 16:56 ` Jeff King @ 2012-11-09 4:40 ` Junio C Hamano 2012-11-09 16:37 ` Jeff King 1 sibling, 1 reply; 6+ messages in thread From: Junio C Hamano @ 2012-11-09 4:40 UTC (permalink / raw) To: Phil Pennock; +Cc: git, peff Phil Pennock <phil@apcera.com> 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gitweb: make remote_heads config setting work. 2012-11-09 4:40 ` Junio C Hamano @ 2012-11-09 16:37 ` Jeff King 2012-11-20 22:21 ` Re* " Junio C Hamano 0 siblings, 1 reply; 6+ messages in thread From: Jeff King @ 2012-11-09 16:37 UTC (permalink / raw) To: Junio C Hamano; +Cc: Phil Pennock, git 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re* [PATCH] gitweb: make remote_heads config setting work. 2012-11-09 16:37 ` Jeff King @ 2012-11-20 22:21 ` Junio C Hamano 2012-11-21 19:31 ` Jeff King 0 siblings, 1 reply; 6+ messages in thread From: Junio C Hamano @ 2012-11-20 22:21 UTC (permalink / raw) To: Jeff King; +Cc: Phil Pennock, git Jeff King <peff@peff.net> 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 <gitster@pobox.com> --- 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) { ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: Re* [PATCH] gitweb: make remote_heads config setting work. 2012-11-20 22:21 ` Re* " Junio C Hamano @ 2012-11-21 19:31 ` Jeff King 0 siblings, 0 replies; 6+ messages in thread From: Jeff King @ 2012-11-21 19:31 UTC (permalink / raw) To: Junio C Hamano; +Cc: Phil Pennock, git 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-11-21 19:31 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-11-05 23:50 [PATCH] gitweb: make remote_heads config setting work Phil Pennock 2012-11-08 16:56 ` Jeff King 2012-11-09 4:40 ` Junio C Hamano 2012-11-09 16:37 ` Jeff King 2012-11-20 22:21 ` Re* " Junio C Hamano 2012-11-21 19:31 ` Jeff King
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).