{"thread":{"id":"29154","subject":"[PATCH] Gitweb: Avoid warnings when a repo does not have a valid HEAD","startedAt":"2011-12-13T22:35:05Z","lastAt":"2011-12-14T09:00:26Z","messageCount":7,"participants":["Joe Ratterman","Johannes Sixt","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"181086","messageId":"1323815706-10560-1-git-send-email-jratt0@gmail.com","threadId":"29154","inReplyTo":null,"subject":"[PATCH] Gitweb: Avoid warnings when a repo does not have a valid HEAD","fromName":"Joe Ratterman","fromEmail":"jratt0@gmail.com","sentAt":"2011-12-13T22:35:05Z","receivedAt":"2011-12-13T22:35:05Z","isPatch":true,"sender":{"key":"jratt0@gmail.com","avatar":"https://avatars.githubusercontent.com/u/66082737?v=4"},"body":"It is possible that the HEAD reference does not point to an existing\nbranch.  When viewing such a repository in gitweb, a message like this\none was sent to the error log:\n\n  gitweb.cgi: Use of uninitialized value in string eq at /usr/src/git/gitweb/gitweb.cgi line 5115.\n\nSigned-off-by: Joe Ratterman <jratt0@gmail.com>\n---\n gitweb/gitweb.perl |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 4f0c3bd..5af06d6 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -5440,7 +5440,7 @@ sub git_heads_body {\n \tfor (my $i = $from; $i <= $to; $i++) {\n \t\tmy $entry = $headlist->[$i];\n \t\tmy %ref = %$entry;\n-\t\tmy $curr = $ref{'id'} eq $head;\n+\t\tmy $curr = $head ? ($ref{'id'} eq $head) : 0;\n \t\tif ($alternate) {\n \t\t\tprint \"<tr class=\\\"dark\\\">\\n\";\n \t\t} else {\n-- \n1.7.7.1\n"},{"id":"181087","messageId":"1323815706-10560-2-git-send-email-jratt0@gmail.com","threadId":"29154","inReplyTo":"1323815706-10560-1-git-send-email-jratt0@gmail.com","subject":"[PATCH] Add '-P' as a synonym for '--no-pager' in the git command","fromName":"Joe Ratterman","fromEmail":"jratt0@gmail.com","sentAt":"2011-12-13T22:35:06Z","receivedAt":"2011-12-13T22:35:06Z","isPatch":true,"sender":{"key":"jratt0@gmail.com","avatar":"https://avatars.githubusercontent.com/u/66082737?v=4"},"body":"  Also, add both -P|--no-pager to the existing -p|--paginate in bash\n  completion.\n\nSigned-off-by: Joe Ratterman <jratt0@gmail.com>\n---\n Documentation/git.txt                  |    3 ++-\n contrib/completion/git-completion.bash |    2 +-\n git.c                                  |    4 ++--\n t/t7006-pager.sh                       |    8 ++++++++\n 4 files changed, 13 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex e869032..c7f8445 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -10,7 +10,7 @@ SYNOPSIS\n --------\n [verse]\n 'git' [--version] [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]\n-    [-p|--paginate|--no-pager] [--no-replace-objects] [--bare]\n+    [-p|--paginate|-P|--no-pager] [--no-replace-objects] [--bare]\n     [--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]\n     [-c <name>=<value>]\n     [--help] <command> [<args>]\n@@ -329,6 +329,7 @@ help ...`.\n \tconfiguration options (see the \"Configuration Mechanism\" section\n \tbelow).\n \n+-P::\n --no-pager::\n \tDo not pipe git output into a pager.\n \ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex cc1bdf9..8b9ea1b 100755\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -2640,7 +2640,7 @@ _git ()\n \t\tcase \"$i\" in\n \t\t--git-dir=*) __git_dir=\"${i#--git-dir=}\" ;;\n \t\t--bare)      __git_dir=\".\" ;;\n-\t\t--version|-p|--paginate) ;;\n+\t\t--version|-p|--paginate|-P|--no-pager) ;;\n \t\t--help) command=\"help\"; break ;;\n \t\t*) command=\"$i\"; break ;;\n \t\tesac\ndiff --git a/git.c b/git.c\nindex 8e34903..baa1613 100644\n--- a/git.c\n+++ b/git.c\n@@ -7,7 +7,7 @@\n \n const char git_usage_string[] =\n \t\"git [--version] [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]\\n\"\n-\t\"           [-p|--paginate|--no-pager] [--no-replace-objects] [--bare]\\n\"\n+\t\"           [-p|--paginate|-P|--no-pager] [--no-replace-objects] [--bare]\\n\"\n \t\"           [--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]\\n\"\n \t\"           [-c name=value] [--help]\\n\"\n \t\"           <command> [<args>]\";\n@@ -103,7 +103,7 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \t\t\texit(0);\n \t\t} else if (!strcmp(cmd, \"-p\") || !strcmp(cmd, \"--paginate\")) {\n \t\t\tuse_pager = 1;\n-\t\t} else if (!strcmp(cmd, \"--no-pager\")) {\n+\t\t} else if (!strcmp(cmd, \"-P\") || !strcmp(cmd, \"--no-pager\")) {\n \t\t\tuse_pager = 0;\n \t\t\tif (envchanged)\n \t\t\t\t*envchanged = 1;\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex 320e1d1..783915e 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -84,6 +84,14 @@ test_expect_success 'no pager even with --paginate when stdout is a pipe' '\n \t! test -e paginated.out\n '\n \n+test_expect_success TTY 'no pager with -P' '\n+\trm -f paginated.out ||\n+\tcleanup_fail &&\n+\n+\ttest_terminal git -P log &&\n+\t! test -e paginated.out\n+'\n+\n test_expect_success TTY 'no pager with --no-pager' '\n \trm -f paginated.out ||\n \tcleanup_fail &&\n-- \n1.7.7.1\n"},{"id":"181116","messageId":"4EE853C0.20505@viscovery.net","threadId":"29154","inReplyTo":"1323815706-10560-2-git-send-email-jratt0@gmail.com","subject":"Re: [PATCH] Add '-P' as a synonym for '--no-pager' in the git command","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2011-12-14T07:44:00Z","receivedAt":"2011-12-14T07:44:00Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 12/13/2011 23:35, schrieb Joe Ratterman:\n> +test_expect_success TTY 'no pager with -P' '\n> +\trm -f paginated.out ||\n> +\tcleanup_fail &&\n> +\n> +\ttest_terminal git -P log &&\n> +\t! test -e paginated.out\n> +'\n> +\n>  test_expect_success TTY 'no pager with --no-pager' '\n>  \trm -f paginated.out ||\n>  \tcleanup_fail &&\n\nWhat kind of bogosity do I see in the context of this hunk (and in the new\ntext as well, but it is not entirely your fault, Joe, since you obviously\nhave only copied an existing test snippet)?\n\nWouldn't rm -f always succeed under normal circumstances, and then the\nrest of the test would be skipped? This use of || was introduced by\nfdf1bc48 (t7006: guard cleanup with test_expect_success).\n\n-- Hannes\n"},{"id":"181118","messageId":"20111214082203.GB13166@elie.hsd1.il.comcast.net","threadId":"29154","inReplyTo":"4EE853C0.20505@viscovery.net","subject":"[PATCH] test: errors preparing for a test are not special","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-12-14T08:22:03Z","receivedAt":"2011-12-14T08:22:03Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"This script uses the following idiom to start each test in a known\ngood state:\n\n\ttest_expect_success 'some commands use a pager' '\n\t\trm -f paginated.out || cleanup_fail &&\n\t\ttest_terminal git log &&\n\t\ttest -e paginated.out\n\t'\n\nwhere \"cleanup_fail\" is a function that prints an error message and\nerrors out.\n\nThat is bogus on three levels:\n\n - Cleanup commands like \"rm -f\" and \"test_unconfig\" are designed not\n   to fail, so this logic would never trip.\n\n - If they were to malfunction anyway, it is not useful to set apart\n   cleanup commands as a special kind of failure with a special error\n   message.  Whichever command fails, the next step is to investigate\n   which command that was, for example by running tests with\n   \"prove -e 'sh -x'\", and fix it.\n\n - Relying on left-associativity of mixed &&/|| lists makes the code\n   somewhat cryptic.\n\nThe fix is simple: drop the \"|| cleanup_fail\" in each test and the\ndefinition of the \"cleanup_fail\" function so no new callers can arise.\n\nReported-by: Johannes Sixt <j6t@kdbg.org>\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nJohannes Sixt wrote:\n> Am 12/13/2011 23:35, schrieb Joe Ratterman:\n\n>>  test_expect_success TTY 'no pager with --no-pager' '\n>>  \trm -f paginated.out ||\n>>  \tcleanup_fail &&\n>\n> What kind of bogosity do I see in the context of this hunk\n[...]\n> Wouldn't rm -f always succeed under normal circumstances, and then the\n> rest of the test would be skipped?\n\nNo, && and || are left-associative.\n\nThat said, I think the || was a mistake.  At the time, I had not yet\nunderstood why some tests in other scripts had cleanup commands\noutside the test body.  I wrongly thought it was to make sure errors\nduring cleanup are not mistaken for errors in the test proper (rather\nthan to ensure cleanup happens when needed even if some tests are\nskipped).\n\n t/t7006-pager.sh |   73 ++++++++++++-----------------------------------------\n 1 files changed, 17 insertions(+), 56 deletions(-)\n\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex 320e1d1d..ff259084 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -6,11 +6,6 @@ test_description='Test automatic use of a pager.'\n . \"$TEST_DIRECTORY\"/lib-pager.sh\n . \"$TEST_DIRECTORY\"/lib-terminal.sh\n \n-cleanup_fail() {\n-\techo >&2 cleanup failed\n-\t(exit 1)\n-}\n-\n test_expect_success 'setup' '\n \tsane_unset GIT_PAGER GIT_PAGER_IN_USE &&\n \ttest_unconfig core.pager &&\n@@ -22,9 +17,7 @@ test_expect_success 'setup' '\n '\n \n test_expect_success TTY 'some commands use a pager' '\n-\trm -f paginated.out ||\n-\tcleanup_fail &&\n-\n+\trm -f paginated.out &&\n \ttest_terminal git log &&\n \ttest -e paginated.out\n '\n@@ -45,49 +38,37 @@ test_expect_failure TTY 'pager runs from subdir' '\n '\n \n test_expect_success TTY 'some commands do not use a pager' '\n-\trm -f paginated.out ||\n-\tcleanup_fail &&\n-\n+\trm -f paginated.out &&\n \ttest_terminal git rev-list HEAD &&\n \t! test -e paginated.out\n '\n \n test_expect_success 'no pager when stdout is a pipe' '\n-\trm -f paginated.out ||\n-\tcleanup_fail &&\n-\n+\trm -f paginated.out &&\n \tgit log | cat &&\n \t! test -e paginated.out\n '\n \n test_expect_success 'no pager when stdout is a regular file' '\n-\trm -f paginated.out ||\n-\tcleanup_fail &&\n-\n+\trm -f paginated.out &&\n \tgit log >file &&\n \t! test -e paginated.out\n '\n \n test_expect_success TTY 'git --paginate rev-list uses a pager' '\n-\trm -f paginated.out ||\n-\tcleanup_fail &&\n-\n+\trm -f paginated.out &&\n \ttest_terminal git --paginate rev-list HEAD &&\n \ttest -e paginated.out\n '\n \n test_expect_success 'no pager even with --paginate when stdout is a pipe' '\n-\trm -f file paginated.out ||\n-\tcleanup_fail &&\n-\n+\trm -f file paginated.out &&\n \tgit --paginate log | cat &&\n \t! test -e paginated.out\n '\n \n test_expect_success TTY 'no pager with --no-pager' '\n-\trm -f paginated.out ||\n-\tcleanup_fail &&\n-\n+\trm -f paginated.out &&\n \ttest_terminal git --no-pager log &&\n \t! test -e paginated.out\n '\n@@ -136,9 +117,7 @@ colorful() {\n }\n \n test_expect_success 'tests can detect color' '\n-\trm -f colorful.log colorless.log ||\n-\tcleanup_fail &&\n-\n+\trm -f colorful.log colorless.log &&\n \tgit log --no-color >colorless.log &&\n \tgit log --color >colorful.log &&\n \t! colorful colorless.log &&\n@@ -147,18 +126,14 @@ test_expect_success 'tests can detect color' '\n \n test_expect_success 'no color when stdout is a regular file' '\n \trm -f colorless.log &&\n-\ttest_config color.ui auto ||\n-\tcleanup_fail &&\n-\n+\ttest_config color.ui auto &&\n \tgit log >colorless.log &&\n \t! colorful colorless.log\n '\n \n test_expect_success TTY 'color when writing to a pager' '\n \trm -f paginated.out &&\n-\ttest_config color.ui auto ||\n-\tcleanup_fail &&\n-\n+\ttest_config color.ui auto &&\n \t(\n \t\tTERM=vt100 &&\n \t\texport TERM &&\n@@ -181,9 +156,7 @@ test_expect_success TTY 'colors are suppressed by color.pager' '\n \n test_expect_success 'color when writing to a file intended for a pager' '\n \trm -f colorful.log &&\n-\ttest_config color.ui auto ||\n-\tcleanup_fail &&\n-\n+\ttest_config color.ui auto &&\n \t(\n \t\tTERM=vt100 &&\n \t\tGIT_PAGER_IN_USE=true &&\n@@ -242,9 +215,7 @@ test_default_pager() {\n \t$test_expectation SIMPLEPAGER,TTY \"$cmd - default pager is used by default\" \"\n \t\tsane_unset PAGER GIT_PAGER &&\n \t\ttest_unconfig core.pager &&\n-\t\trm -f default_pager_used ||\n-\t\tcleanup_fail &&\n-\n+\t\trm -f default_pager_used &&\n \t\tcat >\\$less <<-\\EOF &&\n \t\t#!/bin/sh\n \t\twc >default_pager_used\n@@ -265,9 +236,7 @@ test_PAGER_overrides() {\n \t$test_expectation TTY \"$cmd - PAGER overrides default pager\" \"\n \t\tsane_unset GIT_PAGER &&\n \t\ttest_unconfig core.pager &&\n-\t\trm -f PAGER_used ||\n-\t\tcleanup_fail &&\n-\n+\t\trm -f PAGER_used &&\n \t\tPAGER='wc >PAGER_used' &&\n \t\texport PAGER &&\n \t\t$full_command &&\n@@ -292,9 +261,7 @@ test_core_pager() {\n \n \t$test_expectation TTY \"$cmd - repository-local core.pager setting $used_if_wanted\" \"\n \t\tsane_unset GIT_PAGER &&\n-\t\trm -f core.pager_used ||\n-\t\tcleanup_fail &&\n-\n+\t\trm -f core.pager_used &&\n \t\tPAGER=wc &&\n \t\texport PAGER &&\n \t\ttest_config core.pager 'wc >core.pager_used' &&\n@@ -321,9 +288,7 @@ test_pager_subdir_helper() {\n \t$test_expectation TTY \"$cmd - core.pager $used_if_wanted from subdirectory\" \"\n \t\tsane_unset GIT_PAGER &&\n \t\trm -f core.pager_used &&\n-\t\trm -fr sub ||\n-\t\tcleanup_fail &&\n-\n+\t\trm -fr sub &&\n \t\tPAGER=wc &&\n \t\tstampname=\\$(pwd)/core.pager_used &&\n \t\texport PAGER stampname &&\n@@ -341,9 +306,7 @@ test_GIT_PAGER_overrides() {\n \tparse_args \"$@\"\n \n \t$test_expectation TTY \"$cmd - GIT_PAGER overrides core.pager\" \"\n-\t\trm -f GIT_PAGER_used ||\n-\t\tcleanup_fail &&\n-\n+\t\trm -f GIT_PAGER_used &&\n \t\ttest_config core.pager wc &&\n \t\tGIT_PAGER='wc >GIT_PAGER_used' &&\n \t\texport GIT_PAGER &&\n@@ -356,9 +319,7 @@ test_doesnt_paginate() {\n \tparse_args \"$@\"\n \n \t$test_expectation TTY \"no pager for '$cmd'\" \"\n-\t\trm -f GIT_PAGER_used ||\n-\t\tcleanup_fail &&\n-\n+\t\trm -f GIT_PAGER_used &&\n \t\tGIT_PAGER='wc >GIT_PAGER_used' &&\n \t\texport GIT_PAGER &&\n \t\t$full_command &&\n-- \n1.7.8\n"},{"id":"181120","messageId":"4EE863AF.2070606@viscovery.net","threadId":"29154","inReplyTo":"20111214082203.GB13166@elie.hsd1.il.comcast.net","subject":"Re: [PATCH] test: errors preparing for a test are not special","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2011-12-14T08:51:59Z","receivedAt":"2011-12-14T08:51:59Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 12/14/2011 9:22, schrieb Jonathan Nieder:\n> Johannes Sixt wrote:\n>> Am 12/13/2011 23:35, schrieb Joe Ratterman:\n> \n>>>  test_expect_success TTY 'no pager with --no-pager' '\n>>>  \trm -f paginated.out ||\n>>>  \tcleanup_fail &&\n>>\n>> What kind of bogosity do I see in the context of this hunk\n> [...]\n>> Wouldn't rm -f always succeed under normal circumstances, and then the\n>> rest of the test would be skipped?\n> \n> No, && and || are left-associative.\n\n<Facepalm/> You are right, of course, and I should have known better. (I\nneed more caffein.)\n\nAt any rate, your patch makes the code much more comprehensible.\n\n-- Hannes\n"},{"id":"181121","messageId":"20111214085337.GA19424@elie.hsd1.il.comcast.net","threadId":"29154","inReplyTo":"1323815706-10560-1-git-send-email-jratt0@gmail.com","subject":"Re: [PATCH] Gitweb: Avoid warnings when a repo does not have a valid HEAD","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-12-14T08:53:37Z","receivedAt":"2011-12-14T08:53:37Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"(just cc-ing Jakub)\nJoe Ratterman wrote:\n\n> It is possible that the HEAD reference does not point to an existing\n> branch.  When viewing such a repository in gitweb, a message like this\n> one was sent to the error log:\n>\n>   gitweb.cgi: Use of uninitialized value in string eq at /usr/src/git/gitweb/gitweb.cgi line 5115.\n>\n> Signed-off-by: Joe Ratterman <jratt0@gmail.com>\n> ---\n>  gitweb/gitweb.perl |    2 +-\n>  1 files changed, 1 insertions(+), 1 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 4f0c3bd..5af06d6 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -5440,7 +5440,7 @@ sub git_heads_body {\n>  \tfor (my $i = $from; $i <= $to; $i++) {\n>  \t\tmy $entry = $headlist->[$i];\n>  \t\tmy %ref = %$entry;\n> -\t\tmy $curr = $ref{'id'} eq $head;\n> +\t\tmy $curr = $head ? ($ref{'id'} eq $head) : 0;\n>  \t\tif ($alternate) {\n>  \t\t\tprint \"<tr class=\\\"dark\\\">\\n\";\n>  \t\t} else {\n> -- \n> 1.7.7.1\n> \n> \n"},{"id":"181122","messageId":"20111214090026.GB19424@elie.hsd1.il.comcast.net","threadId":"29154","inReplyTo":"4EE863AF.2070606@viscovery.net","subject":"Re: [PATCH] test: errors preparing for a test are not special","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-12-14T09:00:26Z","receivedAt":"2011-12-14T09:00:26Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Johannes Sixt wrote:\n\n> At any rate, your patch makes the code much more comprehensible.\n\nThanks for the reminder and sanity-check.\n"}]}