[PATCH] gitweb: disambiguate heads and tags withs the same name

[PATCH] gitweb: disambiguate heads and tags withs the same name

[Jakub Narebski]

Avoid wrong disambiguation that would link logs/trees of tags and heads which
share the same name to the same page, leading to a disambiguation that would
prefer the tag, thus making it impossible to access the corresponding
head log and tree without hacking the url by hand.

It does it by using full refname (with 'refs/heads/' or 'refs/tags/' prefix)
instead of shortened one in the URLs in 'heads' and 'tags' tables.

Signed-off-by: Guillaume Seguin <guillaume@segu.in>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This does exactly the same as patch send by Guillaume Seguin earlier
  Message-ID: <1194126032.15420.4.camel@ed3n-m>
  http://permalink.gmane.org/gmane.comp.version-control.git/63317

Original patch added 'refs/heads/' and 'refs/tags/' in git_heads_body
and git_tags_body respectively; this one uses 'fullname' field, which
contain refname before stripping 'refs/heads/' or 'refs/tags/'. The
change is in git_get_heads_list and git_get_tags_list, respectively.

Original patch was either badly whitespace damaged, or GMane has
broken 'raw' display.

Note that this patch does not help handcrafted URLs, and saved URLs from
older version of gitweb.

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 491a3f4..6ff4221 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2234,6 +2234,7 @@ sub git_get_heads_list {
 		my ($hash, $name, $title) = split(' ', $refinfo, 3);
 		my ($committer, $epoch, $tz) =
 			($committerinfo =~ /^(.*) ([0-9]+) (.*)$/);
+		$ref_item{'fullname'}  = $name;
 		$name =~ s!^refs/heads/!!;
 
 		$ref_item{'name'}  = $name;
@@ -2271,6 +2272,7 @@ sub git_get_tags_list {
 		my ($id, $type, $name, $refid, $reftype, $title) = split(' ', $refinfo, 6);
 		my ($creator, $epoch, $tz) =
 			($creatorinfo =~ /^(.*) ([0-9]+) (.*)$/);
+		$ref_item{'fullname'} = $name;
 		$name =~ s!^refs/tags/!!;
 
 		$ref_item{'type'} = $type;
@@ -3691,8 +3693,8 @@ sub git_tags_body {
 		      "<td class=\"link\">" . " | " .
 		      $cgi->a({-href => href(action=>$tag{'reftype'}, hash=>$tag{'refid'})}, $tag{'reftype'});
 		if ($tag{'reftype'} eq "commit") {
-			print " | " . $cgi->a({-href => href(action=>"shortlog", hash=>$tag{'name'})}, "shortlog") .
-			      " | " . $cgi->a({-href => href(action=>"log", hash=>$tag{'name'})}, "log");
+			print " | " . $cgi->a({-href => href(action=>"shortlog", hash=>$tag{'fullname'})}, "shortlog") .
+			      " | " . $cgi->a({-href => href(action=>"log", hash=>$tag{'fullname'})}, "log");
 		} elsif ($tag{'reftype'} eq "blob") {
 			print " | " . $cgi->a({-href => href(action=>"blob_plain", hash=>$tag{'refid'})}, "raw");
 		}
@@ -3727,13 +3729,13 @@ sub git_heads_body {
 		$alternate ^= 1;
 		print "<td><i>$ref{'age'}</i></td>\n" .
 		      ($curr ? "<td class=\"current_head\">" : "<td>") .
-		      $cgi->a({-href => href(action=>"shortlog", hash=>$ref{'name'}),
+		      $cgi->a({-href => href(action=>"shortlog", hash=>$ref{'fullname'}),
 		               -class => "list name"},esc_html($ref{'name'})) .
 		      "</td>\n" .
 		      "<td class=\"link\">" .
-		      $cgi->a({-href => href(action=>"shortlog", hash=>$ref{'name'})}, "shortlog") . " | " .
-		      $cgi->a({-href => href(action=>"log", hash=>$ref{'name'})}, "log") . " | " .
-		      $cgi->a({-href => href(action=>"tree", hash=>$ref{'name'}, hash_base=>$ref{'name'})}, "tree") .
+		      $cgi->a({-href => href(action=>"shortlog", hash=>$ref{'fullname'})}, "shortlog") . " | " .
+		      $cgi->a({-href => href(action=>"log", hash=>$ref{'fullname'})}, "log") . " | " .
+		      $cgi->a({-href => href(action=>"tree", hash=>$ref{'fullname'}, hash_base=>$ref{'name'})}, "tree") .
 		      "</td>\n" .
 		      "</tr>";
 	}
-- 
1.5.3.6

[PATCH] gitweb: Try harder in parse_tag; perhaps it was given ambiguous name

[Jakub Narebski]

If parse_tag was given ambiguous name, i.e. name which is both head
(branch) name and tag name, parse_tag failed because git prefer heads
to tags if there is ambiguity.  Now it tries harder: if git-cat-file
doesn't produce output, try to resolve argument as tag name using
git-show-ref.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This supplements previous patch; while previous modified links to always
use unambiguous name, this one makes 'tag' view work even if passed
ambiguous name which is both name of head and of tag.

 gitweb/gitweb.perl |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 6ff4221..0427290 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1876,6 +1876,18 @@ sub parse_tag {
 	my @comment;
 
 	open my $fd, "-|", git_cmd(), "cat-file", "tag", $tag_id or return;
+	# try harder in case there is head (branch) with the same name as tag
+	if (eof($fd)) {
+		close $fd or return;
+		my $git_command = git_cmd_str();
+		$tag_id = qx($git_command show-ref --hash --tags $tag_id);
+		return unless $tag_id;
+		open $fd, "-|", git_cmd(), "cat-file", "tag", $tag_id or return;
+		if (eof($fd)) {
+			close $fd;
+			return;
+		};
+	}
 	$tag{'id'} = $tag_id;
 	while (my $line = <$fd>) {
 		chomp $line;
-- 
1.5.3.6

Re: [PATCH] gitweb: Try harder in parse_tag; perhaps it was given ambiguous name

[Jakub Narebski]

This patch should be RFC.
Sorry for missing that. 

-- 
Jakub Narebski
Poland

Re: [PATCH] gitweb: Try harder in parse_tag; perhaps it was given ambiguous name

[Junio C Hamano]

I have these two patches still in my mailbox, unapplied:

[PATCH] gitweb: disambiguate heads and tags withs the same name
[PATCH] gitweb: Try harder in parse_tag; perhaps it was given ambiguous name

I am wondering if they should be part of 1.5.4.  They look Ok but it is
not very easy to pick up what the real breakage it is trying to fix from
Perl gibberish.

Can we have tests (not just "we do not spit out anything to stderr") for
gitweb so that each patch can demonstrate the existing breakage, to make
judging easier?

Re: [PATCH] gitweb: Try harder in parse_tag; perhaps it was given ambiguous name

[Jakub Narebski]

On Wed, 5 Dec 2007, Junio C Hamano wrote:

> I have these two patches still in my mailbox, unapplied:
> 
> [PATCH] gitweb: disambiguate heads and tags withs the same name
> [PATCH] gitweb: Try harder in parse_tag; perhaps it was given ambiguous name

Actually second should be [PATCH/RFC] as it penalizes the "not found"
case (extra check 'if really not found').

First patch, which is modified version of Guillaume Seguin patch solves
problem that links in gitweb does lead to correct 'tag' view, while the
second one solves the problem from the other side: instead of ensuring
that links in gitweb are unambiguous it tries to resolve ambiguity.


The problem is caused by the fact that git _always_ prefer heads (head
refs) to tags (tag refs), even when it is clear
  $ git cat-file tags ambiguous-ref
that we want a tag. So alternate solution would be to correct
git-cat-file.
 

> I am wondering if they should be part of 1.5.4.  They look Ok but it is
> not very easy to pick up what the real breakage it is trying to fix from
> Perl gibberish.
> 
> Can we have tests (not just "we do not spit out anything to stderr") for
> gitweb so that each patch can demonstrate the existing breakage, to make
> judging easier?

True, current way of testing gitweb does not allow for test which would
detect breakage noticed by Guillaume.

It would be quite easy I think to add checking if gitweb returns
expected HTTP return code (HTTP status). So what is the portable way
to check if first line of some output matches given regexp (given fixed
string)?

-- 
Jakub Narebski
Poland

Re: [PATCH] gitweb: Try harder in parse_tag; perhaps it was given ambiguous name

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> First patch, which is modified version of Guillaume Seguin patch solves
> problem that links in gitweb does lead to correct 'tag' view, while the
> second one solves the problem from the other side: instead of ensuring
> that links in gitweb are unambiguous it tries to resolve ambiguity.

Ok, I'll queue the first one (disambiguate) for 1.5.4 while letting the
people decide the latter for now.

> The problem is caused by the fact that git _always_ prefer heads (head
> refs) to tags (tag refs), even when it is clear
>   $ git cat-file tags ambiguous-ref
> that we want a tag. So alternate solution would be to correct
> git-cat-file.

You are getting the layering all wrong.

 * git-cat-file takes "object name" on its command line (so do many
   other commands).

 * One of the way to spell an "object name" is to refer to it with a ref
   that can reach it (e.g. to name 12th generation parent of the tip of
   the master branch, you spell "master~12" and you are using the ref
   refs/heads/master).

 * You do not have to always write out the ref in full.  There is a
   defined order to disambiguate refs (see git-rev-parse(1)), that
   allows you to say 'master' and it expands to either refs/tags/master,
   refs/heads/master or whatever.

Now git-cat-file does not care how you spelled your object name, and has
no business influencing the ref disambiguation order.  You _could_ argue
"git cat-file tag <foo>" _expects_ <foo> to name a tag, but that logic
is very flawed (and that is why I said your understanding of layering is
screwed) for two reasons:

 (1) <foo> may be user input to the script that uses cat-file and the
     script may be expecting a tag there.  Perhaps the script is about
     creating a new branch from a tag (expecting a tag) and adds some
     administrative info in the configuration file for the branch.  It
     does first:

	t=$(git cat-file tag "$1") || die not a tag

     and later the script may want to do:

	git branch $newone "$1"
        git config branch.$newone.description "created from tag $1 ($t)"

     If you make "cat-file tag" to favor tag, and in a similar fashion
     if you make "branch" favor branch, the above will not do what you
     expect.

     Consistently resolving the refname without (or minimum number of)
     exceptions would give less surprising result.

 (2) "git cat-file -t <foo>" is to find out what type the object is and
     is meant to be used by callers who do not know the type.  There is
     no "favoring this class of ref over other classses" possible there.

> It would be quite easy I think to add checking if gitweb returns
> expected HTTP return code (HTTP status). So what is the portable way
> to check if first line of some output matches given regexp (given fixed
> string)?

Huh?  Wouldn't something like this be enough?

	>expect.empty &&
	cmd >actual.out 2>actual.err &&
        diff -u expect.empty actual.err &&
        first=$(sed -e '1q' <actual.out) &&
	test "z$first" = "I like it"

Re: [PATCH] gitweb: Try harder in parse_tag; perhaps it was given ambiguous name

[Jakub Narebski]

Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > It would be quite easy I think to add checking if gitweb returns
> > expected HTTP return code (HTTP status). So what is the portable way
> > to check if first line of some output matches given regexp (given fixed
> > string)?
> 
> Huh?  Wouldn't something like this be enough?
> 
> 	>expect.empty &&
> 	cmd >actual.out 2>actual.err &&
>         diff -u expect.empty actual.err &&
>         first=$(sed -e '1q' <actual.out) &&
> 	test "z$first" = "I like it"

Well, actually that is even better idea. We can go for one of the three
levels of HTTP status checking:

1. Check if we got "Status: 200 OK" when we expect it, and not have it
   when we expect other HTTP status, e.g. when requesting nonexistent
   file. The above code is enough for that.

2. We can check if we got expected status number, for example 200 for
   when we expect no error, or 404 when object is not found, or 403
   if there is no such object etc. I was thinking about using this version
   the need to check not full first line, but fragment of it.

3. We can check full first line, for example
     Status: 200 OK
     Status: 403 Forbidden
     Status: 404 Not Found
     Status: 400 Bad Request
   but this might tie gitweb test too tightly with minute details of
   gitweb output. The above code is good for that too.

What do you think, which route we should go in test?
-- 
Jakub Narebski
Poland

Re: [PATCH] gitweb: Try harder in parse_tag; perhaps it was given ambiguous name

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> Well, actually that is even better idea. We can go for one of the three
> levels of HTTP status checking:
>
> 1. Check if we got "Status: 200 OK" when we expect it, and not have it
>    when we expect other HTTP status, e.g. when requesting nonexistent
>    file. The above code is enough for that.
>
> 2. We can check if we got expected status number, for example 200 for
>    when we expect no error, or 404 when object is not found, or 403
>    if there is no such object etc. I was thinking about using this version
>    the need to check not full first line, but fragment of it.
>
> 3. We can check full first line, for example
>      Status: 200 OK
>      Status: 403 Forbidden
>      Status: 404 Not Found
>      Status: 400 Bad Request
>    but this might tie gitweb test too tightly with minute details of
>    gitweb output. The above code is good for that too.
>
> What do you think, which route we should go in test?

4. We should check for what we expect in the parts of gitweb output we
   care about.  E.g.

   * If we are making sure it refuses to serve incorrect request
     (e.g. no such repository), we should check for the status, which is
     what we care about (we may not care about how the actual error
     message is stated).

   * Otherwise (and I suspect this is "most of the tests"), we obviously
     expect to have "200 OK", and check for that;

   BUT IN ADDITION

   * If we want to make sure that a specific aspect of the output was
     buggy and a patch fixed it (e.g. an href used a short refname
     without having refs/heads or refs/tags prefixed, causing
     ambiguity), we also should check that part of output in generated
     HTML, not just the HTTP status header.