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

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

[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

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

[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

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

[Johannes Sixt]

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

[Jonathan Nieder]

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

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

[Johannes Sixt]

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

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

[Jonathan Nieder]

Johannes Sixt wrote:

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

Thanks for the reminder and sanity-check.

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

[Jonathan Nieder]

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

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

[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

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

[Junio C Hamano]

Joe Ratterman <jratt0@gmail.com> writes:

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

Makes one wonder if $head could be '0', but I presume this is about the
case where (!defined $head) holds true. Also makes one wonder if a similar
issue exists on the $ref{"id"} side. I suspect that won't be true unless
you have a very screwed-up repository, but in that case a repository with
a HEAD that points at an unborn branch _and_ have other refs that do point
at existing commit is already screwed-up, so if we want to be extremely
pedantic then perhaps ...

    my $curr = ((defined $head && exists $ref{"id"} && defined $ref{"id"})
		? ($ref{"id"} eq $head)
                : 0);

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Joe Ratterman <jratt0@gmail.com> writes:
>
>> 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.
>>
> ..., but in that case a repository with
> a HEAD that points at an unborn branch _and_ have other refs that do point
> at existing commit is already screwed-up, so if we want to be extremely
> pedantic then perhaps ...
>
>     my $curr = ((defined $head && exists $ref{"id"} && defined $ref{"id"})
> 		? ($ref{"id"} eq $head)
>                 : 0);

Just in case, I was not suggesting to update the patch to look like the
above by "if we want to be extremely pedantic".

After all, that error message in the log may be a good thing that notifies
the site administrators about a suspicious repository so that it can be
fixed (even though it was not a designed "feature" but something that
happens to work).

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

[Jakub Narebski]

On Thu, 15 Dec 2011, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>> Joe Ratterman <jratt0@gmail.com> writes:
>>
>>> 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.
>>>
>> ..., but in that case a repository with
>> a HEAD that points at an unborn branch _and_ have other refs that do point
>> at existing commit is already screwed-up, so if we want to be extremely
>> pedantic then perhaps ...
>>
>>     my $curr = ((defined $head && exists $ref{"id"} && defined $ref{"id"})

       my $curr = ((defined $head && defined $ref{"id"})

is enough, because '!exists $hash{key}' implies '!defined $hash{key}'
(though reverse is not true).

>> 		? ($ref{"id"} eq $head)
>>                 : 0);
> 
> Just in case, I was not suggesting to update the patch to look like the
> above by "if we want to be extremely pedantic".
> 
> After all, that error message in the log may be a good thing that notifies
> the site administrators about a suspicious repository so that it can be
> fixed (even though it was not a designed "feature" but something that
> happens to work).

Well, but for that those error messages should describe error; the above
is quite hard to translate to 

  warning: HEAD points to an unborn branch in repository foo.git

BTW. we should probably warn about this situation more clear, perhaps
by showing warning instead of empty place where subject of current commit
is usually shown.


Anyway git marks up branches (sic!) that _point to_ the same commit as
HEAD because we calculate it anyway, at least for 'summary' view.

Better solution would be to examine HEAD (either using Perl, or using
"git symbolic-ref") to get _name_ of current branch.  I'd rather avoid
yet another command call.

-- 
Jakub Narebski
Poland