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

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

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

Jakub Narebski <jnareb@gmail.com> writes:

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.

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.

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

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

O.K.

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

O.K.

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

Jakub Narebski <jnareb@gmail.com> writes:

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

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

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

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

Thanks!

Jakub Narebski <jnareb@gmail.com> writes:

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

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

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