* [PATCH] Gitweb: Avoid warnings when a repo does not have a valid HEAD @ 2011-12-13 22:35 Joe Ratterman 2011-12-13 22:35 ` [PATCH] Add '-P' as a synonym for '--no-pager' in the git command Joe Ratterman 2011-12-14 8:53 ` [PATCH] Gitweb: Avoid warnings when a repo does not have a valid HEAD Jonathan Nieder 0 siblings, 2 replies; 7+ messages in thread From: Joe Ratterman @ 2011-12-13 22:35 UTC (permalink / raw) To: git; +Cc: Joe Ratterman It is possible that the HEAD reference does not point to an existing branch. When viewing such a repository in gitweb, a message like this one was sent to the error log: gitweb.cgi: Use of uninitialized value in string eq at /usr/src/git/gitweb/gitweb.cgi line 5115. Signed-off-by: Joe Ratterman <jratt0@gmail.com> --- gitweb/gitweb.perl | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index 4f0c3bd..5af06d6 100755 --- a/gitweb/gitweb.perl +++ b/gitweb/gitweb.perl @@ -5440,7 +5440,7 @@ sub git_heads_body { for (my $i = $from; $i <= $to; $i++) { my $entry = $headlist->[$i]; my %ref = %$entry; - my $curr = $ref{'id'} eq $head; + my $curr = $head ? ($ref{'id'} eq $head) : 0; if ($alternate) { print "<tr class=\"dark\">\n"; } else { -- 1.7.7.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH] Add '-P' as a synonym for '--no-pager' in the git command 2011-12-13 22:35 [PATCH] Gitweb: Avoid warnings when a repo does not have a valid HEAD Joe Ratterman @ 2011-12-13 22:35 ` Joe Ratterman 2011-12-14 7:44 ` Johannes Sixt 2011-12-14 8:53 ` [PATCH] Gitweb: Avoid warnings when a repo does not have a valid HEAD Jonathan Nieder 1 sibling, 1 reply; 7+ messages in thread From: Joe Ratterman @ 2011-12-13 22:35 UTC (permalink / raw) To: git; +Cc: Joe Ratterman Also, add both -P|--no-pager to the existing -p|--paginate in bash completion. Signed-off-by: Joe Ratterman <jratt0@gmail.com> --- Documentation/git.txt | 3 ++- contrib/completion/git-completion.bash | 2 +- git.c | 4 ++-- t/t7006-pager.sh | 8 ++++++++ 4 files changed, 13 insertions(+), 4 deletions(-) diff --git a/Documentation/git.txt b/Documentation/git.txt index e869032..c7f8445 100644 --- a/Documentation/git.txt +++ b/Documentation/git.txt @@ -10,7 +10,7 @@ SYNOPSIS -------- [verse] 'git' [--version] [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path] - [-p|--paginate|--no-pager] [--no-replace-objects] [--bare] + [-p|--paginate|-P|--no-pager] [--no-replace-objects] [--bare] [--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>] [-c <name>=<value>] [--help] <command> [<args>] @@ -329,6 +329,7 @@ help ...`. configuration options (see the "Configuration Mechanism" section below). +-P:: --no-pager:: Do not pipe git output into a pager. diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index cc1bdf9..8b9ea1b 100755 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -2640,7 +2640,7 @@ _git () case "$i" in --git-dir=*) __git_dir="${i#--git-dir=}" ;; --bare) __git_dir="." ;; - --version|-p|--paginate) ;; + --version|-p|--paginate|-P|--no-pager) ;; --help) command="help"; break ;; *) command="$i"; break ;; esac diff --git a/git.c b/git.c index 8e34903..baa1613 100644 --- a/git.c +++ b/git.c @@ -7,7 +7,7 @@ const char git_usage_string[] = "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] [--help]\n" " <command> [<args>]"; @@ -103,7 +103,7 @@ static int handle_options(const char ***argv, int *argc, int *envchanged) exit(0); } else if (!strcmp(cmd, "-p") || !strcmp(cmd, "--paginate")) { use_pager = 1; - } else if (!strcmp(cmd, "--no-pager")) { + } else if (!strcmp(cmd, "-P") || !strcmp(cmd, "--no-pager")) { use_pager = 0; if (envchanged) *envchanged = 1; diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh index 320e1d1..783915e 100755 --- a/t/t7006-pager.sh +++ b/t/t7006-pager.sh @@ -84,6 +84,14 @@ test_expect_success 'no pager even with --paginate when stdout is a pipe' ' ! test -e paginated.out ' +test_expect_success TTY 'no pager with -P' ' + rm -f paginated.out || + cleanup_fail && + + test_terminal git -P log && + ! test -e paginated.out +' + test_expect_success TTY 'no pager with --no-pager' ' rm -f paginated.out || cleanup_fail && -- 1.7.7.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] Add '-P' as a synonym for '--no-pager' in the git command 2011-12-13 22:35 ` [PATCH] Add '-P' as a synonym for '--no-pager' in the git command Joe Ratterman @ 2011-12-14 7:44 ` Johannes Sixt 2011-12-14 8:22 ` [PATCH] test: errors preparing for a test are not special Jonathan Nieder 0 siblings, 1 reply; 7+ messages in thread From: Johannes Sixt @ 2011-12-14 7:44 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Joe Ratterman, git Am 12/13/2011 23:35, schrieb Joe Ratterman: > +test_expect_success TTY 'no pager with -P' ' > + rm -f paginated.out || > + cleanup_fail && > + > + test_terminal git -P log && > + ! test -e paginated.out > +' > + > test_expect_success TTY 'no pager with --no-pager' ' > rm -f paginated.out || > cleanup_fail && What kind of bogosity do I see in the context of this hunk (and in the new text as well, but it is not entirely your fault, Joe, since you obviously have only copied an existing test snippet)? Wouldn't rm -f always succeed under normal circumstances, and then the rest of the test would be skipped? This use of || was introduced by fdf1bc48 (t7006: guard cleanup with test_expect_success). -- Hannes ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] test: errors preparing for a test are not special 2011-12-14 7:44 ` Johannes Sixt @ 2011-12-14 8:22 ` Jonathan Nieder 2011-12-14 8:51 ` Johannes Sixt 0 siblings, 1 reply; 7+ messages in thread From: Jonathan Nieder @ 2011-12-14 8:22 UTC (permalink / raw) To: Johannes Sixt; +Cc: Joe Ratterman, git, Junio C Hamano, Jeff King This script uses the following idiom to start each test in a known good state: test_expect_success 'some commands use a pager' ' rm -f paginated.out || cleanup_fail && test_terminal git log && test -e paginated.out ' where "cleanup_fail" is a function that prints an error message and errors out. That is bogus on three levels: - Cleanup commands like "rm -f" and "test_unconfig" are designed not to fail, so this logic would never trip. - If they were to malfunction anyway, it is not useful to set apart cleanup commands as a special kind of failure with a special error message. Whichever command fails, the next step is to investigate which command that was, for example by running tests with "prove -e 'sh -x'", and fix it. - Relying on left-associativity of mixed &&/|| lists makes the code somewhat cryptic. The fix is simple: drop the "|| cleanup_fail" in each test and the definition of the "cleanup_fail" function so no new callers can arise. Reported-by: Johannes Sixt <j6t@kdbg.org> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> --- Johannes Sixt wrote: > Am 12/13/2011 23:35, schrieb Joe Ratterman: >> test_expect_success TTY 'no pager with --no-pager' ' >> rm -f paginated.out || >> cleanup_fail && > > What kind of bogosity do I see in the context of this hunk [...] > Wouldn't rm -f always succeed under normal circumstances, and then the > rest of the test would be skipped? No, && and || are left-associative. That said, I think the || was a mistake. At the time, I had not yet understood why some tests in other scripts had cleanup commands outside the test body. I wrongly thought it was to make sure errors during cleanup are not mistaken for errors in the test proper (rather than to ensure cleanup happens when needed even if some tests are skipped). t/t7006-pager.sh | 73 ++++++++++++----------------------------------------- 1 files changed, 17 insertions(+), 56 deletions(-) diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh index 320e1d1d..ff259084 100755 --- a/t/t7006-pager.sh +++ b/t/t7006-pager.sh @@ -6,11 +6,6 @@ test_description='Test automatic use of a pager.' . "$TEST_DIRECTORY"/lib-pager.sh . "$TEST_DIRECTORY"/lib-terminal.sh -cleanup_fail() { - echo >&2 cleanup failed - (exit 1) -} - test_expect_success 'setup' ' sane_unset GIT_PAGER GIT_PAGER_IN_USE && test_unconfig core.pager && @@ -22,9 +17,7 @@ test_expect_success 'setup' ' ' test_expect_success TTY 'some commands use a pager' ' - rm -f paginated.out || - cleanup_fail && - + rm -f paginated.out && test_terminal git log && test -e paginated.out ' @@ -45,49 +38,37 @@ test_expect_failure TTY 'pager runs from subdir' ' ' test_expect_success TTY 'some commands do not use a pager' ' - rm -f paginated.out || - cleanup_fail && - + rm -f paginated.out && test_terminal git rev-list HEAD && ! test -e paginated.out ' test_expect_success 'no pager when stdout is a pipe' ' - rm -f paginated.out || - cleanup_fail && - + rm -f paginated.out && git log | cat && ! test -e paginated.out ' test_expect_success 'no pager when stdout is a regular file' ' - rm -f paginated.out || - cleanup_fail && - + rm -f paginated.out && git log >file && ! test -e paginated.out ' test_expect_success TTY 'git --paginate rev-list uses a pager' ' - rm -f paginated.out || - cleanup_fail && - + rm -f paginated.out && test_terminal git --paginate rev-list HEAD && test -e paginated.out ' test_expect_success 'no pager even with --paginate when stdout is a pipe' ' - rm -f file paginated.out || - cleanup_fail && - + rm -f file paginated.out && git --paginate log | cat && ! test -e paginated.out ' test_expect_success TTY 'no pager with --no-pager' ' - rm -f paginated.out || - cleanup_fail && - + rm -f paginated.out && test_terminal git --no-pager log && ! test -e paginated.out ' @@ -136,9 +117,7 @@ colorful() { } test_expect_success 'tests can detect color' ' - rm -f colorful.log colorless.log || - cleanup_fail && - + rm -f colorful.log colorless.log && git log --no-color >colorless.log && git log --color >colorful.log && ! colorful colorless.log && @@ -147,18 +126,14 @@ test_expect_success 'tests can detect color' ' test_expect_success 'no color when stdout is a regular file' ' rm -f colorless.log && - test_config color.ui auto || - cleanup_fail && - + test_config color.ui auto && git log >colorless.log && ! colorful colorless.log ' test_expect_success TTY 'color when writing to a pager' ' rm -f paginated.out && - test_config color.ui auto || - cleanup_fail && - + test_config color.ui auto && ( TERM=vt100 && export TERM && @@ -181,9 +156,7 @@ test_expect_success TTY 'colors are suppressed by color.pager' ' test_expect_success 'color when writing to a file intended for a pager' ' rm -f colorful.log && - test_config color.ui auto || - cleanup_fail && - + test_config color.ui auto && ( TERM=vt100 && GIT_PAGER_IN_USE=true && @@ -242,9 +215,7 @@ test_default_pager() { $test_expectation SIMPLEPAGER,TTY "$cmd - default pager is used by default" " sane_unset PAGER GIT_PAGER && test_unconfig core.pager && - rm -f default_pager_used || - cleanup_fail && - + rm -f default_pager_used && cat >\$less <<-\EOF && #!/bin/sh wc >default_pager_used @@ -265,9 +236,7 @@ test_PAGER_overrides() { $test_expectation TTY "$cmd - PAGER overrides default pager" " sane_unset GIT_PAGER && test_unconfig core.pager && - rm -f PAGER_used || - cleanup_fail && - + rm -f PAGER_used && PAGER='wc >PAGER_used' && export PAGER && $full_command && @@ -292,9 +261,7 @@ test_core_pager() { $test_expectation TTY "$cmd - repository-local core.pager setting $used_if_wanted" " sane_unset GIT_PAGER && - rm -f core.pager_used || - cleanup_fail && - + rm -f core.pager_used && PAGER=wc && export PAGER && test_config core.pager 'wc >core.pager_used' && @@ -321,9 +288,7 @@ test_pager_subdir_helper() { $test_expectation TTY "$cmd - core.pager $used_if_wanted from subdirectory" " sane_unset GIT_PAGER && rm -f core.pager_used && - rm -fr sub || - cleanup_fail && - + rm -fr sub && PAGER=wc && stampname=\$(pwd)/core.pager_used && export PAGER stampname && @@ -341,9 +306,7 @@ test_GIT_PAGER_overrides() { parse_args "$@" $test_expectation TTY "$cmd - GIT_PAGER overrides core.pager" " - rm -f GIT_PAGER_used || - cleanup_fail && - + rm -f GIT_PAGER_used && test_config core.pager wc && GIT_PAGER='wc >GIT_PAGER_used' && export GIT_PAGER && @@ -356,9 +319,7 @@ test_doesnt_paginate() { parse_args "$@" $test_expectation TTY "no pager for '$cmd'" " - rm -f GIT_PAGER_used || - cleanup_fail && - + rm -f GIT_PAGER_used && GIT_PAGER='wc >GIT_PAGER_used' && export GIT_PAGER && $full_command && -- 1.7.8 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] test: errors preparing for a test are not special 2011-12-14 8:22 ` [PATCH] test: errors preparing for a test are not special Jonathan Nieder @ 2011-12-14 8:51 ` Johannes Sixt 2011-12-14 9:00 ` Jonathan Nieder 0 siblings, 1 reply; 7+ messages in thread From: Johannes Sixt @ 2011-12-14 8:51 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Joe Ratterman, git, Junio C Hamano, Jeff King Am 12/14/2011 9:22, schrieb Jonathan Nieder: > Johannes Sixt wrote: >> Am 12/13/2011 23:35, schrieb Joe Ratterman: > >>> test_expect_success TTY 'no pager with --no-pager' ' >>> rm -f paginated.out || >>> cleanup_fail && >> >> What kind of bogosity do I see in the context of this hunk > [...] >> Wouldn't rm -f always succeed under normal circumstances, and then the >> rest of the test would be skipped? > > No, && and || are left-associative. <Facepalm/> You are right, of course, and I should have known better. (I need more caffein.) At any rate, your patch makes the code much more comprehensible. -- Hannes ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] test: errors preparing for a test are not special 2011-12-14 8:51 ` Johannes Sixt @ 2011-12-14 9:00 ` Jonathan Nieder 0 siblings, 0 replies; 7+ messages in thread From: Jonathan Nieder @ 2011-12-14 9:00 UTC (permalink / raw) To: Johannes Sixt; +Cc: Joe Ratterman, git, Junio C Hamano, Jeff King Johannes Sixt wrote: > At any rate, your patch makes the code much more comprehensible. Thanks for the reminder and sanity-check. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Gitweb: Avoid warnings when a repo does not have a valid HEAD 2011-12-13 22:35 [PATCH] Gitweb: Avoid warnings when a repo does not have a valid HEAD Joe Ratterman 2011-12-13 22:35 ` [PATCH] Add '-P' as a synonym for '--no-pager' in the git command Joe Ratterman @ 2011-12-14 8:53 ` Jonathan Nieder 1 sibling, 0 replies; 7+ messages in thread From: Jonathan Nieder @ 2011-12-14 8:53 UTC (permalink / raw) To: Joe Ratterman; +Cc: git, Jakub Narebski (just cc-ing Jakub) Joe Ratterman wrote: > It is possible that the HEAD reference does not point to an existing > branch. When viewing such a repository in gitweb, a message like this > one was sent to the error log: > > gitweb.cgi: Use of uninitialized value in string eq at /usr/src/git/gitweb/gitweb.cgi line 5115. > > Signed-off-by: Joe Ratterman <jratt0@gmail.com> > --- > gitweb/gitweb.perl | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl > index 4f0c3bd..5af06d6 100755 > --- a/gitweb/gitweb.perl > +++ b/gitweb/gitweb.perl > @@ -5440,7 +5440,7 @@ sub git_heads_body { > for (my $i = $from; $i <= $to; $i++) { > my $entry = $headlist->[$i]; > my %ref = %$entry; > - my $curr = $ref{'id'} eq $head; > + my $curr = $head ? ($ref{'id'} eq $head) : 0; > if ($alternate) { > print "<tr class=\"dark\">\n"; > } else { > -- > 1.7.7.1 > > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2011-12-14 9:00 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-12-13 22:35 [PATCH] Gitweb: Avoid warnings when a repo does not have a valid HEAD Joe Ratterman 2011-12-13 22:35 ` [PATCH] Add '-P' as a synonym for '--no-pager' in the git command Joe Ratterman 2011-12-14 7:44 ` Johannes Sixt 2011-12-14 8:22 ` [PATCH] test: errors preparing for a test are not special Jonathan Nieder 2011-12-14 8:51 ` Johannes Sixt 2011-12-14 9:00 ` Jonathan Nieder 2011-12-14 8:53 ` [PATCH] Gitweb: Avoid warnings when a repo does not have a valid HEAD Jonathan Nieder
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.