{"thread":{"id":"43275","subject":"Re: [PATCH/RFC (take 2)] gitweb: New improved patchset view","startedAt":"2006-10-30T01:59:18Z","lastAt":"2006-10-30T23:40:23Z","messageCount":9,"participants":["Jakub Narebski","Junio C Hamano","Luben Tuikov"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"297535","messageId":"92622.251.qm@web31812.mail.mud.yahoo.com","threadId":"43275","inReplyTo":"200610300051.37896.jnareb@gmail.com","subject":"Re: [PATCH/RFC (take 2)] gitweb: New improved patchset view","fromName":"Luben Tuikov","fromEmail":"ltuikov@yahoo.com","sentAt":"2006-10-30T01:59:18Z","receivedAt":"2006-10-30T01:59:18Z","isPatch":true,"sender":{"key":"ltuikov@yahoo.com","avatar":null},"body":"--- Jakub Narebski wrote:\n> Replace \"gitweb diff header\" with its full sha1 of blobs and replace\n> it by \"git diff\" header and extended diff header. Change also somewhat\n> highlighting of diffs.\n> \n> Changes:\n> * \"gitweb diff header\" which looked for example like below:\n> file:__ -> file:__\n> where 'file' is file type and '' is full sha1 of blob is\n> changed to\n> diff --git _a/_ _b/_\n> In both cases links are visible and use default link style. If file\n> is added, a/ is not hyperlinked, if file is deleted, b/\n> is not hyperlinked.\n\n\"Everything clickable underlined\" isn't the best way to represent things.\nAnyway, my 2 cents is that I don't like the overly explicit underlineing.\nI liked it the way it was in take 1.\n\n> * there is added \"extended diff header\", with and \n> hyperlinked (and shortened to 7 characters), and \n> explained: '' is extended to ' ()'.\n> * hyperlinking should work also when is originally\n> quoted. For now we present filename quoted. This needed changes to\n> parse_difftree_raw_line subroutine. And doesn't work: perhaps\n> unquote is broken.\n\nIn which case we shouldn't commit this. IOW, let's commit things\nwhich we _know_ to work.\n\nWhy not resubmit your original patch with the bugfixes as few comments\nas mentioned?\n\n> * from-file/to-file two-line header lines have slightly darker color\n> than removed/added lines.\n> * chunk header has now delicate line above for easier finding chunk\n> boundary, and top margin of 1px.\n\nWouldn't this be confusing with the other fine lines?\nI personally don't like this chunk separation. Chunk separation\nalready exists as is and we view it all the time elsewhere.\n\nIf you'd like to separate chunks, why not darken the background\nof the section of line the chunk header is printed at? I.e.\nanything between the @@ including the @@.\n\n Luben\n"},{"id":"295032","messageId":"200610300905.04454.jnareb@gmail.com","threadId":"43275","inReplyTo":"92622.251.qm@web31812.mail.mud.yahoo.com","subject":"Re: [PATCH/RFC (take 2)] gitweb: New improved patchset view","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2006-10-30T08:05:04Z","receivedAt":"2006-10-30T08:05:04Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Luben Tuikov wrote:\n> --- Jakub Narebski wrote:\n>> Replace \"gitweb diff header\" with its full sha1 of blobs and replace\n>> it by \"git diff\" header and extended diff header. Change also somewhat\n>> highlighting of diffs.\n>> \n>> Changes:\n>> * \"gitweb diff header\" which looked for example like below:\n>> file:__ -> file:__\n>> where 'file' is file type and '' is full sha1 of blob is\n>> changed to\n>> diff --git _a/_ _b/_\n>> In both cases links are visible and use default link style. If file\n>> is added, a/ is not hyperlinked, if file is deleted, b/\n>> is not hyperlinked.\n> \n> \"Everything clickable underlined\" isn't the best way to represent things.\n> Anyway, my 2 cents is that I don't like the overly explicit underlineing.\n> I liked it the way it was in take 1.\n\nThet is the only \"obviously link\" per patch. And I think there should be\nat least one non-hidden link.\n\nBTW. comments like this are the reason I've sent the patch as-is, without\nresolving the strange filenames problem (it would be nice if somebody was\nto send code; well Junio send patch to address core git filename quoting\nissue).\n\n>> * there is added \"extended diff header\", with and \n>> hyperlinked (and shortened to 7 characters), and \n>> explained: '' is extended to ' ()'.\n>> * hyperlinking should work also when is originally\n>> quoted. For now we present filename quoted. This needed changes to\n>> parse_difftree_raw_line subroutine. And doesn't work: perhaps\n>> unquote is broken.\n> \n> In which case we shouldn't commit this. IOW, let's commit things\n> which we _know_ to work.\n> \n> Why not resubmit your original patch with the bugfixes as few comments\n> as mentioned?\n\nI'll do that, but for now quoting/unquoting filename is broken, both\nin gitweb but also to lesser extent in git core (quoting perfectly valid\nUTF-8 characters).\n\nI'll try to adress that, but I wanted to send next RFC patch for review.\n\n>> * from-file/to-file two-line header lines have slightly darker color\n>> than removed/added lines.\n>> * chunk header has now delicate line above for easier finding chunk\n>> boundary, and top margin of 1px.\n> \n> Wouldn't this be confusing with the other fine lines?\n> I personally don't like this chunk separation. Chunk separation\n> already exists as is and we view it all the time elsewhere.\n\nBut not always the program displaying diff can display such line\nseparating chunks, for example on text terminal it can't.\n\nBut if you think that the dotted 1px #ffbbff line is too intrusive,\nwe can remove it (and perhaps increase vertical space a few pixels).\nI'd like to have more opinions first.\n\nBTW. you can easily override it in your CSS file.\n \n> If you'd like to separate chunks, why not darken the background\n> of the section of line the chunk header is printed at? I.e.\n> anything between the @@ including the @@.\n\nI'd rather have this one in a separate commit (this needs changes\nto format_diff_line, not only to git_patchset_body).\n\n-- \nJakub Narebski\n"},{"id":"297553","messageId":"7vy7qyw6w6.fsf@assigned-by-dhcp.cox.net","threadId":"43275","inReplyTo":"200610300905.04454.jnareb@gmail.com","subject":"Re: [PATCH/RFC (take 2)] gitweb: New improved patchset view","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-10-30T08:21:45Z","receivedAt":"2006-10-30T08:21:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> ..., without\n> resolving the strange filenames problem (it would be nice if somebody was\n> to send code; well Junio send patch to address core git filename quoting\n> issue).\n\nHaving showed that patch, I do not think it is a good way to go.\n\nI think the UI layer like gitweb should have freedom to choose\nits own pathname handling, and should read from -z output.\n\nThe git plumbing is agnostic to the character encoding issues,\nand does not care what character set pathnames are encoded, and\nwhat character set the file contents are encoded. These are\nvery project specific and should be handled per project.\n\nOne of the primary purposes, aside from being understandable by\nhumans, of human readable \"git diff\" format is to convey machine\nreadable information without corruption in text form transport\n(think \"e-mail\"), so quoting pathnames to avoid high-bits has\nclear advantage (pathname character set and file contents\ncharacter set are often different, and when both are non-ascii\nwe would need to sacrifice one if we spit diff out in a single\ntext file --- we choose to quote pathnames so that it would not\ninterfere with file contents, which would be more important to\nbe inspectable by humans reading \"diff\" output). People in the\npast suggested converting everything to UTF-8 but that is not a\nusable robust solution in the real world.\n\nThe HTML output generated by gitweb do not have the \"machine\nreadably robust\" requirement, so it _can_ stress more on\nreadability by humans, including converting both pathname and\ncontents to UTF-8; for this, the project needs to tell gitweb\nwhat its pathname character encoding is, and blob contents\nencoding, at the worst case blob-by-blob, but more often\npath-by-path.\n"},{"id":"296200","messageId":"7vlkmyw5hx.fsf@assigned-by-dhcp.cox.net","threadId":"43275","inReplyTo":"7vy7qyw6w6.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH/RFC (take 2)] gitweb: New improved patchset view","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-10-30T08:51:54Z","receivedAt":"2006-10-30T08:51:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jakub Narebski writes:\n>\n>> ..., without\n>> resolving the strange filenames problem (it would be nice if somebody was\n>> to send code; well Junio send patch to address core git filename quoting\n>> issue).\n>\n> Having showed that patch, I do not think it is a good way to go.\n>\n> I think the UI layer like gitweb should have freedom to choose\n> its own pathname handling, and should read from -z output.\n>\n> The git plumbing is agnostic to the character encoding issues,\n> and does not care what character set pathnames are encoded, and\n> what character set the file contents are encoded. These are\n> very project specific and should be handled per project.\n\nSide note.\n\nThere is one action item for shorter term I realized we would\nneed. We need to make patch-id generation use unquoted\nfilenames, so that no matter how differently we might\nreimplement quote_c_style() later, we would get the same\npatch-id from the diff between the same two trees. Currently I\nthink it hashes the quoted filename that appear on ---/+++ lines\n(and \"rename from\"/\"rename to\" lines as well, but usually you do\nnot use -M/-C when computing the patch-id, so these are not\nreally an issue).\n\nI think longer term plan for git should include standardizing on\nhow pathname encoding and content encoding are handled at the UI\nlayer.\n\nAnd that plan _should_ eventually include the textual diff part.\nBut textual diff has a machine readability requirements, so we\nneed to proceed very carefully. I think the patch I sent out\nearlier is probably good enough if we only care about utf-8, but\nwould not correctly handle other sane pathname encodings such as\nextended UNIX code (let alone broken but still widely used\nencodings such as MS-Kanji aka Shift_JIS).\n\n"},{"id":"295847","messageId":"200610301043.22033.jnareb@gmail.com","threadId":"43275","inReplyTo":"7vy7qyw6w6.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH/RFC (take 2)] gitweb: New improved patchset view","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2006-10-30T09:43:21Z","receivedAt":"2006-10-30T09:43:21Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Junio C Hamano wrote:\n> Jakub Narebski writes:\n> \n>> ..., without\n>> resolving the strange filenames problem (it would be nice if somebody was\n>> to send code; well Junio send patch to address core git filename quoting\n>> issue).\n> \n> Having showed that patch, I do not think it is a good way to go.\n> \n> I think the UI layer like gitweb should have freedom to choose\n> its own pathname handling, and should read from -z output.\n\nThat's a very good idea. I'll send separate patch (if noone else will\ndo this, that is) which would convert gitweb to always use -z output,\nboth git-ls-tree and git-diff-tree... oh, well, there is no -z patch\noutput, so in patch part we would have to replace git quoted part by\ngitweb quoted part.\n\nBTW gitweb has to do it's own pathname handling, at least LF, perhaps\nTAB, perhaps other control characters like ESC; quote quoting character\nand mark filename as quoted somewhat (it can be with CSS style, not\nnecessary by surrounding with some kind of quotes; besides that can\nalso be added via CSS in modern enough browsers using :before and\n:after pseudo-elements and 'content' property).\n\n-- \nJakub Narebski\n"},{"id":"298158","messageId":"200610301458.33634.jnareb@gmail.com","threadId":"43275","inReplyTo":"200610301043.22033.jnareb@gmail.com","subject":"Re: [PATCH/RFC (take 2)] gitweb: New improved patchset view","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2006-10-30T13:58:32Z","receivedAt":"2006-10-30T13:58:32Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Jakub Narebski wrote:\n> Junio C Hamano wrote:\n>>\n>> I think the UI layer like gitweb should have freedom to choose\n>> its own pathname handling, and should read from -z output.\n> \n> That's a very good idea. I'll send separate patch (if noone else will\n> do this, that is) which would convert gitweb to always use -z output,\n> both git-ls-tree and git-diff-tree... oh, well, there is no -z patch\n> output, so in patch part we would have to replace git quoted part by\n> gitweb quoted part.\n\nI have realized that it is not as easy as it sounds, at least for the\ngit-diff-tree output. For the LF-terminated output (without '-z') you\nknow that LF separates records, and you can split on LF ('\\n'). It is\nnot the case for '-z' '\\0' delimited output: NUL ('\\0') might also mean\nend of one of the filenames in the rename/copy case, and is used to \nseparate filename(s) from the score (although here TAB would be \nenough). And that is probably the case that gitweb uses default \ngit-diff-tree output, and _tries_ to unescape(...) filename.\n\nThe solution would be perhaps to add '--zz' option to use '-z' output \nbut to separate records by double NUL, i.e. '\\0\\0'...\n\nBy the way, why diff-tree \"raw\" format for merge gives only one, final,\nfilename?\n-- \nJakub Narebski\n"},{"id":"297650","messageId":"7v7iyhv29o.fsf@assigned-by-dhcp.cox.net","threadId":"43275","inReplyTo":"200610301458.33634.jnareb@gmail.com","subject":"Re: [PATCH/RFC (take 2)] gitweb: New improved patchset view","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-10-30T22:59:15Z","receivedAt":"2006-10-30T22:59:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> I have realized that it is not as easy as it sounds, at least for the\n> git-diff-tree output...\n\nI do not think so.a The output is designed to be machine\nparsable. See Documentation/diff-format.txt and also please\nrefer to 65a9289d:diff-helper.c for a sample code to see how to\nparse the output.\n\n\n"},{"id":"294195","messageId":"200610310032.09726.jnareb@gmail.com","threadId":"43275","inReplyTo":"7v7iyhv29o.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH/RFC (take 2)] gitweb: New improved patchset view","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2006-10-30T23:32:09Z","receivedAt":"2006-10-30T23:32:09Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Junio C Hamano wrote:\n> Jakub Narebski writes:\n> \n>> I have realized that it is not as easy as it sounds, at least for the\n>> git-diff-tree output...\n> \n> I do not think so.a The output is designed to be machine\n> parsable. See Documentation/diff-format.txt and also please\n> refer to 65a9289d:diff-helper.c for a sample code to see how to\n> parse the output.\n\nWhat I wanted to say is that the output is not easy to use in\n\"split first, parse later\", with sometimes counting number of entries\nin between. With current format you rather have to pass in one go,\nfor '-z' format reading filenames from the stream.\n\nBy the way, git-ls-tree -z uses TAB as a start of filename, while\ngit-diff-tree -z uses NUL as a start of (first) filename. Why?\n-- \nJakub Narebski\n"},{"id":"295422","messageId":"7vd589tlso.fsf@assigned-by-dhcp.cox.net","threadId":"43275","inReplyTo":"200610310032.09726.jnareb@gmail.com","subject":"Re: [PATCH/RFC (take 2)] gitweb: New improved patchset view","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-10-30T23:40:23Z","receivedAt":"2006-10-30T23:40:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> By the way, git-ls-tree -z uses TAB as a start of filename, while\n> git-diff-tree -z uses NUL as a start of (first) filename. Why?\n\nI do not know a good explanation other than hysterical raisins.\n"}]}