[PATCHv2 0/3] gitweb: Dealing with being on unborn branch

[PATCHv2 0/3] gitweb: Dealing with being on unborn branch

[Jakub Narebski]

This is resend (as series) of patches that were sent first time
individually in "Fwd: Git-web error" thread sent to git mailing list
by rajesh boyapati:

  http://thread.gmane.org/gmane.comp.version-control.git/189228

Unfortunately the thread is heavily fragmented in GMane interface,
because Rajesh responses were not sent to git mailing list.

The problem that original poster seen was caused by the fact that:

rb> For my git projects on gerrit, our main branch name is "base".
rb> We don't have any code on "master" branch. [...]
rb> [...] the HEAD file is pointing to "ref: refs/heads/master".
rb> So, I think that's the reason for errors.

Some of errors might appear in the way Rajesh sees them because gitweb
in this case is deployed from Gerrit (Git-based review board, in Java);
at least I couldn't reproduce some of errors in exactly the same way
as presented.

Note that there is no problem if repository is totally empty, without
any valid branch.  The problem is when HEAD doesn't point to the valid
commit object, but there are other commits and branches.

Anyway beside not using 'master' branch, but not renaming it /
repointing HEAD, such situation might happen when you have just
created an orphan branch, but haven't made any commits on it yet.


Table of contents:
~~~~~~~~~~~~~~~~~~
* [PATCHv2 1/3] gitweb: Deal with HEAD pointing to unborn branch
  in "heads" view

This fixes a real issue, and is a true fix (for current way of
generating "heads" view).  Note that there is "heads" excerpt in
"summary" view.

* [PATCHv2/RFC 2/3] gitweb: Harden parse_commit and parse_commits

Just add an early exit in the case of invalid parameters.  Though if
caller is passing undefined value for commit identifier, it would
probably not deal correctly with parse_commit() returning undef,
though it should deal correctly with parse_commits() returning empty
list.

More of band-aid than a real fix.

* [RFC/PATCHv2 3/3] gitweb: Silence stderr in parse_commit*()
  subroutines

This adds an overhead of forking a shell, and possibility of code
injection if our quote_command() is incorrect... and actually does not
fix issue for the original poster, and is not a problem for ordinary
(non-Gerrit) gitweb.


Shortlog:
~~~~~~~~~
Jakub Narebski (3):
  gitweb: Deal with HEAD pointing to unborn branch in "heads" view
  gitweb: Harden parse_commit and parse_commits
  gitweb: Silence stderr in parse_commit*() subroutines


Diffstat:
~~~~~~~~~
 gitweb/gitweb.perl                     |   21 ++++++++++++++-------
 t/t9500-gitweb-standalone-no-errors.sh |    9 +++++++++
 2 files changed, 23 insertions(+), 7 deletions(-)

-- 
1.7.9

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

[Jakub Narebski]

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).

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.

This commit adds test and fixes the issue of being on unborn branch
(of HEAD not pointing to a commit) in "heads" view, and also in
"summary" view -- which includes "heads" excerpt as subview.

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

Includes simple test for 'summary' view and being on unborn branch.

Reported-by: rajesh boyapati <boyapatisrajesh@gmail.com>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This patch first appeared on git mailing list in "Fwd: Git-web error"
as

  [PATCH] gitweb: Deal with HEAD pointing to unborn branch in "heads" view
  Message-Id: <201202032233.05324.jnareb@gmail.com>
  http://article.gmane.org/gmane.comp.version-control.git/189805

The $head -> $head_at change is not really necessary, but while we are
changing that area of code I felt it would be good to make it better.


What we should do in the future is rework git_heads_body to work with
symbolic references rather than mark branches that point to the same
commit as HEAD does.

That would also help in the "detached HEAD" case...

 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 87a95e2..0fdca5b 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

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

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

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

Also, drop the " (so it is ...)"  part, which does not add much useful
information.  Your next paragraph describes how a repository can come to
this state anyway.

> 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.
>
> This commit adds test and fixes the issue of being on unborn branch
> (of HEAD not pointing to a commit) in "heads" view, and also in
> "summary" view -- which includes "heads" excerpt as subview.

The reader has not seen anything more than "has problems" at this point,
so "fixes the issue of ..." is not very helpful.  You could have just said
"adds tests and fixes it", if you said that the unspecified "problems"
apear in "heads" and "summary" view at the beginning of the log message.

Thanks.

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

[Jakub Narebski]

On Thu, 16 Feb 2012, Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > 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

> Also, drop the " (so it is ...)"  part, which does not add much useful
> information.  Your next paragraph describes how a repository can come to
> this state anyway.

O.K.

Anyway the description that repository might be in such a strange state
might be more important that the patch in itself...

> > 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.
> >
> > This commit adds test and fixes the issue of being on unborn branch
> > (of HEAD not pointing to a commit) in "heads" view, and also in
> > "summary" view -- which includes "heads" excerpt as subview.
> 
> The reader has not seen anything more than "has problems" at this point,
> so "fixes the issue of ..." is not very helpful.  You could have just said
> "adds tests and fixes it", if you said that the unspecified "problems"
> apear in "heads" and "summary" view at the beginning of the log message.

O.K.


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

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

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> On Thu, 16 Feb 2012, Junio C Hamano wrote:
>> Jakub Narebski <jnareb@gmail.com> writes:
>> 
>> > 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 <jnareb@gmail.com>
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 <jnareb@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 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

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

[Jakub Narebski]

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 <jnareb@gmail.com>
> 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 <jnareb@gmail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---

Thanks!

-- 
Jakub Narebski
Poland

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

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> 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).

You should be more honest and admit that showing unrelated branches that
happen to point at the same commit as the current HEAD does (this includes
the case where HEAD is detached) as if they are *ALL* current branch is
*NEITER* working *CORRECTLY* nor *SUPPORT*ing "detached HEAD" at all.  It
may not be giving a runtime error, but instead it is showing AN INCORRECT
RESULT.

I'd grant you that this is not a new problem this patch introduces, and it
may not even be a bug you introduced long time ago.  The patch gives the
same INCORRECT RESULT as it intended to do before the patch, and removes
one runtime error, so it is not worsening the situation, but that does not
change the fact that the code after the patch is still *WRONG*.

[PATCHv2/RFC 2/3] gitweb: Harden parse_commit and parse_commits

[Jakub Narebski]

Gitweb has problems and gives errors when repository it shows is on
unborn branch (HEAD doesn't point to a valid commit), but there exist
other branches.

One of errors that shows in gitweb logs is undefined $commit_id in
parse_commits() subroutine.  Therefore we harden both parse_commit()
and parse_commits() against undefined $commit_id, and against no
output from git-rev-list because HEAD doesn't point to a commit.

Reported-by: rajesh boyapati <boyapatisrajesh@gmail.com>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This patch first appeared on git mailing list in "Fwd: Git-web error"
thread as

  [PATCH] gitweb: Harden parse_commit and parse_commits
  Message-Id: <201202081604.17187.jnareb@gmail.com>
  http://article.gmane.org/gmane.comp.version-control.git/190237

More prevention of generating warnings, rather than real fix.

 gitweb/gitweb.perl |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 0fdca5b..2eaf585 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3334,6 +3334,8 @@ sub parse_commit {
 	my ($commit_id) = @_;
 	my %co;
 
+	return unless defined $commit_id;
+
 	local $/ = "\0";
 
 	open my $fd, "-|", git_cmd(), "rev-list",
@@ -3343,7 +3345,9 @@ sub parse_commit {
 		$commit_id,
 		"--",
 		or die_error(500, "Open git-rev-list failed");
-	%co = parse_commit_text(<$fd>, 1);
+	my $commit_text = <$fd>;
+	%co = parse_commit_text($commit_text, 1)
+		if defined $commit_text;
 	close $fd;
 
 	return %co;
@@ -3353,6 +3357,7 @@ sub parse_commits {
 	my ($commit_id, $maxcount, $skip, $filename, @args) = @_;
 	my @cos;
 
+	return unless defined $commit_id;
 	$maxcount ||= 1;
 	$skip ||= 0;
 
-- 
1.7.9

[RFC/PATCHv2 3/3] gitweb: Silence stderr in parse_commit*() subroutines

[Jakub Narebski]

git-rev-list command in parse_commit() and parse_commits() can fail
because $commit_id doesn't point to a valid commit; for example if we
are on unborn branch HEAD doesn't point to a valid commit.

In this case git-rev-list prints

  fatal: bad revision 'HEAD'

on its standard error.  This commit silences this warning, at the cost
of using shell to redirect it to /dev/null, and relying on
quote_command() to protect against code injection.

Note however that such error message from git (from external command)
usually (but not always) does not appear in web server logs.

Reported-by: rajesh boyapati <boyapatisrajesh@gmail.com>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This patch first appeared on git mailing list in "Fwd: Git-web error"
thread as

  [PATCH] gitweb: Silence stderr in parse_commit*() subroutines
  Message-Id: <201202111402.31684.jnareb@gmail.com>
  http://article.gmane.org/gmane.comp.version-control.git/190511

More proof of concept rather than real fix for real issue;
it doesn't even fix the issue (for some reason) for original
poster.

 gitweb/gitweb.perl |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 2eaf585..224ace4 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3338,12 +3338,13 @@ sub parse_commit {
 
 	local $/ = "\0";
 
-	open my $fd, "-|", git_cmd(), "rev-list",
+	open my $fd, "-|", quote_command(
+		git_cmd(), "rev-list",
 		"--parents",
 		"--header",
 		"--max-count=1",
 		$commit_id,
-		"--",
+		"--") . ' 2>/dev/null',
 		or die_error(500, "Open git-rev-list failed");
 	my $commit_text = <$fd>;
 	%co = parse_commit_text($commit_text, 1)
@@ -3363,7 +3364,8 @@ sub parse_commits {
 
 	local $/ = "\0";
 
-	open my $fd, "-|", git_cmd(), "rev-list",
+	open my $fd, "-|", quote_command(
+		git_cmd(), "rev-list",
 		"--header",
 		@args,
 		("--max-count=" . $maxcount),
@@ -3371,7 +3373,7 @@ sub parse_commits {
 		@extra_options,
 		$commit_id,
 		"--",
-		($filename ? ($filename) : ())
+		($filename ? ($filename) : ())) . ' 2>/dev/null'
 		or die_error(500, "Open git-rev-list failed");
 	while (my $line = <$fd>) {
 		my %co = parse_commit_text($line);
-- 
1.7.9