Re: Re* [PATCH] gitweb: make remote_heads config setting work.
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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re* [PATCH] gitweb: make remote_heads config setting work.
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) {
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gitweb: make remote_heads config setting work.
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gitweb: make remote_heads config setting work.
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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gitweb: make remote_heads config setting work.
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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
