{"thread":{"id":"43433","subject":"Re: [RFC] gitweb TODO","startedAt":"2006-11-17T18:01:40Z","lastAt":"2006-11-18T00:04:05Z","messageCount":6,"participants":["Jakub Narebski","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"295478","messageId":"200611171901.40839.jnareb@gmail.com","threadId":"43433","inReplyTo":null,"subject":"[RFC] gitweb TODO","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2006-11-17T18:01:40Z","receivedAt":"2006-11-17T18:01:40Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"These are a few gitweb issues and features I'm currently working on \n(or plan working on).\n\n1. New patchset view (commitdiff, blobdiff)\n\nIn \"old\" gitweb commitdiff view was generated by iterating over lines of \ngit-diff-tree raw format output, and generating diffs using \ngit-cat-file and external diff utility (/usr/bin/diff). This required \nhaving temporary directory for diff generation, and of course diffs \ndidn't have extended git headers.\n\nThe \"new\" commitdiff view is generated from single git-diff-tree \n--raw-with-path output. But I have made incorrect assumption that one \nline from \"raw\" diff-tree output always corresponds to only one patch \nin the patchset part of output. This is not the case. I'm not sure if \nthose are the only cases when patch is broken, but changing file into \nsymlink or symlink into file ('T' status), and explicit breaking ('B' \nstatus) generates two patches to one line of raw difftree output. The \nsecond is not of much importance for gitweb, unless yoy add -B to \n@diff_opts, but the first is important; it is currently broken, see\n   http://tinyurl.com/y3cfop\n(commit 4c52c0d31f0f7142d81a465c40789befc2e86548 on \ngitweb-test-funny-char branch in git.git repository).\n\nI have thought of the following (mutually exclusive) ways to fix this\n\n a. Change core git git-diff-tree command to not break (some?) of T\n    changes into two patches. From what gitster said on #git this \n    feature is for git-diff patches to be patch(1) compatibile; but\n    -M already causes patches to be incompatibile with patch. I'm\n    thinking here about adding some kind of -s/--single-patch\n    --do-not-break-patch-into-two-please command line option for\n    git-diff\n\n b. Check the raw difftree line for status and perhaps other info\n    to know if the line generates more than one patch. It needs detailed\n    knowledge about _when_ git-diff generates more than one patch to one\n    \"raw\" format line, and would break if core diff changes in that\n    detail. Simplest to implement, I think...\n\n    Could you tell me all the cases when git generates more than one\n    patch for one \"raw\" diff format line, please?\n\n c. \"Cache\" git diff header, or the whole patch, or the whole patchset.\n    It is enough to cache (write lines to \"buffer\"/\"cache\" array) up to\n    the extended header \"^index\" line, which can be used to check if to\n    go to the next dofftree \"raw\" line (or wven which of \"raw\" difftree\n    lines this particular patch corresponds to). Does not require\n    changes in diff core, and is less fragile, less susceptible to\n    breakage.\n\nWhich of those would be the best to implement?\n\n\n2. Difftree combined diff gitweb \"raw\" format\n\nCurrently \"commitdiff\" view consist of the gitweb representation of \n\"raw\" git-diff output (list of changed files a la git-whatchanged), and \nthe patchset (a la git-show). \"commit\" view has only list of changed \nfiles, nearly exacttly the same as in \"commitdiff\" view (but with links \nto blobdiff view instead of links to appropriate patch in \"commitdiff\" \nview).\n\nI have though about using one of the combined diff outputs for merge \ncommits. The problem is how to represent the whatchanged part. Which \nparts of gitweb difftree output to leave? And what about the fact that \nwe have raw output for -c/--combined diff format, but not for chunk \nsimplifying --cc (compact combined) output?\n\n\n3. Committags support (and implementation)\n\nThere was some proposed implementation here, both by me and by Junio, \nbut no definite patches were accepted. We have the following mutually \nexclusive ways to do this:\n\n a. Do esc_html first, then do all committags simultaneously. The\n    advantage is that it is perhaps slightly faster, the disadvantages\n    is additional complication in code, and the fact that regexp\n    defining committags have to be either on esc_html-ed pattern, or be\n    converted to esc_html-ed input.\n\n b. Have a chain of committags, and do committags sequentially. This\n    means that we have to divide output into part to be further parsed,\n    and the part which should not be parsed further; here Junio proposed\n    wondefull idea of list mixed of strings (to be committags parsed)\n    and string references (to be left as is). As the last \"committag\"\n    we perform esc_html (uless for example some project stores commit\n    messages in HTML, XML, or some structured text like AsciiDoc or\n    reStructuredText).\n\nWhich of those would be better to implement?\n\n\n4. Feeds (RSS, Atom,...)\n\nThere is new Atom format feed, there was request for per-branch feeds.\nThe feed output certainly needs cleanup. The questions are: \n\na.) What should be in the feed (only commit message + authorship, or \nalso whatchanged; what format use for whatchanged; do add information \nabout tags created with given window of time, etc.)? \n\nb.) Should we use \"text/xml\" or (unstandarized) \"application/rss+xml\" as \ntype/Content-Type for RSS 2.01 format, should we use \n\"application/atom+xml\" for Atom format?\n\nc.) Should we add APP (Atom Publishing Protocol) support in addition to \nOPML?\n\nd.) Should we put log, shortlog, history, rss, atom views together in \none subroutine, and select the view using $format parameter of this \nsubroutine?\n\n-- \nJakub Narebski\n"},{"id":"298080","messageId":"7vwt5tlvy5.fsf@assigned-by-dhcp.cox.net","threadId":"43433","inReplyTo":"200611171901.40839.jnareb@gmail.com","subject":"Re: [RFC] gitweb TODO","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-11-17T19:22:42Z","receivedAt":"2006-11-17T19:22:42Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski <jnareb@gmail.com> writes:\n\n> These are a few gitweb issues and features I'm currently working on \n> (or plan working on).\n>\n> 1. New patchset view (commitdiff, blobdiff)\n>\n>  c. \"Cache\" git diff header, or the whole patch, or the whole patchset.\n>     It is enough to cache (write lines to \"buffer\"/\"cache\" array) up to\n>     the extended header \"^index\" line, which can be used to check if to\n>     go to the next dofftree \"raw\" line (or wven which of \"raw\" difftree\n>     lines this particular patch corresponds to). Does not require\n>     changes in diff core, and is less fragile, less susceptible to\n>     breakage.\n\nThis is the most appropriate.  Right now it is not independently\ncontrollable but it is not so inplausible for somebody to want\nto get non recursive view of 'raw' part along with textual diffs\nwhen running \"--raw -p\" diff and your solution c. is robust\nagainst even such changes.\n\nI would probably not call that \"caching\" but keeping track of\nwhere you are, and you would need to know in which filepair you\nare in anyway when you want to implement links to blob view from\nthe hunk header lines.\n\n> 2. Difftree combined diff gitweb \"raw\" format\n>...\n> I have though about using one of the combined diff outputs for merge \n> commits. The problem is how to represent the whatchanged part. Which \n> parts of gitweb difftree output to leave? And what about the fact that \n> we have raw output for -c/--combined diff format, but not for chunk \n> simplifying --cc (compact combined) output?\n\nI am not sure what's more useful to show there.  The part of the\noutput shows \"the list of files that have changed by this\ncommit\" for non-merge commits, so \"the list of files that have\nchanged in this merge\" is what you would want to show, but there\nare three ways you can define \"what changed\" for a merge (see\nthe message that explains --cc to linux@horizontal I sent\nyesterday).  I find \"diff --name-status $it^1 $it\" the most\nnatural semantics for the most of the time, and that is what we\ndo for --stat output.\n\nIf you want to treat all the parents equally, you could read\nfrom \"diff-tree -c --raw $it\" which would show list of files\nthat needed file-level merges, along with the blob object names\nfrom all parents.\n\nWe might want to give that when \"diff-tree --cc --patch-with-raw\n$it\" is asked for in the \"raw\" part.  I dunno.\n\n> 3. Committags support (and implementation)\n>...\n> Which of those would be better to implement?\n\nI think that is a minor implementation detail; I think the\nlatter is probably less painful to maintain in the longer run\nbecause if you encode things in earlier stages, postprocessing\nstages need to know which part of the result from the earlier\nprocessing needs decoding before further processing, but I\nsuspect you will be the one primarily hacking on that part, so\nit is not my preference, but just a suggestion to make your life\nless painful.\n"},{"id":"298407","messageId":"200611172130.11631.jnareb@gmail.com","threadId":"43433","inReplyTo":"7vwt5tlvy5.fsf@assigned-by-dhcp.cox.net","subject":"Re: [RFC] gitweb TODO","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2006-11-17T20:30:11Z","receivedAt":"2006-11-17T20:30:11Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Junio C Hamano wrote:\n> Jakub Narebski <jnareb@gmail.com> writes:\n> \n>> These are a few gitweb issues and features I'm currently working on \n>> (or plan working on).\n>>\n>> 1. New patchset view (commitdiff, blobdiff)\n>>\n>>  c. \"Cache\" git diff header, or the whole patch, or the whole patchset.\n>>     It is enough to cache (write lines to \"buffer\"/\"cache\" array) up to\n>>     the extended header \"^index\" line, which can be used to check if to\n>>     go to the next dofftree \"raw\" line (or wven which of \"raw\" difftree\n>>     lines this particular patch corresponds to). Does not require\n>>     changes in diff core, and is less fragile, less susceptible to\n>>     breakage.\n> \n> This is the most appropriate.  Right now it is not independently\n> controllable but it is not so inplausible for somebody to want\n> to get non recursive view of 'raw' part along with textual diffs\n> when running \"--raw -p\" diff and your solution c. is robust\n> against even such changes.\n\nWhat about the fact that git-diff -M is _not_ patch-compatibile;\ndoes creating two patches for one difftree raw format line for\nmode change/'T' status (I guess only for \"type\" mode changes, i.e.\nfile to/from symlink, file to/from directory) helps understanding\nthe change? If not, perhaps it would be better to introduce option\nto not break the patch...\n \n> I would probably not call that \"caching\" but keeping track of\n> where you are, and you would need to know in which filepair you\n> are in anyway when you want to implement links to blob view from\n> the hunk header lines.\n\nI'd say \"buffering\" rather than \"caching\". The problem is that\nyou have to read up to the \"index <hash>..<hash>[ <mode>]\" to\ncheck if you have to go to the next raw difftree line or not.\nAnd the info from difftree line is needed to hyperlink parts\nof extended git diff header (and also to hyperlink parts of\npatch).\n\n>> 2. Difftree combined diff gitweb \"raw\" format\n>>...\n>> I have though about using one of the combined diff outputs for merge \n>> commits. The problem is how to represent the whatchanged part. Which \n>> parts of gitweb difftree output to leave? And what about the fact that \n>> we have raw output for -c/--combined diff format, but not for chunk \n>> simplifying --cc (compact combined) output?\n> \n> I am not sure what's more useful to show there.  The part of the\n> output shows \"the list of files that have changed by this\n> commit\" for non-merge commits, so \"the list of files that have\n> changed in this merge\" is what you would want to show, but there\n> are three ways you can define \"what changed\" for a merge (see\n> the message that explains --cc to linux@horizontal I sent\n> yesterday).  I find \"diff --name-status $it^1 $it\" the most\n> natural semantics for the most of the time, and that is what we\n> do for --stat output.\n> \n> If you want to treat all the parents equally, you could read\n> from \"diff-tree -c --raw $it\" which would show list of files\n> that needed file-level merges, along with the blob object names\n> from all parents.\n\nWe should have whatchanged part corresponding to the patchset\npart at least in \"commitdiff\" view, which means '-c' (and for\nthe time being perhaps mean '-c' also in patchset part). '--cc'\nwhich uses '-c' for the raw part would be nice...\n\n\"Commit\" view could use \"diff --name-status $hash^1 $hash\", \n(i.e. I think the same what it does now), even if it is not\ncompatibile with \"commitdiff\" view.\n\n-- \nJakub Narebski\n"},{"id":"297919","messageId":"7vu00xixxk.fsf@assigned-by-dhcp.cox.net","threadId":"43433","inReplyTo":"200611172130.11631.jnareb@gmail.com","subject":"Re: [RFC] gitweb TODO","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-11-17T21:08:07Z","receivedAt":"2006-11-17T21:08:07Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski <jnareb@gmail.com> writes:\n\n> What about the fact that git-diff -M is _not_ patch-compatibile;\n\nWhat about it?  I've never said patch compatibility is an issue.\nWe have something patch cannot represent or understand and you\nshould admit it.  The point is to make it easier to massage by\nhand, when the recipient does not have git handy.\n\nWith -M, the recipient can read and understand the patch text\nbetter than \"remove this oldfile and create this newfile that\nthe diff output does not tell you is related\" diff.  And we say\n\"rename\" in plain language so the recipient _can_ do \"mv A B\"\nthen \"patch -p1\".  Similarly, with -T that changes a symlink\ninto a real file, if we do not do the current \"remove the old\nand then create the new\" and did instead \"show the textual diff\nthat can be applied\", a non-git tool that does not understand\nthe typechange can mistakenly muck with the target of the\nsymlink, which is a disaster.  \"Remove the target and then\ncreate this\" at least would have lesser damage -- the object\nleft as the result is incorrect nevertheless, but reading the\ncontents and creating a symlink that has that contents by hand\nis easily done in a pinch.\n\n> We should have whatchanged part corresponding to the patchset\n> part at least in \"commitdiff\" view, which means '-c' (and for\n> the time being perhaps mean '-c' also in patchset part). '--cc'\n> which uses '-c' for the raw part would be nice...\n\nI am not sure what you mean by patchset part, but if you are\ntalking about the multiway diff text, I think most of the time\noutput from \"-c -p\" is much less interesting than \"--cc\".\n\n\n"},{"id":"294764","messageId":"200611172224.40252.jnareb@gmail.com","threadId":"43433","inReplyTo":"7vu00xixxk.fsf@assigned-by-dhcp.cox.net","subject":"Re: [RFC] gitweb TODO","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2006-11-17T21:24:39Z","receivedAt":"2006-11-17T21:24:39Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Junio C Hamano wrote:\n> Jakub Narebski <jnareb@gmail.com> writes:\n> \n>> What about the fact that git-diff -M is _not_ patch-compatibile;\n> \n> What about it?  I've never said patch compatibility is an issue.\n> We have something patch cannot represent or understand and you\n> should admit it.  The point is to make it easier to massage by\n> hand, when the recipient does not have git handy.\n> \n> With -M, the recipient can read and understand the patch text\n> better than \"remove this oldfile and create this newfile that\n> the diff output does not tell you is related\" diff.  And we say\n> \"rename\" in plain language so the recipient _can_ do \"mv A B\"\n> then \"patch -p1\".  Similarly, with -T that changes a symlink\n> into a real file, if we do not do the current \"remove the old\n> and then create the new\" and did instead \"show the textual diff\n> that can be applied\", a non-git tool that does not understand\n> the typechange can mistakenly muck with the target of the\n> symlink, which is a disaster.  \"Remove the target and then\n> create this\" at least would have lesser damage -- the object\n> left as the result is incorrect nevertheless, but reading the\n> contents and creating a symlink that has that contents by hand\n> is easily done in a pinch.\n\nSo we split patch for \"type change\" mode change for patch -p1\nsafety. But for gitweb more important than producing safe patch\nis producing _readable_ patch[*1*]. Hence request for -T option \n(or under other, better name) to git-diff which would _not_\nsplit patch (not create two patches for one raw difftree line).\n \n[*1*] Well, as far as diff for symlink is readable anyway.\n\n>> We should have whatchanged part corresponding to the patchset\n>> part at least in \"commitdiff\" view, which means '-c' (and for\n>> the time being perhaps mean '-c' also in patchset part). '--cc'\n>> which uses '-c' for the raw part would be nice...\n> \n> I am not sure what you mean by patchset part, but if you are\n> talking about the multiway diff text, I think most of the time\n> output from \"-c -p\" is much less interesting than \"--cc\".\n\nSorry, perhaps I was not clear. I'd like for git-diff-tree --cc\n--raw -p to output both raw part (perhaps taken from -c) and\npatch part[*2*]. Gitweb needs raw part for both whatchanged \nand also for creating hyperlinks and gitweb quoted filenames\nin the patch part (although with git diff headers buffering\nand git diff extended headers parsing we could get the information\nneeded for hyperlinks and gitweb quoting of filenames for patch\npart from patch part).\n\n[*2*] I'm wondering why we don't have --patch long version of -p\noption to git-diff[-tree].\n\n-- \nJakub Narebski\n"},{"id":"295113","messageId":"7vveldhb7u.fsf@assigned-by-dhcp.cox.net","threadId":"43433","inReplyTo":"200611172224.40252.jnareb@gmail.com","subject":"Re: [RFC] gitweb TODO","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-11-18T00:04:05Z","receivedAt":"2006-11-18T00:04:05Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski <jnareb@gmail.com> writes:\n\n> ... But for gitweb more important than producing safe patch\n> is producing _readable_ patch[*1*]. Hence request for -T option \n> (or under other, better name) to git-diff which would _not_\n> split patch (not create two patches for one raw difftree line).\n>  \n> [*1*] Well, as far as diff for symlink is readable anyway.\n\nHonestly, I do not have strong feeling either way.  As you say,\nI suspect diff to change between symlink and regular file is not\nreadable no matter how you present it, and it is a corner case\nthat is not very interesting.  It happens in real life but it is\nrare enough that split patches or a single patch would not make\nmuch difference either way.\n\nSo I would not oppose to a patch to add an option to update\ngit-diff to produce either format, but I doubt it is worth the\neffort required to make sure that the change does not break\nanything else and also to make matching adjustment to git-apply,\nso that it can understand both formats.\n\n>> I am not sure what you mean by patchset part, but if you are\n>> talking about the multiway diff text, I think most of the time\n>> output from \"-c -p\" is much less interesting than \"--cc\".\n>\n> Sorry, perhaps I was not clear. I'd like for git-diff-tree --cc\n> --raw -p to output both raw part (perhaps taken from -c) and\n> patch part...\n\nI can see why somebody might want to do this, to exactly the\nsame degree that I can see why somebody might want to use the\ncombination of \"--raw -p\".\n\nI think this would work in git.git itself:\n\n\tgit diff-tree --cc --numstat --raw -p v1.0.0\n"}]}