* [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).