{"thread":{"id":"11073","subject":"[PATCH] gitweb: Try harder in parse_tag; perhaps it was given ambiguous name","startedAt":"2007-12-01T01:45:27Z","lastAt":"2007-12-05T21:14:23Z","messageCount":8,"participants":["Jakub Narebski","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"61534","messageId":"200712010245.29204.jnareb@gmail.com","threadId":"11073","inReplyTo":null,"subject":"[PATCH] gitweb: disambiguate heads and tags withs the same name","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2007-12-01T01:45:27Z","receivedAt":"2007-12-01T01:45:27Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Avoid wrong disambiguation that would link logs/trees of tags and heads which\nshare the same name to the same page, leading to a disambiguation that would\nprefer the tag, thus making it impossible to access the corresponding\nhead log and tree without hacking the url by hand.\n\nIt does it by using full refname (with 'refs/heads/' or 'refs/tags/' prefix)\ninstead of shortened one in the URLs in 'heads' and 'tags' tables.\n\nSigned-off-by: Guillaume Seguin \nSigned-off-by: Jakub Narebski \n---\nThis does exactly the same as patch send by Guillaume Seguin earlier\n Message-ID: <1194126032.15420.4.camel@ed3n-m>\n http://permalink.gmane.org/gmane.comp.version-control.git/63317\n\nOriginal patch added 'refs/heads/' and 'refs/tags/' in git_heads_body\nand git_tags_body respectively; this one uses 'fullname' field, which\ncontain refname before stripping 'refs/heads/' or 'refs/tags/'. The\nchange is in git_get_heads_list and git_get_tags_list, respectively.\n\nOriginal patch was either badly whitespace damaged, or GMane has\nbroken 'raw' display.\n\nNote that this patch does not help handcrafted URLs, and saved URLs from\nolder version of gitweb.\n\n gitweb/gitweb.perl | 14 ++++++++------\n 1 files changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 491a3f4..6ff4221 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -2234,6 +2234,7 @@ sub git_get_heads_list {\n \t\tmy ($hash, $name, $title) = split(' ', $refinfo, 3);\n \t\tmy ($committer, $epoch, $tz) =\n \t\t\t($committerinfo =~ /^(.*) ([0-9]+) (.*)$/);\n+\t\t$ref_item{'fullname'} = $name;\n \t\t$name =~ s!^refs/heads/!!;\n \n \t\t$ref_item{'name'} = $name;\n@@ -2271,6 +2272,7 @@ sub git_get_tags_list {\n \t\tmy ($id, $type, $name, $refid, $reftype, $title) = split(' ', $refinfo, 6);\n \t\tmy ($creator, $epoch, $tz) =\n \t\t\t($creatorinfo =~ /^(.*) ([0-9]+) (.*)$/);\n+\t\t$ref_item{'fullname'} = $name;\n \t\t$name =~ s!^refs/tags/!!;\n \n \t\t$ref_item{'type'} = $type;\n@@ -3691,8 +3693,8 @@ sub git_tags_body {\n \t\t \"\" . \" | \" .\n \t\t $cgi->a({-href => href(action=>$tag{'reftype'}, hash=>$tag{'refid'})}, $tag{'reftype'});\n \t\tif ($tag{'reftype'} eq \"commit\") {\n-\t\t\tprint \" | \" . $cgi->a({-href => href(action=>\"shortlog\", hash=>$tag{'name'})}, \"shortlog\") .\n-\t\t\t \" | \" . $cgi->a({-href => href(action=>\"log\", hash=>$tag{'name'})}, \"log\");\n+\t\t\tprint \" | \" . $cgi->a({-href => href(action=>\"shortlog\", hash=>$tag{'fullname'})}, \"shortlog\") .\n+\t\t\t \" | \" . $cgi->a({-href => href(action=>\"log\", hash=>$tag{'fullname'})}, \"log\");\n \t\t} elsif ($tag{'reftype'} eq \"blob\") {\n \t\t\tprint \" | \" . $cgi->a({-href => href(action=>\"blob_plain\", hash=>$tag{'refid'})}, \"raw\");\n \t\t}\n@@ -3727,13 +3729,13 @@ sub git_heads_body {\n \t\t$alternate ^= 1;\n \t\tprint \"$ref{'age'}\\n\" .\n \t\t ($curr ? \"\" : \"\") .\n-\t\t $cgi->a({-href => href(action=>\"shortlog\", hash=>$ref{'name'}),\n+\t\t $cgi->a({-href => href(action=>\"shortlog\", hash=>$ref{'fullname'}),\n \t\t -class => \"list name\"},esc_html($ref{'name'})) .\n \t\t \"\\n\" .\n \t\t \"\" .\n-\t\t $cgi->a({-href => href(action=>\"shortlog\", hash=>$ref{'name'})}, \"shortlog\") . \" | \" .\n-\t\t $cgi->a({-href => href(action=>\"log\", hash=>$ref{'name'})}, \"log\") . \" | \" .\n-\t\t $cgi->a({-href => href(action=>\"tree\", hash=>$ref{'name'}, hash_base=>$ref{'name'})}, \"tree\") .\n+\t\t $cgi->a({-href => href(action=>\"shortlog\", hash=>$ref{'fullname'})}, \"shortlog\") . \" | \" .\n+\t\t $cgi->a({-href => href(action=>\"log\", hash=>$ref{'fullname'})}, \"log\") . \" | \" .\n+\t\t $cgi->a({-href => href(action=>\"tree\", hash=>$ref{'fullname'}, hash_base=>$ref{'name'})}, \"tree\") .\n \t\t \"\\n\" .\n \t\t \"\";\n \t}\n-- \n1.5.3.6\n"},{"id":"61533","messageId":"200712010247.25107.jnareb@gmail.com","threadId":"11073","inReplyTo":"200712010245.29204.jnareb@gmail.com","subject":"[PATCH] gitweb: Try harder in parse_tag; perhaps it was given ambiguous name","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2007-12-01T01:47:24Z","receivedAt":"2007-12-01T01:47:24Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"If parse_tag was given ambiguous name, i.e. name which is both head\n(branch) name and tag name, parse_tag failed because git prefer heads\nto tags if there is ambiguity. Now it tries harder: if git-cat-file\ndoesn't produce output, try to resolve argument as tag name using\ngit-show-ref.\n\nSigned-off-by: Jakub Narebski \n---\nThis supplements previous patch; while previous modified links to always\nuse unambiguous name, this one makes 'tag' view work even if passed\nambiguous name which is both name of head and of tag.\n\n gitweb/gitweb.perl | 12 ++++++++++++\n 1 files changed, 12 insertions(+), 0 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 6ff4221..0427290 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -1876,6 +1876,18 @@ sub parse_tag {\n \tmy @comment;\n \n \topen my $fd, \"-|\", git_cmd(), \"cat-file\", \"tag\", $tag_id or return;\n+\t# try harder in case there is head (branch) with the same name as tag\n+\tif (eof($fd)) {\n+\t\tclose $fd or return;\n+\t\tmy $git_command = git_cmd_str();\n+\t\t$tag_id = qx($git_command show-ref --hash --tags $tag_id);\n+\t\treturn unless $tag_id;\n+\t\topen $fd, \"-|\", git_cmd(), \"cat-file\", \"tag\", $tag_id or return;\n+\t\tif (eof($fd)) {\n+\t\t\tclose $fd;\n+\t\t\treturn;\n+\t\t};\n+\t}\n \t$tag{'id'} = $tag_id;\n \twhile (my $line = <$fd>) {\n \t\tchomp $line;\n-- \n1.5.3.6\n"},{"id":"61555","messageId":"200712010406.44203.jnareb@gmail.com","threadId":"11073","inReplyTo":"200712010247.25107.jnareb@gmail.com","subject":"Re: [PATCH] gitweb: Try harder in parse_tag; perhaps it was given ambiguous name","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2007-12-01T03:06:42Z","receivedAt":"2007-12-01T03:06:42Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"This patch should be RFC.\nSorry for missing that. \n\n-- \nJakub Narebski\nPoland\n"},{"id":"61975","messageId":"7v3auh1v75.fsf@gitster.siamese.dyndns.org","threadId":"11073","inReplyTo":"200712010247.25107.jnareb@gmail.com","subject":"Re: [PATCH] gitweb: Try harder in parse_tag; perhaps it was given ambiguous name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-05T07:01:34Z","receivedAt":"2007-12-05T07:01:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"I have these two patches still in my mailbox, unapplied:\n\n[PATCH] gitweb: disambiguate heads and tags withs the same name\n[PATCH] gitweb: Try harder in parse_tag; perhaps it was given ambiguous name\n\nI am wondering if they should be part of 1.5.4. They look Ok but it is\nnot very easy to pick up what the real breakage it is trying to fix from\nPerl gibberish.\n\nCan we have tests (not just \"we do not spit out anything to stderr\") for\ngitweb so that each patch can demonstrate the existing breakage, to make\njudging easier?\n"},{"id":"61992","messageId":"200712051113.40654.jnareb@gmail.com","threadId":"11073","inReplyTo":"7v3auh1v75.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] gitweb: Try harder in parse_tag; perhaps it was given ambiguous name","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2007-12-05T10:13:39Z","receivedAt":"2007-12-05T10:13:39Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Wed, 5 Dec 2007, Junio C Hamano wrote:\n\n> I have these two patches still in my mailbox, unapplied:\n> \n> [PATCH] gitweb: disambiguate heads and tags withs the same name\n> [PATCH] gitweb: Try harder in parse_tag; perhaps it was given ambiguous name\n\nActually second should be [PATCH/RFC] as it penalizes the \"not found\"\ncase (extra check 'if really not found').\n\nFirst patch, which is modified version of Guillaume Seguin patch solves\nproblem that links in gitweb does lead to correct 'tag' view, while the\nsecond one solves the problem from the other side: instead of ensuring\nthat links in gitweb are unambiguous it tries to resolve ambiguity.\n\n\nThe problem is caused by the fact that git _always_ prefer heads (head\nrefs) to tags (tag refs), even when it is clear\n $ git cat-file tags ambiguous-ref\nthat we want a tag. So alternate solution would be to correct\ngit-cat-file.\n \n\n> I am wondering if they should be part of 1.5.4. They look Ok but it is\n> not very easy to pick up what the real breakage it is trying to fix from\n> Perl gibberish.\n> \n> Can we have tests (not just \"we do not spit out anything to stderr\") for\n> gitweb so that each patch can demonstrate the existing breakage, to make\n> judging easier?\n\nTrue, current way of testing gitweb does not allow for test which would\ndetect breakage noticed by Guillaume.\n\nIt would be quite easy I think to add checking if gitweb returns\nexpected HTTP return code (HTTP status). So what is the portable way\nto check if first line of some output matches given regexp (given fixed\nstring)?\n\n-- \nJakub Narebski\nPoland\n"},{"id":"62050","messageId":"7v7ijsq5zm.fsf@gitster.siamese.dyndns.org","threadId":"11073","inReplyTo":"200712051113.40654.jnareb@gmail.com","subject":"Re: [PATCH] gitweb: Try harder in parse_tag; perhaps it was given ambiguous name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-05T19:46:53Z","receivedAt":"2007-12-05T19:46:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> First patch, which is modified version of Guillaume Seguin patch solves\n> problem that links in gitweb does lead to correct 'tag' view, while the\n> second one solves the problem from the other side: instead of ensuring\n> that links in gitweb are unambiguous it tries to resolve ambiguity.\n\nOk, I'll queue the first one (disambiguate) for 1.5.4 while letting the\npeople decide the latter for now.\n\n> The problem is caused by the fact that git _always_ prefer heads (head\n> refs) to tags (tag refs), even when it is clear\n> $ git cat-file tags ambiguous-ref\n> that we want a tag. So alternate solution would be to correct\n> git-cat-file.\n\nYou are getting the layering all wrong.\n\n * git-cat-file takes \"object name\" on its command line (so do many\n other commands).\n\n * One of the way to spell an \"object name\" is to refer to it with a ref\n that can reach it (e.g. to name 12th generation parent of the tip of\n the master branch, you spell \"master~12\" and you are using the ref\n refs/heads/master).\n\n * You do not have to always write out the ref in full. There is a\n defined order to disambiguate refs (see git-rev-parse(1)), that\n allows you to say 'master' and it expands to either refs/tags/master,\n refs/heads/master or whatever.\n\nNow git-cat-file does not care how you spelled your object name, and has\nno business influencing the ref disambiguation order. You _could_ argue\n\"git cat-file tag \" _expects_ to name a tag, but that logic\nis very flawed (and that is why I said your understanding of layering is\nscrewed) for two reasons:\n\n (1) may be user input to the script that uses cat-file and the\n script may be expecting a tag there. Perhaps the script is about\n creating a new branch from a tag (expecting a tag) and adds some\n administrative info in the configuration file for the branch. It\n does first:\n\n\tt=$(git cat-file tag \"$1\") || die not a tag\n\n and later the script may want to do:\n\n\tgit branch $newone \"$1\"\n git config branch.$newone.description \"created from tag $1 ($t)\"\n\n If you make \"cat-file tag\" to favor tag, and in a similar fashion\n if you make \"branch\" favor branch, the above will not do what you\n expect.\n\n Consistently resolving the refname without (or minimum number of)\n exceptions would give less surprising result.\n\n (2) \"git cat-file -t \" is to find out what type the object is and\n is meant to be used by callers who do not know the type. There is\n no \"favoring this class of ref over other classses\" possible there.\n\n> It would be quite easy I think to add checking if gitweb returns\n> expected HTTP return code (HTTP status). So what is the portable way\n> to check if first line of some output matches given regexp (given fixed\n> string)?\n\nHuh? Wouldn't something like this be enough?\n\n\t>expect.empty &&\n\tcmd >actual.out 2>actual.err &&\n diff -u expect.empty actual.err &&\n first=$(sed -e '1q' Jakub Narebski writes:\n> \n> > It would be quite easy I think to add checking if gitweb returns\n> > expected HTTP return code (HTTP status). So what is the portable way\n> > to check if first line of some output matches given regexp (given fixed\n> > string)?\n> \n> Huh? Wouldn't something like this be enough?\n> \n> \t>expect.empty &&\n> \tcmd >actual.out 2>actual.err &&\n> diff -u expect.empty actual.err &&\n> first=$(sed -e '1q' \ttest \"z$first\" = \"I like it\"\n\nWell, actually that is even better idea. We can go for one of the three\nlevels of HTTP status checking:\n\n1. Check if we got \"Status: 200 OK\" when we expect it, and not have it\n when we expect other HTTP status, e.g. when requesting nonexistent\n file. The above code is enough for that.\n\n2. We can check if we got expected status number, for example 200 for\n when we expect no error, or 404 when object is not found, or 403\n if there is no such object etc. I was thinking about using this version\n the need to check not full first line, but fragment of it.\n\n3. We can check full first line, for example\n Status: 200 OK\n Status: 403 Forbidden\n Status: 404 Not Found\n Status: 400 Bad Request\n but this might tie gitweb test too tightly with minute details of\n gitweb output. The above code is good for that too.\n\nWhat do you think, which route we should go in test?\n-- \nJakub Narebski\nPoland\n"},{"id":"62055","messageId":"7vmysoondc.fsf@gitster.siamese.dyndns.org","threadId":"11073","inReplyTo":"200712052202.27111.jnareb@gmail.com","subject":"Re: [PATCH] gitweb: Try harder in parse_tag; perhaps it was given ambiguous name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-05T21:14:23Z","receivedAt":"2007-12-05T21:14:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> Well, actually that is even better idea. We can go for one of the three\n> levels of HTTP status checking:\n>\n> 1. Check if we got \"Status: 200 OK\" when we expect it, and not have it\n> when we expect other HTTP status, e.g. when requesting nonexistent\n> file. The above code is enough for that.\n>\n> 2. We can check if we got expected status number, for example 200 for\n> when we expect no error, or 404 when object is not found, or 403\n> if there is no such object etc. I was thinking about using this version\n> the need to check not full first line, but fragment of it.\n>\n> 3. We can check full first line, for example\n> Status: 200 OK\n> Status: 403 Forbidden\n> Status: 404 Not Found\n> Status: 400 Bad Request\n> but this might tie gitweb test too tightly with minute details of\n> gitweb output. The above code is good for that too.\n>\n> What do you think, which route we should go in test?\n\n4. We should check for what we expect in the parts of gitweb output we\n care about. E.g.\n\n * If we are making sure it refuses to serve incorrect request\n (e.g. no such repository), we should check for the status, which is\n what we care about (we may not care about how the actual error\n message is stated).\n\n * Otherwise (and I suspect this is \"most of the tests\"), we obviously\n expect to have \"200 OK\", and check for that;\n\n BUT IN ADDITION\n\n * If we want to make sure that a specific aspect of the output was\n buggy and a patch fixed it (e.g. an href used a short refname\n without having refs/heads or refs/tags prefixed, causing\n ambiguity), we also should check that part of output in generated\n HTML, not just the HTTP status header.\n"}]}