git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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] 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

* 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

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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).