Re: [PATCHv2 1/3] gitweb: Deal with HEAD pointing to unborn branch in "heads" view

Jakub Narebski [off-list ref] writes:

On Thu, 16 Feb 2012, Junio C Hamano wrote:

quoted

Jakub Narebski [off-list ref] writes:

quoted

Gitweb has problems if HEAD points to an unborn branch, with no
commits on it yet, but there are other branches present (so it is not
newly initialized repository).

It would be more readable if you rephrase the vague "has problems" with a
concrete description of what the problem is.

Sorry about this.

The problem is that gitweb would generate the following warning, writing
it in web server logs:

  Use of uninitialized value in string eq

When known and easily describable in a short paragraph, let's write both
the cause and the symptom together.

Should I re-roll this patch with improved commit message?

I was following Peff's 4 step review process, and I was in step #1 (read
the log message---can I understand what this is about without looking at
the patch?), so I haven't reached the diff part of your message ;-)

You added "defined $head_at &&" to protect against the undef comparison
for all uses of $head and I do not see any $head that was left unconverted
by mistake, so the patch looks OK to me (at times I wish gitweb were
written in a language more strict than Perl so that a compiler can catch
such mistakes).

But after trying to write a reroll myself, I have to wonder what would
happen if you have two branches pointing at the same commit as the one at
HEAD.  Why isn't the use of current_head class controlled by comparison
between the name of the ref and the output from "symbolic-ref HEAD"?

-- >8 --
From: Jakub Narebski <redacted>
Date: Wed, 15 Feb 2012 16:36:41 +0100
Subject: [PATCH] gitweb: Fix "heads" view when there is no current branch

In a repository whose HEAD points to an unborn branch with no commits,
"heads" view and "summary" view (which shows what is shown in "heads"
view) compared the object names of commits at the tip of branches with the
output from "git rev-parse HEAD", which caused comparison of a string with
undef and resulted in a warning in the server log.

This can happen if non-bare repository (with default 'master' branch)
is updated not via committing but by other means like push to it, or
Gerrit.  It can happen also just after running "git checkout --orphan
<new branch>" but before creating any new commit on this branch.

Rewrite the comparison so that it also works when $head points at nothing;
in such a case, no branch can be "the current branch", add a test for it.

While at it rename local variable $head to $head_at, as it points to
current commit rather than current branch name (HEAD contents).

Reported-by: Rajesh Boyapati
Signed-off-by: Jakub Narebski <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
 gitweb/gitweb.perl                     |    4 ++--
 t/t9500-gitweb-standalone-no-errors.sh |    9 +++++++++
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 3fc7380..af154c3 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl

@@ -5633,7 +5633,7 @@ sub git_tags_body {
 
 sub git_heads_body {
 	# uses global variable $project
-	my ($headlist, $head, $from, $to, $extra) = @_;
+	my ($headlist, $head_at, $from, $to, $extra) = @_;
 	$from = 0 unless defined $from;
 	$to = $#{$headlist} if (!defined $to || $#{$headlist} < $to);
 

@@ -5642,7 +5642,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 = defined $head_at && $ref{'id'} eq $head_at;
 		if ($alternate) {
 			print "<tr class=\"dark\">\n";
 		} else {

diff --git a/t/t9500-gitweb-standalone-no-errors.sh b/t/t9500-gitweb-standalone-no-errors.sh
index 0f771c6..81246a6 100755
--- a/t/t9500-gitweb-standalone-no-errors.sh
+++ b/t/t9500-gitweb-standalone-no-errors.sh

@@ -739,4 +739,13 @@ test_expect_success \
 	'echo "\$projects_list_group_categories = 1;" >>gitweb_config.perl &&
 	 gitweb_run'
 
+# ----------------------------------------------------------------------
+# unborn branches
+
+test_expect_success \
+	'unborn HEAD: "summary" page (with "heads" subview)' \
+	'git checkout orphan_branch || git checkout --orphan orphan_branch &&
+	 test_when_finished "git checkout master" &&
+	 gitweb_run "p=.git;a=summary"'
+
 test_done

-- 
1.7.9.1.236.gedc23

On Fri, 17 Feb 2012, Junio C Hamano wrote:
 

But after trying to write a reroll myself, I have to wonder what would
happen if you have two branches pointing at the same commit as the one at
HEAD.  Why isn't the use of current_head class controlled by comparison
between the name of the ref and the output from "symbolic-ref HEAD"?

If there is more than one branch that points to HEAD commit, they all
will be highlighted.

Using "git symbolic-ref HEAD", or just reading '.git/HEAD' file or symlink
is on my todo list.  This will make gitweb highlight current branch
correctly even if there is more than one branch that point to the same
HEAD commit, and make it possible to support "detached HEAD" (which I think
is not supported at all now).

Anyway the test is here to stay... :-)
 

-- >8 --
From: Jakub Narebski <redacted>
Date: Wed, 15 Feb 2012 16:36:41 +0100
Subject: [PATCH] gitweb: Fix "heads" view when there is no current branch

In a repository whose HEAD points to an unborn branch with no commits,
"heads" view and "summary" view (which shows what is shown in "heads"
view) compared the object names of commits at the tip of branches with the
output from "git rev-parse HEAD", which caused comparison of a string with
undef and resulted in a warning in the server log.

This can happen if non-bare repository (with default 'master' branch)
is updated not via committing but by other means like push to it, or
Gerrit.  It can happen also just after running "git checkout --orphan
<new branch>" but before creating any new commit on this branch.

Rewrite the comparison so that it also works when $head points at nothing;
in such a case, no branch can be "the current branch", add a test for it.

While at it rename local variable $head to $head_at, as it points to
current commit rather than current branch name (HEAD contents).

Reported-by: Rajesh Boyapati
Signed-off-by: Jakub Narebski <redacted>
Signed-off-by: Junio C Hamano <redacted>
---

Thanks!

-- 
Jakub Narebski
Poland