{"thread":{"id":"22204","subject":"Re: [PATCH 1/7] gitweb: Load checking","startedAt":"2010-01-13T17:18:37Z","lastAt":"2010-01-13T17:33:55Z","messageCount":2,"participants":["Jakub Narebski"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"131493","messageId":"201001131818.38703.jnareb@gmail.com","threadId":"22204","inReplyTo":"1263374828-15342-2-git-send-email-warthog9@eaglescrag.net","subject":"Re: [PATCH 1/7] gitweb: Load checking","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-01-13T17:18:37Z","receivedAt":"2010-01-13T17:18:37Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Wed, 13 Jan 2010, John 'Warthog9' Hawley wrote:\n> \n> v3 - warthog9: small additional adjustment to indicate where new load\n>      checking should be added for future improvements\n> v2 - Jakub: switching to using\n\n\"switching to using\" what?\n\n> v1 - warthog9: Initial creation\n> \n> Signed-off-by: John 'Warthog9' Hawley <warthog9@kernel.org>\n> Signed-off-by: Jakub Narebski <jnareb@gmail.com>\n> ---\n\nAlso, such comments should be put in the comment section, here i.e. between\n\"---\\n\" separator and the diffstat, and not in the commit message.\n\n>  gitweb/README      |    7 ++++++-\n>  gitweb/gitweb.perl |   45 +++++++++++++++++++++++++++++++++++++++++----\n>  2 files changed, 47 insertions(+), 5 deletions(-)\n\nOther that this minor nick (which I guess could be fixed by Junio when\napplying), this one looks good.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"131497","messageId":"201001131833.55603.jnareb@gmail.com","threadId":"22204","inReplyTo":"1263374828-15342-3-git-send-email-warthog9@eaglescrag.net","subject":"Re: [PATCH 2/7] gitweb: Add option to force version match","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-01-13T17:33:55Z","receivedAt":"2010-01-13T17:33:55Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Wed, 13 Jan 2010, John 'Warthog9' Hawley wrote:\n> From: John 'Warthog9' Hawley <warthog9@kernel.org>\n> \n> This adds $git_versions_must_match variable, which is set to true\n\ns/is set to true value/if set to true,/\n\n> value checks that we are running on the same version of git that we\n> shipped with, and if not throw '500 Internal Server Error' error.\n> What is checked is the version of gitweb (embedded in building\n> gitweb.cgi), against version of runtime git binary used.\n> \n> Gitweb can usually run with a mismatched git install.  This is more\n> here to give an obvious warning as to whats going on vs. silently\n> failing.\n> \n> By default this feature is turned off.\n\n[moved version info below, after \"---\" separator]\n\n> Signed-off-by: John 'Warthog9' Hawley <warthog9@kernel.org>\n> Signed-off-by: Jakub Narebski <jnareb@gmail.com>\n> ---\n\nMoved to the correct place: information about patch versioning should\nbe put in comment section[1], not in commit message.\n \n[1] Which means between \"---\" separator and diffstat.\n\n> v3 - warthog9: adjust to use die_error instead of recreating it\n> v2 - Jakub: Changes to make non-default, and change naming\n> v1 - warthog9: Initial\n\n>  gitweb/README      |    3 +++\n>  gitweb/gitweb.perl |   28 ++++++++++++++++++++++++++++\n>  2 files changed, 31 insertions(+), 0 deletions(-)\n\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n\n> @@ -587,6 +590,31 @@ if (defined $maxload && get_loadavg() > $maxload) {\n>  our $git_version = qx(\"$GIT\" --version) =~ m/git version (.*)$/ ? $1 : \"unknown\";\n>  $number_of_git_cmds++;\n>  \n> +# Throw an error if git versions does not match, if $git_versions_must_match is true.\n> +if ($git_versions_must_match &&\n> +    $git_version ne $version) {\n> +\tmy $admin_contact =\n> +\t\tdefined $ENV{'SERVER_ADMIN'} ? \", $ENV{'SERVER_ADMIN'},\" : '';\n> +\tmy $err_msg = <<EOT;\n> +Internal Server Error\n> +<br />\n> +</div>\n> +<hr />\n> +<div class=\"readme\">\n> +<h1 align=\"center\">*** Warning ***</h1>\n> +<p>\n> +This version of gitweb was compiled for <b>@{[esc_html($version)]}</b>,\n> +however git version <b>@{[esc_html($git_version)]}</b> was found on server,\n> +and administrator requested strict version checking.\n> +</p>\n> +<p>\n> +Please contact the server administrator${admin_contact} to either configure\n> +gitweb to allow mismatched versions, or update git or gitweb installation.\n> +</p>\n> +EOT\n> +\tdie_error(500, $err_msg);\n> +}\n\nI'm sorry to be this picky, but don't you think that caller should not need\nto know \"intimate\" details on how die_error works.  In particular please\nnotice that you must have known that die_error wraps its output in <div>...\nwhich is not even mentioned in comments.\n\nThe second argument of die_error() subroutine is meant to be alternative\ndescription of error condition: short and one line.  For situations such\nlike this one, where we want to describe error in more details, it would\nbe better, I think, to change signature of die_error() to take (optionally)\nthree arguments, and use 'die_error(500, undef, $err_msg);', with $err_msg\nstarting from '<h1>...</h1>'.\n\n@@ -3460,6 +3460,7 @@\n sub die_error {\n \tmy $status = shift || 500;\n \tmy $error = shift || \"Internal server error\";\n+\tmy $extra = shift;\n \n \tmy %http_responses = (\n \t\t400 => '400 Bad Request',\n@@ -3475,8 +3476,13 @@ sub die_error {\n <br /><br />\n $status - $error\n <br />\n-</div>\n EOF\n+\tif (defined $extra) {\n+\t\tprint \"<hr />\\n\" .\n+\t\t      \"$extra\\n\";\n+\t}\n+\tprint \"</div>\\n\";\n+\n \tgit_footer_html();\n \texit;\n }\n\n\nOther that this minor issue it looks good.\n\n-- \nJakub Narebski\nPoland\n"}]}