gitweb: disambiguate heads and tags withs the same name
PATCH 1/1 gitweb: disambiguate heads and tags withs the same name
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.
---
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(-)
gitweb/gitweb.perl +8 -6
@@ -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>"; }
Signature · 1 line
1.5.3.6
PATCH 1/1 gitweb: Try harder in parse_tag; perhaps it was given ambiguous name
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.
---
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(-)
gitweb/gitweb.perl +12 -0
@@ -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;
Signature · 1 line
1.5.3.6
Re: [PATCH] gitweb: Try harder in parse_tag; perhaps it was given ambiguous name
This patch should be RFC.
Sorry for missing that.
Signature · 2 lines
Jakub Narebski Poland
Re: [PATCH] gitweb: Try harder in parse_tag; perhaps it was given ambiguous name
In reply to Jakub NarebskiI 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
Quoted 4 lines · 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
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.
Quoted 7 lines
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)?
Signature · 2 lines
Jakub Narebski Poland
Re: [PATCH] gitweb: Try harder in parse_tag; perhaps it was given ambiguous name
Quoted 4 lines · Jakub Narebski
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.
Quoted 5 lines
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.
Quoted 4 lines
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
Quoted 7 lines · Junio C Hamano
Quoted 4 lines · Jakub Narebski
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)?
Quoted 7 lines
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?
Signature · 2 lines
Jakub Narebski Poland
Re: [PATCH] gitweb: Try harder in parse_tag; perhaps it was given ambiguous name
Quoted 20 lines · Jakub Narebski
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.
Quoted 1 line
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.