{"thread":{"id":"32026","subject":"[PATCH] gitweb: make remote_heads config setting work.","startedAt":"2012-11-05T23:50:47Z","lastAt":"2012-11-21T19:31:15Z","messageCount":6,"participants":["Phil Pennock","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"202575","messageId":"20121105235047.GA78156@redoubt.spodhuis.org","threadId":"32026","inReplyTo":null,"subject":"[PATCH] gitweb: make remote_heads config setting work.","fromName":"Phil Pennock","fromEmail":"phil@apcera.com","sentAt":"2012-11-05T23:50:47Z","receivedAt":"2012-11-05T23:50:47Z","isPatch":true,"sender":{"key":"phil@apcera.com","avatar":null},"body":"Git configuration items can not contain underscores in their name; the\n'remote_heads' feature can not be enabled on a per-repository basis with\nthat name.\n\nThis changes the git-config option to be `gitweb.remoteheads` but does\nnot change the gitweb.conf option, to avoid backwards compatibility\nissues.  We strip underscores from keys before looking through\ngit-config output for them.\n\nSigned-off-by: Phil Pennock <phil@apcera.com>\"\n---\n gitweb/gitweb.perl |    3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 10ed9e5..f2144c8 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -541,7 +541,7 @@ our %feature = (\n \t# $feature{'remote_heads'}{'default'} = [1];\n \t# To have project specific config enable override in $GITWEB_CONFIG\n \t# $feature{'remote_heads'}{'override'} = 1;\n-\t# and in project config gitweb.remote_heads = 0|1;\n+\t# and in project config gitweb.remoteheads = 0|1;\n \t'remote_heads' => {\n \t\t'sub' => sub { feature_bool('remote_heads', @_) },\n \t\t'override' => 0,\n@@ -2702,6 +2702,7 @@ sub git_get_project_config {\n \t\t$key = lc($key);\n \t}\n \t$key =~ s/^gitweb\\.//;\n+\t$key =~ s/_//g;\n \treturn if ($key =~ m/\\W/);\n \n \t# type sanity check\n-- \n1.7.10.3\n"},{"id":"202654","messageId":"20121108165648.GG15560@sigill.intra.peff.net","threadId":"32026","inReplyTo":"20121105235047.GA78156@redoubt.spodhuis.org","subject":"Re: [PATCH] gitweb: make remote_heads config setting work.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-08T16:56:48Z","receivedAt":"2012-11-08T16:56:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 05, 2012 at 06:50:47PM -0500, Phil Pennock wrote:\n\n> Git configuration items can not contain underscores in their name; the\n> 'remote_heads' feature can not be enabled on a per-repository basis with\n> that name.\n> \n> This changes the git-config option to be `gitweb.remoteheads` but does\n> not change the gitweb.conf option, to avoid backwards compatibility\n> issues.  We strip underscores from keys before looking through\n> git-config output for them.\n\nMakes sense. Thanks for considering the backwards compatibility angle.\nHopefully we can avoid adding names with underscores in the future, but I\nthink the mapping of \"remote_head -> remotehead\" is obvious enough that\nwe do not need to worry about deprecating and replacing the old name.\n\n-Peff\n"},{"id":"202682","messageId":"7vk3tvqthw.fsf@alter.siamese.dyndns.org","threadId":"32026","inReplyTo":"20121105235047.GA78156@redoubt.spodhuis.org","subject":"Re: [PATCH] gitweb: make remote_heads config setting work.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-11-09T04:40:11Z","receivedAt":"2012-11-09T04:40:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phil Pennock <phil@apcera.com> writes:\n\n> @@ -2702,6 +2702,7 @@ sub git_get_project_config {\n>  \t\t$key = lc($key);\n>  \t}\n>  \t$key =~ s/^gitweb\\.//;\n> +\t$key =~ s/_//g;\n>  \treturn if ($key =~ m/\\W/);\n>  \n>  \t# type sanity check\n\nThe idea to strip \"_\" from \"remote_heads\" to create \"remoteheads\" is\nfine, but I am not sure if the implementation is good.\n\nLooking at the code before this part:\n\n\tif (my ($hi, $mi, $lo) = ($key =~ /^([^.]*)\\.(.*)\\.([^.]*)$/)) {\n\t\t$key = join(\".\", lc($hi), $mi, lc($lo));\n\t} else {\n\t\t$key = lc($key);\n\t}\n\t$key =~ s/^gitweb\\.//;\n\treturn if ($key =~ m/\\W/);\n\nthe new code is munding the $hi and $mi parts, while the mistaken\nconfiguration this patch is trying to correct is about the $lo part,\nand possibly the $hi part, but never the $mi part.\n\nIt is expected that $hi will always be gitweb, and I suspect that\nthere may not be any \"per something\" configuration variable (e.g.\n\"gitweb.master.blame\" may be used to override the default value\ngiven by \"gitweb.blame\" only for the master branch), that uses $mi\npart, so it might not matter to munge the entire $key like this\npatch does with the current code.\n\nBut that makes it even more important to get this part right _now_;\notherwise, it is likely that this new code will introduce a bug to\na future patch that adds \"per something\" configuration support.\n"},{"id":"202707","messageId":"20121109163710.GD19725@sigill.intra.peff.net","threadId":"32026","inReplyTo":"7vk3tvqthw.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] gitweb: make remote_heads config setting work.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-09T16:37:10Z","receivedAt":"2012-11-09T16:37:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 08, 2012 at 08:40:11PM -0800, Junio C Hamano wrote:\n\n> Looking at the code before this part:\n> \n> \tif (my ($hi, $mi, $lo) = ($key =~ /^([^.]*)\\.(.*)\\.([^.]*)$/)) {\n> \t\t$key = join(\".\", lc($hi), $mi, lc($lo));\n> \t} else {\n> \t\t$key = lc($key);\n> \t}\n> \t$key =~ s/^gitweb\\.//;\n> \treturn if ($key =~ m/\\W/);\n> \n> the new code is munding the $hi and $mi parts, while the mistaken\n> configuration this patch is trying to correct is about the $lo part,\n> and possibly the $hi part, but never the $mi part.\n\nGood catch. I think the \"return\" in the existing code suffers from the\nsame problem: it will bail on non-word characters in the $mi part, but\nthat part should allow arbitrary characters.\n\n-Peff\n"},{"id":"203576","messageId":"7vpq37rk3v.fsf_-_@alter.siamese.dyndns.org","threadId":"32026","inReplyTo":"20121109163710.GD19725@sigill.intra.peff.net","subject":"Re* [PATCH] gitweb: make remote_heads config setting work.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-11-20T22:21:40Z","receivedAt":"2012-11-20T22:21:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Thu, Nov 08, 2012 at 08:40:11PM -0800, Junio C Hamano wrote:\n>\n>> Looking at the code before this part:\n>> \n>> \tif (my ($hi, $mi, $lo) = ($key =~ /^([^.]*)\\.(.*)\\.([^.]*)$/)) {\n>> \t\t$key = join(\".\", lc($hi), $mi, lc($lo));\n>> \t} else {\n>> \t\t$key = lc($key);\n>> \t}\n>> \t$key =~ s/^gitweb\\.//;\n>> \treturn if ($key =~ m/\\W/);\n>> \n>> the new code is munding the $hi and $mi parts, while the mistaken\n>> configuration this patch is trying to correct is about the $lo part,\n>> and possibly the $hi part, but never the $mi part.\n>\n> Good catch. I think the \"return\" in the existing code suffers from the\n> same problem: it will bail on non-word characters in the $mi part, but\n> that part should allow arbitrary characters.\n\nI am tired of keeping the \"expecting reroll\" entries and having to\nworry about them, so let's do this\n\n-- >8 --\nSubject: [squash] gitweb: make remote_heads config setting work\n\nOnly the top-level and bottom-level names are case insensitive and\nspelled without \"_\".  Protect future support of subsection names\nfrom name mangling.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n gitweb/gitweb.perl | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git c/gitweb/gitweb.perl w/gitweb/gitweb.perl\nindex f2144c8..c421fa4 100755\n--- c/gitweb/gitweb.perl\n+++ w/gitweb/gitweb.perl\n@@ -2697,13 +2697,15 @@ sub git_get_project_config {\n \t# only subsection, if exists, is case sensitive,\n \t# and not lowercased by 'git config -z -l'\n \tif (my ($hi, $mi, $lo) = ($key =~ /^([^.]*)\\.(.*)\\.([^.]*)$/)) {\n+\t\t$lo =~ s/_//g;\n \t\t$key = join(\".\", lc($hi), $mi, lc($lo));\n+\t\treturn if ($lo =~ /\\W/ || $hi =~ /\\W/);\n \t} else {\n \t\t$key = lc($key);\n+\t\t$key =~ s/_//g;\n+\t\treturn if ($key =~ /\\W/);\n \t}\n \t$key =~ s/^gitweb\\.//;\n-\t$key =~ s/_//g;\n-\treturn if ($key =~ m/\\W/);\n \n \t# type sanity check\n \tif (defined $type) {\n"},{"id":"203618","messageId":"20121121193115.GB16280@sigill.intra.peff.net","threadId":"32026","inReplyTo":"7vpq37rk3v.fsf_-_@alter.siamese.dyndns.org","subject":"Re: Re* [PATCH] gitweb: make remote_heads config setting work.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-21T19:31:15Z","receivedAt":"2012-11-21T19:31:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 20, 2012 at 02:21:40PM -0800, Junio C Hamano wrote:\n\n> > Good catch. I think the \"return\" in the existing code suffers from the\n> > same problem: it will bail on non-word characters in the $mi part, but\n> > that part should allow arbitrary characters.\n> \n> I am tired of keeping the \"expecting reroll\" entries and having to\n> worry about them, so let's do this\n> \n> -- >8 --\n> Subject: [squash] gitweb: make remote_heads config setting work\n> \n> Only the top-level and bottom-level names are case insensitive and\n> spelled without \"_\".  Protect future support of subsection names\n> from name mangling.\n\nI think the end result is fine, though you are really fixing a bug here\n(the /\\W/ check is moved up), and then also putting the remote_heads\nfix (s/_//g) into the right place. IOW, if you are going to squash, you\nshould probably note the bug-fix part in the commit message explicitly.\n\n-Peff\n"}]}