[PATCH] Gitweb: Avoid warnings when a repo does not have a valid HEAD

Subsystems: the rest

DORMANTno replies

7 messages, 3 authors, 2016-06-15 · open the first message on its own page

[PATCH] Gitweb: Avoid warnings when a repo does not have a valid HEAD

From: Joe Ratterman <hidden>
Date: 2016-06-15 22:52:38

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 <redacted>
---
 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

[PATCH] Add '-P' as a synonym for '--no-pager' in the git command

From: Joe Ratterman <hidden>
Date: 2016-06-15 22:52:38

  Also, add both -P|--no-pager to the existing -p|--paginate in bash
  completion.

Signed-off-by: Joe Ratterman <redacted>
---
 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

Re: [PATCH] Add '-P' as a synonym for '--no-pager' in the git command

From: Johannes Sixt <hidden>
Date: 2016-06-15 22:52:38

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

[PATCH] test: errors preparing for a test are not special

From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:52:38

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 <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
Johannes Sixt wrote:

Am 12/13/2011 23:35, schrieb Joe Ratterman:

quoted

 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

Re: [PATCH] test: errors preparing for a test are not special

From: Johannes Sixt <hidden>
Date: 2016-06-15 22:52:38

Am 12/14/2011 9:22, schrieb Jonathan Nieder:

Johannes Sixt wrote:

quoted

Am 12/13/2011 23:35, schrieb Joe Ratterman:

quoted

 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

[...]

quoted

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

Re: [PATCH] Gitweb: Avoid warnings when a repo does not have a valid HEAD

From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:52:38

(just cc-ing Jakub)
Joe Ratterman wrote:

quoted hunk

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 <redacted>
---
 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

Re: [PATCH] test: errors preparing for a test are not special

From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:52:38

Johannes Sixt wrote:

At any rate, your patch makes the code much more comprehensible.

Thanks for the reminder and sanity-check.

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help