{"thread":{"id":"43258","subject":"Re: [PATCH/RFC] gitweb: New improved patchset view","startedAt":"2006-10-29T11:47:43Z","lastAt":"2006-10-29T19:33:15Z","messageCount":5,"participants":["Luben Tuikov","Jakub Narebski","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"296053","messageId":"7viri34a3k.fsf@assigned-by-dhcp.cox.net","threadId":"43258","inReplyTo":"200610291122.30852.jnareb@gmail.com","subject":"Re: [PATCH/RFC] gitweb: New improved patchset view","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-10-29T11:47:43Z","receivedAt":"2006-10-29T11:47:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\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 link\n> and uses default link style is changed to\n> diff --git a/ b/\n> where is hidden link (i.e. underline on hover, only)\n> to appropriate version of file. If file is added, a/ is not\n> hyperlinked, if file is deleted, b/ is not hyperlinked.\n\nI do not have time to look at the code right now, but here are \nquick comments on the output.\n\nI personally do not mind \"hidden\" but it might be more obvious\nto make them normal links -- the filenames in the header are not\npart of long text people need to \"read\". On the other hand,\nit feels a bit wasteful to have these hidden links both on \"diff\n--git\" line *and* ---/+++ lines (these three are very close to\neach other).\n\n> * there is added \"extended diff header\", with and \n> hyperlinked (and shortened to 7 characters), and \n> explained: '' is extnded to '/ ()'.\n\nIt somehow feels that deviating from what \"git diff\" gives makes\nit very distracting; I would feel better if \"/-rw-r--r-- (file)\"\nwere not there.\n\nAlso I think arguing over 7 or 8 hexdigits is pointless; if you\nare reading this from \"git diff\", it is probably the easiest to\nmatch what \"git diff\" gave you. One thing we _might_ want to do\nin the future is to change \"git diff\" to use DEFAULT_ABBREV\nhexdigits at the minimum but more if needed to disambiguate; I\nthink it currently does not do the \"more if needed\" part.\n\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.\n\nThis feels Ok.\n\n> * from-file/to-file two-line header lines have slightly darker color\n> than removed/added lines.\n\nThis visually feels right.\n\n> * chunk header has now delicate line above for easier finding chunk\n> boundary, and top margin of 1px.\n\nThis visually feels right.\n"},{"id":"294621","messageId":"200610291324.42566.jnareb@gmail.com","threadId":"43258","inReplyTo":"7viri34a3k.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH/RFC] gitweb: New improved patchset view","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2006-10-29T12:24:42Z","receivedAt":"2006-10-29T12:24:42Z","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>> 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 link\n>> and uses default link style is changed to\n>> diff --git a/ b/\n>> where is hidden link (i.e. underline on hover, only)\n>> to appropriate version of file. If file is added, a/ is not\n>> hyperlinked, if file is deleted, b/ is not hyperlinked.\n> \n> I do not have time to look at the code right now, but here are \n> quick comments on the output.\n\nCode is a bit horrible I think in trying to be smart and avoid\ncode repetition.\n\n> I personally do not mind \"hidden\" but it might be more obvious\n> to make them normal links -- the filenames in the header are not\n> part of long text people need to \"read\". On the other hand,\n> it feels a bit wasteful to have these hidden links both on \"diff\n> --git\" line *and* ---/+++ lines (these three are very close to\n> each other).\n\nGood idea. We have lost visible links with the change. And easy to\nimplement: just remove style override for .diff.header a.list from\nCSS (or what would be better but harder to code remove \"list\" class\nfrom element in \"git header\").\n\nOriginally there was visible link with full sha1 of blob as text,\nand hidden link with filename as text; perhaps dropping the latter\n---/+++ line link is better idea (here hyperlink should I think be\nhidden, if there is any, to match git-diff and GNU diff -u output).\nAnd this would simplify code somewhat...\n\nThe hidden (but here I think we can change to not-hidden links) links\nin the \"index\" extended header line are to stay I think: this is the\nonly place where we can have links to all the versions of file in the\nfuture combined commitdiff format.\n\n>> * there is added \"extended diff header\", with and \n>> hyperlinked (and shortened to 7 characters), and \n>> explained: '' is extnded to '/ ()'.\n> \n> It somehow feels that deviating from what \"git diff\" gives makes\n> it very distracting; I would feel better if \"/-rw-r--r-- (file)\"\n> were not there.\n\nWell, the old version had \"()\" in gitweb diff header.\nNot all git/gitweb users are familiar with POSIX numeric modes;\nperhaps changing the style (color for example) of added stuff\nto mark it as added would be enough?\n\n> Also I think arguing over 7 or 8 hexdigits is pointless; if you\n> are reading this from \"git diff\", it is probably the easiest to\n> match what \"git diff\" gave you. One thing we _might_ want to do\n> in the future is to change \"git diff\" to use DEFAULT_ABBREV\n> hexdigits at the minimum but more if needed to disambiguate; I\n> think it currently does not do the \"more if needed\" part.\n\nI used 7 hexdigits because git-diff uses 7 by default.\n\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.\n> \n> This feels Ok.\n\nI'm not sure if we shouldn't unquote filename always, or at least\nremove double quotes surrounding filename, or [ab]/filename. The decision\nto leave visible filename quoted was based on the troubles to code\nit correctly in \"git diff\" header where one of the filenames might\nnot be hyperlinked: if file was added or deleted.\n\n>> * from-file/to-file two-line header lines have slightly darker color\n>> than removed/added lines.\n> \n> This visually feels right.\n\nWhat is left is fine-tuning the color.\n\n>> * chunk header has now delicate line above for easier finding chunk\n>> boundary, and top margin of 1px.\n> \n> This visually feels right.\n\nWhat is left is fine-tuning the line and vertical space.\n-- \nJakub Narebski\n"},{"id":"298771","messageId":"200610291348.41389.jnareb@gmail.com","threadId":"43258","inReplyTo":"200610291324.42566.jnareb@gmail.com","subject":"Re: [PATCH/RFC] gitweb: New improved patchset view","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2006-10-29T12:48:41Z","receivedAt":"2006-10-29T12:48:41Z","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>> Jakub Narebski writes:\n>>>\n>>> * there is added \"extended diff header\", with and \n>>> hyperlinked (and shortened to 7 characters), and \n>>> explained: '' is extnded to '/ ()'.\n>> \n>> It somehow feels that deviating from what \"git diff\" gives makes\n>> it very distracting; I would feel better if \"/-rw-r--r-- (file)\"\n>> were not there.\n> \n> Well, the old version had \"()\" in gitweb diff header.\n> Not all git/gitweb users are familiar with POSIX numeric modes;\n> perhaps changing the style (color for example) of added stuff\n> to mark it as added would be enough?\n\nOr we could use the following code:\n\n 100755\n\nwhich gives explanation on mouse-over.\n-- \nJakub Narebski\n"},{"id":"294270","messageId":"200610291635.09525.jnareb@gmail.com","threadId":"43258","inReplyTo":"7viri34a3k.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH/RFC] gitweb: New improved patchset view","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2006-10-29T15:35:09Z","receivedAt":"2006-10-29T15:35: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>> Changes:\n[...]\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.\n> \n> This feels Ok.\n\nEmphasisis on \"should\". Check\n http://roke(.)dyndns(.)info/cgi-bin/gitweb/gitweb.cgi?p=git.git;a=commitdiff;h=gitweb/test\n\nBy the way, should we use quoted or unquoted filename?\n-- \nJakub Narebski\n"},{"id":"294154","messageId":"952802.95154.qm@web31802.mail.mud.yahoo.com","threadId":"43258","inReplyTo":"7viri34a3k.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH/RFC] gitweb: New improved patchset view","fromName":"Luben Tuikov","fromEmail":"ltuikov@yahoo.com","sentAt":"2006-10-29T19:33:15Z","receivedAt":"2006-10-29T19:33:15Z","isPatch":true,"sender":{"key":"ltuikov@yahoo.com","avatar":null},"body":"--- Junio C Hamano wrote:\n> It somehow feels that deviating from what \"git diff\" gives makes\n> it very distracting; I would feel better if \"/-rw-r--r-- (file)\"\n> were not there.\n\nAgree.\n\n> Also I think arguing over 7 or 8 hexdigits is pointless; if you\n> are reading this from \"git diff\", it is probably the easiest to\n> match what \"git diff\" gave you. One thing we _might_ want to do\n\nAgree.\n\n Luben\n"}]}