{"thread":{"id":"13775","subject":"[PATCH] Avoid errors from git-rev-parse in gitweb blame","startedAt":"2008-06-03T10:46:17Z","lastAt":"2008-06-08T20:28:52Z","messageCount":40,"participants":["Rafael Garcia-Suarez","Lea Wiemann","Jakub Narebski","Luben Tuikov","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"78476","messageId":"1212489977-26822-1-git-send-email-rgarciasuarez@gmail.com","threadId":"13775","inReplyTo":null,"subject":"[PATCH] Avoid errors from git-rev-parse in gitweb blame","fromName":"Rafael Garcia-Suarez","fromEmail":"rgarciasuarez@gmail.com","sentAt":"2008-06-03T10:46:17Z","receivedAt":"2008-06-03T10:46:17Z","isPatch":true,"sender":{"key":"rgarciasuarez@gmail.com","avatar":null},"body":"git-rev-parse will abort with an error when passed a non-existent\nrevision spec, such as \"deadbeef^\" where deadbeef has no parent.\nUsing the --revs-only parameter makes this error go away, while\nretaining functionality, keeping the web server error log nice\nand clean.\n\nSigned-off-by: Rafael Garcia-Suarez \n---\n gitweb/gitweb.perl | 4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 55fb100..f3b4b24 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -4226,9 +4226,9 @@ HTML\n \t\t\t esc_html($rev));\n \t\t\tprint \"\\n\";\n \t\t}\n-\t\topen (my $dd, \"-|\", git_cmd(), \"rev-parse\", \"$full_rev^\")\n+\t\topen (my $dd, \"-|\", git_cmd(), \"rev-parse\", '--revs-only', \"$full_rev^\")\n \t\t\tor die_error(undef, \"Open git-rev-parse failed\");\n-\t\tmy $parent_commit = <$dd>;\n+\t\tmy $parent_commit = <$dd> || '';\n \t\tclose $dd;\n \t\tchomp($parent_commit);\n \t\tmy $blamed = href(action => 'blame',\n-- \n1.5.6.rc1\n"},{"id":"78483","messageId":"48452E42.9080305@gmail.com","threadId":"13775","inReplyTo":"1212489977-26822-1-git-send-email-rgarciasuarez@gmail.com","subject":"Re: [PATCH] Avoid errors from git-rev-parse in gitweb blame","fromName":"Lea Wiemann","fromEmail":"lewiemann@gmail.com","sentAt":"2008-06-03T11:42:58Z","receivedAt":"2008-06-03T11:42:58Z","isPatch":true,"sender":{"key":"lewiemann@gmail.com","avatar":null},"body":"Rafael Garcia-Suarez wrote:\n> git-rev-parse will abort with an error when passed a non-existent\n> revision spec, [...]\n>\n> -\t\topen (my $dd, \"-|\", git_cmd(), \"rev-parse\", \"$full_rev^\")\n> +\t\topen (my $dd, \"-|\", git_cmd(), \"rev-parse\", '--revs-only', \"$full_rev^\")\n\nThis is no formal objection, but it would be nice if you could at the \nsame time add a comment to the code that explains this -- like \"do not \nfail [or 'barf on stderr'] if there is no parent revision\". Makes it \neasier to change it later, since \"--revs-only\" is not particularly \nobvious. :)\n\n-- Lea\n"},{"id":"78484","messageId":"m34p8a2173.fsf@localhost.localdomain","threadId":"13775","inReplyTo":"1212489977-26822-1-git-send-email-rgarciasuarez@gmail.com","subject":"Re: [PATCH] Avoid errors from git-rev-parse in gitweb blame","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-06-03T11:43:50Z","receivedAt":"2008-06-03T11:43:50Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Cc-ed Luben Tuikov, author of this part.\n\nRafael Garcia-Suarez writes:\n\n> git-rev-parse will abort with an error when passed a non-existent\n> revision spec, such as \"deadbeef^\" where deadbeef has no parent.\n> Using the --revs-only parameter makes this error go away, while\n> retaining functionality, keeping the web server error log nice\n> and clean.\n\nThanks. This error wasn't detected earlier probably because\n'blame' view is rarely enabled; and repo.or.cz gitweb which has\n'blame' enabled IIRC use gitweb which is modified there, allowing\nincremental blame using AJAX.\n\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 55fb100..f3b4b24 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -4226,9 +4226,9 @@ git_blame2\n> \t\t\t esc_html($rev));\n> \t\t\tprint \"\\n\";\n> \t\t}\n> -\t\topen (my $dd, \"-|\", git_cmd(), \"rev-parse\", \"$full_rev^\")\n> +\t\topen (my $dd, \"-|\", git_cmd(), \"rev-parse\", '--revs-only', \"$full_rev^\")\n> \t\t\tor die_error(undef, \"Open git-rev-parse failed\");\n> -\t\tmy $parent_commit = <$dd>;\n> +\t\tmy $parent_commit = <$dd> || '';\n> \t\tclose $dd;\n> \t\tchomp($parent_commit);\n> \t\tmy $blamed = href(action => 'blame',\n\nI'd rather remove this, correct it, or make it optional (this is very\nfork-heavy).\n\nBut this patch is good as it is now...\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"78486","messageId":"b77c1dce0806030503r55c95d73t5ff244821f76cf1@mail.gmail.com","threadId":"13775","inReplyTo":"m34p8a2173.fsf@localhost.localdomain","subject":"Re: [PATCH] Avoid errors from git-rev-parse in gitweb blame","fromName":"Rafael Garcia-Suarez","fromEmail":"rgarciasuarez@gmail.com","sentAt":"2008-06-03T12:03:47Z","receivedAt":"2008-06-03T12:03:47Z","isPatch":true,"sender":{"key":"rgarciasuarez@gmail.com","avatar":null},"body":"2008/6/3 Jakub Narebski :\n>> - open (my $dd, \"-|\", git_cmd(), \"rev-parse\", \"$full_rev^\")\n>> + open (my $dd, \"-|\", git_cmd(), \"rev-parse\", '--revs-only', \"$full_rev^\")\n>> or die_error(undef, \"Open git-rev-parse failed\");\n>> - my $parent_commit = <$dd>;\n>> + my $parent_commit = <$dd> || '';\n>> close $dd;\n>> chomp($parent_commit);\n>> my $blamed = href(action => 'blame',\n>\n> I'd rather remove this, correct it, or make it optional (this is very\n> fork-heavy).\n\nNot sure how to do the same thing in pure perl.\nWe could however cache the results of git-rev-parse, since the same\nrev is likely to appear many times in the list.\n"},{"id":"78489","messageId":"200806031445.23002.jnareb@gmail.com","threadId":"13775","inReplyTo":"b77c1dce0806030503r55c95d73t5ff244821f76cf1@mail.gmail.com","subject":"Re: [PATCH] Avoid errors from git-rev-parse in gitweb blame","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-06-03T12:45:22Z","receivedAt":"2008-06-03T12:45:22Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Tue, 3 June 2008, Rafael Garcia-Suarez wrote:\n> 2008/6/3 Jakub Narebski :\n>> Rafael Garcia-Suarez wrote:\n>>>\n>>> - open (my $dd, \"-|\", git_cmd(), \"rev-parse\", \"$full_rev^\")\n>>> + open (my $dd, \"-|\", git_cmd(), \"rev-parse\", '--revs-only', \"$full_rev^\")\n>>> or die_error(undef, \"Open git-rev-parse failed\");\n>>> - my $parent_commit = <$dd>;\n>>> + my $parent_commit = <$dd> || '';\n>>> close $dd;\n>>> chomp($parent_commit);\n>>> my $blamed = href(action => 'blame',\n>>\n>> I'd rather remove this, correct it, or make it optional (this is very\n>> fork-heavy).\n> \n> Not sure how to do the same thing in pure Perl.\n\nI was thinking about extending git-blame porcelain format (and also\nincremental format, of course) by 'parents' (and perhaps\n'original-parents') header...\n\n> We could however cache the results of git-rev-parse, since the same\n> rev is likely to appear many times in the list.\n\n...but starting with cache of git-rev-parse results, or optionally\nallowing extended sha-1 syntax (including ^) in hash* CGI\nparameters in gitweb would be a good idea.\n\nBut as I wrote, I'm fine with the patch as it is now.\n-- \nJakub Narebski\nPoland\n"},{"id":"78492","messageId":"b77c1dce0806030600x520d35edxbe6e732ce6cc4ad6@mail.gmail.com","threadId":"13775","inReplyTo":"200806031445.23002.jnareb@gmail.com","subject":"Re: [PATCH] Avoid errors from git-rev-parse in gitweb blame","fromName":"Rafael Garcia-Suarez","fromEmail":"rgarciasuarez@gmail.com","sentAt":"2008-06-03T13:00:20Z","receivedAt":"2008-06-03T13:00:20Z","isPatch":true,"sender":{"key":"rgarciasuarez@gmail.com","avatar":null},"body":"2008/6/3 Jakub Narebski :\n>>> I'd rather remove this, correct it, or make it optional (this is very\n>>> fork-heavy).\n>>\n>> Not sure how to do the same thing in pure Perl.\n>\n> I was thinking about extending git-blame porcelain format (and also\n> incremental format, of course) by 'parents' (and perhaps\n> 'original-parents') header...\n\nOK, I see. That would be nice. Also: currently taking \"$full_rev^\"\ndirects the user to the parent commit, but it would be more\nuser-friendly to point at the previous commit where the selected file\nwas modified instead.\n\n>> We could however cache the results of git-rev-parse, since the same\n>> rev is likely to appear many times in the list.\n>\n> ...but starting with cache of git-rev-parse results, or optionally\n> allowing extended sha-1 syntax (including ^) in hash* CGI\n> parameters in gitweb would be a good idea.\n>\n> But as I wrote, I'm fine with the patch as it is now.\n\nI've sent a new version (take 2) with caching. And comments, as Lea suggested :)\n"},{"id":"78493","messageId":"200806031512.20729.jnareb@gmail.com","threadId":"13775","inReplyTo":"b77c1dce0806030600x520d35edxbe6e732ce6cc4ad6@mail.gmail.com","subject":"Re: [PATCH] Avoid errors from git-rev-parse in gitweb blame","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-06-03T13:12:20Z","receivedAt":"2008-06-03T13:12:20Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Dnia wtorek 3. czerwca 2008 15:00, Rafael Garcia-Suarez napisał:\n> 2008/6/3 Jakub Narebski :\n>> On Tue, 3 June 2008, Rafael Garcia-Suarez wrote:\n\n>> I was thinking about extending git-blame porcelain format (and also\n>> incremental format, of course) by 'parents' (and perhaps\n>> 'original-parents') header...\n> \n> OK, I see. That would be nice. Also: currently taking \"$full_rev^\"\n> directs the user to the parent commit, but it would be more\n> user-friendly to point at the previous commit where the selected file\n> was modified instead.\n\nThat's what I meant by distinguishing between 'parents' and\n'original-parents' (or 'rewritten-parents' and 'parents'): first are\nrewritten parents in history limited to specified file (with the\naddition of code movements and copying across files/filenames),\nsecond are original parents of a commit.\n\nFor gitweb we would use the first set (I wonder what to do in the case\nof merge commit, i.e. more than one parent).\n \n>>> We could however cache the results of git-rev-parse, since the same\n>>> rev is likely to appear many times in the list.\n>>\n>> ...but starting with cache of git-rev-parse results, or optionally\n>> allowing extended sha-1 syntax (including ^) in hash* CGI\n>> parameters in gitweb would be a good idea.\n>>\n>> But as I wrote, I'm fine with the patch as it is now.\n> \n> I've sent a new version (take 2) with caching. And comments, as Lea\n> suggested :) \n \nNice. Thanks a lot.\n\nAck, FWIW.\n-- \nJakub Narebski\nPoland\n"},{"id":"78497","messageId":"b77c1dce0806030636i434e4716r8a52d6aeb93e9719@mail.gmail.com","threadId":"13775","inReplyTo":"200806031512.20729.jnareb@gmail.com","subject":"Re: [PATCH] Avoid errors from git-rev-parse in gitweb blame","fromName":"Rafael Garcia-Suarez","fromEmail":"rgarciasuarez@gmail.com","sentAt":"2008-06-03T13:36:26Z","receivedAt":"2008-06-03T13:36:26Z","isPatch":true,"sender":{"key":"rgarciasuarez@gmail.com","avatar":null},"body":"2008/6/3 Jakub Narebski :\n>>\n>> OK, I see. That would be nice. Also: currently taking \"$full_rev^\"\n>> directs the user to the parent commit, but it would be more\n>> user-friendly to point at the previous commit where the selected file\n>> was modified instead.\n>\n> That's what I meant by distinguishing between 'parents' and\n> 'original-parents' (or 'rewritten-parents' and 'parents'): first are\n> rewritten parents in history limited to specified file (with the\n> addition of code movements and copying across files/filenames),\n> second are original parents of a commit.\n>\n> For gitweb we would use the first set (I wonder what to do in the case\n> of merge commit, i.e. more than one parent).\n\nCurrently that takes the left parent. Or something.\n\nShameless plug : the sources for perl 5 are currently being kept in a\nperforce repository. There is a rough web interface to it at\nhttp://public.activestate.com/cgi-bin/perlbrowse with excellent blame\nlog navigation features (including navigation against p4\nintegrations).\n\nSince we're going to move the official perl 5 vcs to git (many many\nthanks to Sam Vilain for that, BTW), I'm more or less trying to\nduplicate this blame log navigation in gitweb. So it might result in a\nfew patches here :)\n"},{"id":"78503","messageId":"200806031614.29161.jnareb@gmail.com","threadId":"13775","inReplyTo":"b77c1dce0806030636i434e4716r8a52d6aeb93e9719@mail.gmail.com","subject":"Re: [PATCH] Avoid errors from git-rev-parse in gitweb blame","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-06-03T14:14:28Z","receivedAt":"2008-06-03T14:14:28Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Tue, 3 June 2008, Rafael Garcia-Suarez wrote:\n> 2008/6/3 Jakub Narebski :\n>>>\n>>> OK, I see. That would be nice. Also: currently taking \"$full_rev^\"\n>>> directs the user to the parent commit, but it would be more\n>>> user-friendly to point at the previous commit where the selected file\n>>> was modified instead.\n>>\n>> That's what I meant by distinguishing between 'parents' and\n>> 'original-parents' (or 'rewritten-parents' and 'parents'): first are\n>> rewritten parents in history limited to specified file (with the\n>> addition of code movements and copying across files/filenames),\n>> second are original parents of a commit.\n>>\n>> For gitweb we would use the first set (I wonder what to do in the case\n>> of merge commit, i.e. more than one parent).\n> \n> Currently that takes the left parent. Or something.\n> \n> Shameless plug : the sources for perl 5 are currently being kept in a\n> perforce repository. There is a rough web interface to it at\n> http://public.activestate.com/cgi-bin/perlbrowse with excellent blame\n> log navigation features (including navigation against p4\n> integrations).\n\nBy the way, what is the difference between '<<' links and 'br' link\nin the above mentioned annotate/blame interface?\n\nI'd like to say that I prefer gitweb's marking blame by blocks, not by\nlines, and extra info on mouseover. But having blame navigation\ncapability of perforce web interface would be really nice (I think\n\"git gui blame\" has something like this; I don't know about other\ntools like qgit, giggle, or ugit).\n\n> Since we're going to move the official perl 5 vcs to git (many many\n> thanks to Sam Vilain for that, BTW),\n\nBTW. how in your opinion Git compares to Perforce, both as a tool\nitself, and also about quality of companion tools such like gitweb\nor git-gui?\n\n> I'm more or less trying to \n> duplicate this blame log navigation in gitweb. So it might result in a\n> few patches here :)\n\nI think it would be really nice. Will you want to use git-diff-tree\nto mark differences from the version we came from (marked by 'hp',\n'hpb' and 'fp' URI parameters), or would you rather extend git-blame?\n\n-- \nJakub Narebski\nPoland\n"},{"id":"78506","messageId":"48455433.8080500@gmail.com","threadId":"13775","inReplyTo":"200806031445.23002.jnareb@gmail.com","subject":"Re: [PATCH] Avoid errors from git-rev-parse in gitweb blame","fromName":"Lea Wiemann","fromEmail":"lewiemann@gmail.com","sentAt":"2008-06-03T14:24:51Z","receivedAt":"2008-06-03T14:24:51Z","isPatch":true,"sender":{"key":"lewiemann@gmail.com","avatar":null},"body":"Jakub Narebski wrote:\n> I was thinking about extending git-blame porcelain format (and also\n> incremental format, of course) by 'parents' (and perhaps\n> 'original-parents') header...\n\nRegarding prettiness, I don't find parents in the porcelain output \nparticularly useful, but if other people think they need this, I won't \nobject. :)\n\nRegarding performance, it would be good to show that the solution I'm \nsuggesting in my separate is slower than extending git-blame before \nimplementing anything. (I doubt it matters performance-wise.)\n\n-- Lea\n"},{"id":"78508","messageId":"b77c1dce0806030740td820c52ve45619812313776c@mail.gmail.com","threadId":"13775","inReplyTo":"200806031614.29161.jnareb@gmail.com","subject":"Re: [PATCH] Avoid errors from git-rev-parse in gitweb blame","fromName":"Rafael Garcia-Suarez","fromEmail":"rgarciasuarez@gmail.com","sentAt":"2008-06-03T14:40:44Z","receivedAt":"2008-06-03T14:40:44Z","isPatch":true,"sender":{"key":"rgarciasuarez@gmail.com","avatar":null},"body":"2008/6/3 Jakub Narebski :\n>> Shameless plug : the sources for perl 5 are currently being kept in a\n>> perforce repository. There is a rough web interface to it at\n>> http://public.activestate.com/cgi-bin/perlbrowse with excellent blame\n>> log navigation features (including navigation against p4\n>> integrations).\n>\n> By the way, what is the difference between '<<' links and 'br' link\n> in the above mentioned annotate/blame interface?\n\n\"br\" navigates to another branch from which this file has been\nintegrated (in p4 speak.)\n\n> I'd like to say that I prefer gitweb's marking blame by blocks, not by\n> lines, and extra info on mouseover. But having blame navigation\n> capability of perforce web interface would be really nice (I think\n> \"git gui blame\" has something like this; I don't know about other\n> tools like qgit, giggle, or ugit).\n>\n>> Since we're going to move the official perl 5 vcs to git (many many\n>> thanks to Sam Vilain for that, BTW),\n>\n> BTW. how in your opinion Git compares to Perforce, both as a tool\n> itself, and also about quality of companion tools such like gitweb\n> or git-gui?\n\nI'm not using companion tools much, but I'm really impatient to switch to git.\n(I'm often working offline and applying patches from mailboxes. That\nalready makes two good reasons for switching:)\n\n> I think it would be really nice. Will you want to use git-diff-tree\n> to mark differences from the version we came from (marked by 'hp',\n> 'hpb' and 'fp' URI parameters), or would you rather extend git-blame?\n\nI don't know. I'll look at git-diff-tree.\n"},{"id":"78512","messageId":"200806031656.04780.jnareb@gmail.com","threadId":"13775","inReplyTo":"b77c1dce0806030740td820c52ve45619812313776c@mail.gmail.com","subject":"Re: [PATCH] Avoid errors from git-rev-parse in gitweb blame","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-06-03T14:56:04Z","receivedAt":"2008-06-03T14:56:04Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Rafael Garcia-Suarez wrote:\n> 2008/6/3 Jakub Narebski :\n>>> Shameless plug : the sources for perl 5 are currently being kept in a\n>>> perforce repository. There is a rough web interface to it at\n>>> http://public.activestate.com/cgi-bin/perlbrowse with excellent blame\n>>> log navigation features (including navigation against p4\n>>> integrations).\n>>\n>> By the way, what is the difference between '<<' links and 'br' link\n>> in the above mentioned annotate/blame interface?\n> \n> \"br\" navigates to another branch from which this file has been\n> integrated (in p4 speak.)\n\nDoes it mark merge commits then? Or perhaps branch points? What\ndoes \"branch from which this file has been integrated\" mean in git\nspeak (in the terms of DAG of commits)?\n\n\nIf the history of a file looks like this\n\n ....*---*---A---M---C...\n /\n ....*---B-/ \n\nand the line comes from \"evil merge\" M git-blame would return M as\nblamed commit. If the line comes from one or the other branch, from\ncommit A or B, it makes I think no difference to git-blame; git tries\nto be \"branch agnostic\" (no special meaning to first parent; well,\nbesides rev~n notation and --first-parent walk option). I guess it\nis not the case in Perforce?\n\n[...]\n>> [...]. Will you want to use git-diff-tree\n>> to mark differences from the version we came from (marked by 'hp',\n>> 'hpb' and 'fp' URI parameters), or would you rather extend git-blame?\n> \n> I don't know. I'll look at git-diff-tree.\n\nWhat I meant here, would you plan on extending git-blame, or would you\nuse patchset (textual) diff between revision we are at, and revision we\ncame from. git-diff-tree just compares two trees (and have to have\npatch output explicitely enabled). Sorry for the confusion.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"78514","messageId":"b77c1dce0806030807t7654ac2cm96aa06690c7a5c02@mail.gmail.com","threadId":"13775","inReplyTo":"200806031656.04780.jnareb@gmail.com","subject":"Re: [PATCH] Avoid errors from git-rev-parse in gitweb blame","fromName":"Rafael Garcia-Suarez","fromEmail":"rgarciasuarez@gmail.com","sentAt":"2008-06-03T15:07:23Z","receivedAt":"2008-06-03T15:07:23Z","isPatch":true,"sender":{"key":"rgarciasuarez@gmail.com","avatar":null},"body":"2008/6/3 Jakub Narebski :\n>>> By the way, what is the difference between '<<' links and 'br' link\n>>> in the above mentioned annotate/blame interface?\n>>\n>> \"br\" navigates to another branch from which this file has been\n>> integrated (in p4 speak.)\n>\n> Does it mark merge commits then? Or perhaps branch points? What\n> does \"branch from which this file has been integrated\" mean in git\n> speak (in the terms of DAG of commits)?\n>\n>\n> If the history of a file looks like this\n>\n> ....*---*---A---M---C...\n> /\n> ....*---B-/\n>\n> and the line comes from \"evil merge\" M git-blame would return M as\n> blamed commit. If the line comes from one or the other branch, from\n> commit A or B, it makes I think no difference to git-blame; git tries\n> to be \"branch agnostic\" (no special meaning to first parent; well,\n> besides rev~n notation and --first-parent walk option). I guess it\n> is not the case in Perforce?\n\nNo, in perforce the branch you integrate changes from is always explicit.\n\n> [...]\n>>> [...]. Will you want to use git-diff-tree\n>>> to mark differences from the version we came from (marked by 'hp',\n>>> 'hpb' and 'fp' URI parameters), or would you rather extend git-blame?\n>>\n>> I don't know. I'll look at git-diff-tree.\n>\n> What I meant here, would you plan on extending git-blame, or would you\n> use patchset (textual) diff between revision we are at, and revision we\n> came from. git-diff-tree just compares two trees (and have to have\n> patch output explicitely enabled). Sorry for the confusion.\n\nI'm under the impression that extending git-blame is a more flexible solution.\n"},{"id":"78523","messageId":"200806031950.39602.jnareb@gmail.com","threadId":"13775","inReplyTo":"b77c1dce0806030807t7654ac2cm96aa06690c7a5c02@mail.gmail.com","subject":"Re: [PATCH] Avoid errors from git-rev-parse in gitweb blame","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-06-03T17:50:38Z","receivedAt":"2008-06-03T17:50:38Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Tue, 3 Jun 2008, Rafael Garcia-Suarez wrote:\n> 2008/6/3 Jakub Narebski :\n\nBy the way, could you please try to not remove all but last quote \nattributions? It should be, I think, as simple as replying then \nremoving unnecessary parts, instead of selecting parts you want reply \nto and then hitting reply. TIA\n\n>>>> By the way, what is the difference between '<<' links and 'br' link\n>>>> in the above mentioned annotate/blame interface?\n>>>\n>>> \"br\" navigates to another branch from which this file has been\n>>> integrated (in p4 speak.)\n>>\n>> Does it mark merge commits then? Or perhaps branch points? What\n>> does \"branch from which this file has been integrated\" mean in git\n>> speak (in the terms of DAG of commits)?\n>>\n>>\n>> If the history of a file looks like this\n>>\n>> ....*---*---A---M---C...\n>> /\n>> ....*---B-/\n>>\n>> and the line comes from \"evil merge\" M git-blame would return M as\n>> blamed commit. If the line comes from one or the other branch, from\n>> commit A or B, it makes I think no difference to git-blame; git tries\n>> to be \"branch agnostic\" (no special meaning to first parent; well,\n>> besides rev~n notation and --first-parent walk option). I guess it\n>> is not the case in Perforce?\n> \n> No, in perforce the branch you integrate changes from is always\n> explicit. \n\nSo, in git speak, it means that 'br' means that blamed commit (commit \nwhich brought current version of given line) is not in first-parent \nline, and '<<' means that commit is in --first-parent history of a file \n(taking into account code copying and movement... err, at least in git \ncase...), doesn't it?\n \n>> [...]\n>>>> [...]. Will you want to use git-diff-tree\n>>>> to mark differences from the version we came from (marked by 'hp',\n>>>> 'hpb' and 'fp' URI parameters), or would you rather extend\n>>>> git-blame? \n>>>\n>>> I don't know. I'll look at git-diff-tree.\n>>\n>> What I meant here, would you plan on extending git-blame, or would\n>> you use patchset (textual) diff between revision we are at, and\n>> revision we came from. git-diff-tree just compares two trees (and\n>> have to have patch output explicitely enabled). Sorry for the\n>> confusion. \n> \n> I'm under the impression that extending git-blame is a more flexible\n> solution. \n\nI don't think that it is correct solution in this case. I'm not sure if \nit can even be done. \n\nWhat you have (what \"annotated file view\" in Perforce web interface has) \nis difference annotations (one sided side-bys side diff ;-)), something \nlike Eclipse QuickDiff, or like word-diff (or \"git diff --color-words\")\nput _ON TOP_ of blame info. \n\nGenerating such single pane in-file diff is orthogonal to generating \nblame info. I think it would be best solved using patchset (textual) \ndiff output; if git-diff would support \"context\" and not only \"unified\" \npatch output it could be used there.\n\n\nWhat was I thinking when mentioning extending git-blame was \"reblame\", \ni.e. blaming only those lines which are different from some child \nversion. But while this would be very useful for tools such like \n\"git gui blame\" or blameview, it just won't work well I guess for web \napplication (unless caching everything, and generating blame diff from \ncached blame).\n\n\nAs to extending git-blame --porcelain to output \"parents ...\" \nheader, it is better solution than \"git rev-list --no-walk\" used as a \nkind of git-rev-parse sequencer not only because it is one fork less \n(and blame has has this parent info anyway), but mainly that it better \nfits with the streaming flow of gitweb's git_blame2(). (I'll write \nabout it more in separate letter).\n\n-- \nJakub Narebski\nPoland\n"},{"id":"78543","messageId":"839911.60903.qm@web31810.mail.mud.yahoo.com","threadId":"13775","inReplyTo":"m34p8a2173.fsf@localhost.localdomain","subject":"Re: [PATCH] Avoid errors from git-rev-parse in gitweb blame","fromName":"Luben Tuikov","fromEmail":"ltuikov@yahoo.com","sentAt":"2008-06-03T20:18:49Z","receivedAt":"2008-06-03T20:18:49Z","isPatch":true,"sender":{"key":"ltuikov@yahoo.com","avatar":null},"body":"--- On Tue, 6/3/08, Jakub Narebski wrote:\n> From: Jakub Narebski \n> Subject: Re: [PATCH] Avoid errors from git-rev-parse in gitweb blame\n> To: \"Rafael Garcia-Suarez\" \n> Cc: git@vger.kernel.org, \"Luben Tuikov\" \n> Date: Tuesday, June 3, 2008, 4:43 AM\n> Cc-ed Luben Tuikov, author of this part.\n\nThanks guys! :-)\n\n> Rafael Garcia-Suarez \n> writes:\n> \n> > git-rev-parse will abort with an error when passed a\n> non-existent\n> > revision spec, such as \"deadbeef^\" where\n> deadbeef has no parent.\n\nYes, I've known about this ever since I coded this.\nThe reasoning was that the value of parsing up the \"tree\" of\nparent changes was a lot more (valuable) than the value of detecting that\n\"deadbeef^\" had no parent -- which would've been logically apparent\nto the coder/reviewer/user of \"blame2\".\n\n> > Using the --revs-only parameter makes this error go\n> away, while\n> > retaining functionality, keeping the web server error\n> log nice\n> > and clean.\n\nOk, that's fine, as long as indeed the functionality is preserved.\nI leave it up to Jakub and Junio to make sure that indeed the\nfunctionality is preserved. (I.e. saving us yet another patch\nfrom anyone of us.)\n\n> Thanks. This error wasn't detected earlier probably\n> because\n> 'blame' view is rarely enabled; and repo.or.cz\n> gitweb which has\n> 'blame' enabled IIRC use gitweb which is modified\n> there, allowing\n> incremental blame using AJAX.\n\nI'm a heavy user of \"blame2\", often exploring the course of\n\"evolution\" of the code and/or code lines and segments. I value this\nin gitweb (being easier/more visual to use as opposed to command\nline).\n\n> \n> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> > index 55fb100..f3b4b24 100755\n> > --- a/gitweb/gitweb.perl\n> > +++ b/gitweb/gitweb.perl\n> > @@ -4226,9 +4226,9 @@ git_blame2\n> > \t\t\t esc_html($rev));\n> > \t\t\tprint \"\\n\";\n> > \t\t}\n> > -\t\topen (my $dd, \"-|\", git_cmd(),\n> \"rev-parse\", \"$full_rev^\")\n> > +\t\topen (my $dd, \"-|\", git_cmd(),\n> \"rev-parse\", '--revs-only',\n> \"$full_rev^\")\n> > \t\t\tor die_error(undef, \"Open git-rev-parse\n> failed\");\n> > -\t\tmy $parent_commit = <$dd>;\n> > +\t\tmy $parent_commit = <$dd> || '';\n> > \t\tclose $dd;\n> > \t\tchomp($parent_commit);\n> > \t\tmy $blamed = href(action => 'blame',\n> \n> I'd rather remove this, correct it, or make it optional\n> (this is very\n> fork-heavy).\n> \n> But this patch is good as it is now...\n\nYes, I agree it is good. Just you guys make sure that it doesn't change\nthe value of the functionality of the code.\n\nThanks everyone!\n Luben\n"},{"id":"78541","messageId":"200806032224.08714.jnareb@gmail.com","threadId":"13775","inReplyTo":"48455433.8080500@gmail.com","subject":"Re: [PATCH] Avoid errors from git-rev-parse in gitweb blame","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-06-03T20:24:07Z","receivedAt":"2008-06-03T20:24:07Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"I have joined there two separate threads... probably attaching them\nin a wrong place.\n\nOn Tue, 3 June 2008, Lea Wiemann wrote:\n> Rafael Garcia-Suarez wrote:\n> >\n> > Finally, to avoid forking git-rev-parse too many times, cache its\n> > results in a new hash %parent_commits.\n>\n> I'm not too happy with this:\n>\n> 1) Minor point: I'm working on caching for the backend right now\n> (IOW, basically what you're doing, just centralized in a separate\n> module), so you're essentially duplicating work, and you're making it\n> (a little) harder for me to refactor gitweb since I have to rip out\n> your cache code.\n\nI don't think %parent_commits hash is suitable for caching; it is only\nintermediate step, reducing number of git command calls (and forks)\nfrom number of blocks of changes in a blame, to number of distinct \ncommits blamed. From this you would put info into appropriate Perl \nstructure.\n\n\nATTENTION! This example shows where caching [parsed] data have problems \ncompared to front-end caching (caching output). Caching data is \n(usually) the best solution for pages which are generated from some \nparsed data _as a whole_, or can be generated from parsed data as a \nwhole, i.e. heads, tags, summary, projects list, shortlog, history, \nview etc.\n\nProblems occur when we try to cache page with _streaming_ output, such \nas blob view, blame view, diff part of commitdiff etc. Here better \nsolution might be either front-end cache (caching HTML output), or \nback-end caching (caching output of git commands).\n\n> Those few lines won't hurt, but in general I suggest that nobody\n> make any larger efforts to cache stuff in gitweb for the next few\n> weeks. \n\nUnderstandable, we want to avoid conflicts.\n\nBy the way, if we agree that version %parent_commits is too intrusive \ndusring GSoC 2008, I think it would be good to accept into maint the \npatch with --revs-only, which fixes real bug, even if it is annoyance \nlevel only...\n\n> 2) Major point: You're still forking a lot.  The Right Thing is to\n> condense everything into a single call -- I believe \"git-rev-list\n> --parents --no-walk hash hash hash...\" is correct and easily\n> parsable. Its output seems to be lines of\n>      hash parent_1 parent_2 ... parent_n\n> with n >= 0.  Can you implement that?  It would be very useful and\n> also reusable for me!\n\nThis is not a good solution for 'blame' view, which is generated \"on the \nfly\", by streaming git-blame output via filter. Above solution goes \ncounter to code flow flow: gitweb would have to somehow get list of all \nblamed commits. (See also note above about caching \"stream-generated\" \npages).\n\nModifying git-blame --porcelain (and --incremental) output has the \nadvantage of simple code on gitweb side, retaining \"streamed\" page \ngeneration. It would be one fork less, but I guess that is negligible. \nIt would be useful for other blame viewers such as \"git gui blame\" to \ndo similar data mining fast. Other consumers of git-blame output \nshould be (if written correctly) not affected by additional header \nwhich they don't understand. \n\nThe disadvantage would be for gitweb to require version of git binary \nwhich has this feature...\n\n\nOf course implementing get_parents($hash[, $hash...]) in either gitweb, \nor Git.pm, using \"git-rev-list --parents --no-walk \" could still \nbe useful.\n\n\n----------------------------------------------------------------------\nOn Tue, 3 June 2008, Lea Wiemann wrote:\n> Jakub Narebski wrote:\n> > I was thinking about extending git-blame porcelain format (and also\n> > incremental format, of course) by 'parents' (and perhaps\n> > 'original-parents') header...\n> \n> Regarding prettiness, I don't find parents in the porcelain output \n> particularly useful, but if other people think they need this, I won't \n> object. :)\n\nThe change in gitweb was introduced by commit 244a70e (Blame \"linenr\" \nlink jumps to previous state at \"orig_lineno\"), by Luben Tuikov; please \nread commit message for explanation how it could be used for data \nmining / browsing annotated history of a file.\n\nAs I wrote above, this solution allows to have very simple, streaming \ncode in gitweb dealing with \"line before change\" links.\n\nThe porcelain (and incremental) format of git-blame was created in such \nway to allow easy extending it; true and rewritten parents would help \nin navigating annotated file view in history.\n\nIt should not, I think, affect your efforts; it is not you that proposed \nto write such extension.\n\n> Regarding performance, it would be good to show that the solution I'm \n> suggesting in my separate is slower than extending git-blame before \n> implementing anything. (I doubt it matters performance-wise.)\n\nIt is one fork more. And as I wrote above, you need list of all blamed \ncommits upfront, which goes counter to currently used \"streaming \noutput\" code flow; it would complicate code, and thus reduce \nperformance.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"78545","messageId":"200806032229.40447.jnareb@gmail.com","threadId":"13775","inReplyTo":"839911.60903.qm@web31810.mail.mud.yahoo.com","subject":"Re: [PATCH] Avoid errors from git-rev-parse in gitweb blame","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-06-03T20:29:40Z","receivedAt":"2008-06-03T20:29:40Z","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> > Rafael Garcia-Suarez wrote\n\n> > > +           my $parent_commit = <$dd> || '';\n\nBy the way, here you would probably want \n\n+ my $parent_commit = <$dd> || '--root';\n\n(if it works).\n-- \nJakub Narebski\nPoland\n"},{"id":"78547","messageId":"940824.46903.qm@web31808.mail.mud.yahoo.com","threadId":"13775","inReplyTo":"b77c1dce0806030600x520d35edxbe6e732ce6cc4ad6@mail.gmail.com","subject":"Re: [PATCH] Avoid errors from git-rev-parse in gitweb blame","fromName":"Luben Tuikov","fromEmail":"ltuikov@yahoo.com","sentAt":"2008-06-03T20:35:31Z","receivedAt":"2008-06-03T20:35:31Z","isPatch":true,"sender":{"key":"ltuikov@yahoo.com","avatar":null},"body":"--- On Tue, 6/3/08, Rafael Garcia-Suarez wrote:\n> From: Rafael Garcia-Suarez \n> Subject: Re: [PATCH] Avoid errors from git-rev-parse in gitweb blame\n> To: \"Jakub Narebski\" \n> Cc: git@vger.kernel.org, \"Luben Tuikov\" \n> Date: Tuesday, June 3, 2008, 6:00 AM\n> 2008/6/3 Jakub Narebski :\n> >>> I'd rather remove this, correct it, or\n> make it optional (this is very\n> >>> fork-heavy).\n> >>\n> >> Not sure how to do the same thing in pure Perl.\n> >\n> > I was thinking about extending git-blame porcelain\n> format (and also\n> > incremental format, of course) by 'parents'\n> (and perhaps\n> > 'original-parents') header...\n> \n> OK, I see. That would be nice. Also: currently taking\n> \"$full_rev^\"\n> directs the user to the parent commit, but it would be more\n> user-friendly to point at the previous commit where the\n> selected file\n> was modified instead.\n\nThe intention was that it shouldn't necessarily be the (strict) parent\nof the change (changed segment), since it may or may not have changed\nin the strict parent commit. The intention was that it \"starts\"/\"opens\"\nthe parent commit so that \"git\" would start from there and find the actual\nchange/commit where that line/segment has changed. And it has worked\npretty fine for me when data-mining (something I do quite often) code\nevolution.\n\nMy commit 244a70e608204a515c214a11c43f3ecf7642533a was really derived\nfrom a command line, which I had started to use quite often and had\nbeen \"looking for\" for quite some time.\n\n> >> We could however cache the results of\n> git-rev-parse, since the same\n> >> rev is likely to appear many times in the list.\n> >\n> > ...but starting with cache of git-rev-parse results,\n> or optionally\n> > allowing extended sha-1 syntax (including\n> ^) in hash* CGI\n> > parameters in gitweb would be a good idea.\n> >\n> > But as I wrote, I'm fine with the patch as it is\n> now.\n> \n> I've sent a new version (take 2) with caching. And\n> comments, as Lea suggested :)\n\nYes, hashing is good if it speeds up lookups without altering\nintended functionality.\n\nThanks everyone!\n Luben\n"},{"id":"78551","messageId":"354228.30165.qm@web31807.mail.mud.yahoo.com","threadId":"13775","inReplyTo":"200806031614.29161.jnareb@gmail.com","subject":"Re: [PATCH] Avoid errors from git-rev-parse in gitweb blame","fromName":"Luben Tuikov","fromEmail":"ltuikov@yahoo.com","sentAt":"2008-06-03T21:03:52Z","receivedAt":"2008-06-03T21:03:52Z","isPatch":true,"sender":{"key":"ltuikov@yahoo.com","avatar":null},"body":"--- On Tue, 6/3/08, Jakub Narebski wrote:\n> From: Jakub Narebski \n> Subject: Re: [PATCH] Avoid errors from git-rev-parse in gitweb blame\n> To: \"Rafael Garcia-Suarez\" \n> Cc: git@vger.kernel.org, \"Luben Tuikov\" , \"Sam Vilain\" \n> Date: Tuesday, June 3, 2008, 7:14 AM\n> On Tue, 3 June 2008, Rafael Garcia-Suarez wrote:\n> > 2008/6/3 Jakub Narebski :\n> >>>\n> >>> OK, I see. That would be nice. Also: currently\n> taking \"$full_rev^\"\n> >>> directs the user to the parent commit, but it\n> would be more\n> >>> user-friendly to point at the previous commit\n> where the selected file\n> >>> was modified instead.\n> >>\n> >> That's what I meant by distinguishing between\n> 'parents' and\n> >> 'original-parents' (or\n> 'rewritten-parents' and 'parents'): first\n> are\n> >> rewritten parents in history limited to specified\n> file (with the\n> >> addition of code movements and copying across\n> files/filenames),\n> >> second are original parents of a commit.\n> >>\n> >> For gitweb we would use the first set (I wonder\n> what to do in the case\n> >> of merge commit, i.e. more than one parent).\n> > \n> > Currently that takes the left parent. Or something.\n> > \n> > Shameless plug : the sources for perl 5 are currently\n> being kept in a\n> > perforce repository. There is a rough web interface to\n> it at\n> > http://public.activestate.com/cgi-bin/perlbrowse with\n> excellent blame\n> > log navigation features (including navigation against\n> p4\n> > integrations).\n> \n> By the way, what is the difference between\n> '<<' links and 'br' link\n> in the above mentioned annotate/blame interface?\n> \n> I'd like to say that I prefer gitweb's marking\n> blame by blocks, not by\n> lines, and extra info on mouseover.\n\nCompletely agree.\n\n> But having blame\n> navigation\n> capability of perforce web interface would be really nice\n> (I think\n> \"git gui blame\" has something like this; I\n> don't know about other\n> tools like qgit, giggle, or ugit).\n\nThat was the intention of git_blame2()... myself just previously coming\nfrom perforce...\n\nSo yes, long time ago, in a galaxy far, far away, I was using perforce for\ndata mining of the evolution of the code, in order to deduct intention.\nAnd I had wanted the same capability in git, thus git_blame2 and commit\n244a70e608204a515c214a11c43f3ecf7642533a.\n\n> BTW. how in your opinion Git compares to Perforce, both as\n> a tool\n> itself, and also about quality of companion tools such like\n> gitweb\n> or git-gui?\n\nWhat I did was prompted by what I had used with perforce, and on top\nof it, improving on it.\n\nSo yes, I like git and gitweb. At the moment they give me what I need,\nand of course an edge over other SCMs is the distributed nature of git.\n\nThanks!\n Luben\n\n\n> \n> > I'm more or\n> less trying to \n> > duplicate this blame log navigation in gitweb. So it\n> might result in a\n> > few patches here :)\n> \n> I think it would be really nice. Will you want to use\n> git-diff-tree\n> to mark differences from the version we came from (marked\n> by 'hp',\n> 'hpb' and 'fp' URI parameters), or would\n> you rather extend git-blame?\n> \n> -- \n> Jakub Narebski\n> Poland\n"},{"id":"78552","messageId":"655725.88869.qm@web31813.mail.mud.yahoo.com","threadId":"13775","inReplyTo":"200806031656.04780.jnareb@gmail.com","subject":"Re: [PATCH] Avoid errors from git-rev-parse in gitweb blame","fromName":"Luben Tuikov","fromEmail":"ltuikov@yahoo.com","sentAt":"2008-06-03T21:09:53Z","receivedAt":"2008-06-03T21:09:53Z","isPatch":true,"sender":{"key":"ltuikov@yahoo.com","avatar":null},"body":"--- On Tue, 6/3/08, Jakub Narebski wrote:\n> From: Jakub Narebski \n> Subject: Re: [PATCH] Avoid errors from git-rev-parse in gitweb blame\n> To: \"Rafael Garcia-Suarez\" \n> Cc: git@vger.kernel.org, \"Luben Tuikov\" , \"Sam Vilain\" \n> Date: Tuesday, June 3, 2008, 7:56 AM\n> Rafael Garcia-Suarez wrote:\n> > 2008/6/3 Jakub Narebski :\n> >>> Shameless plug : the sources for perl 5 are\n> currently being kept in a\n> >>> perforce repository. There is a rough web\n> interface to it at\n> >>>\n> http://public.activestate.com/cgi-bin/perlbrowse with\n> excellent blame\n> >>> log navigation features (including navigation\n> against p4\n> >>> integrations).\n> >>\n> >> By the way, what is the difference between\n> '<<' links and 'br' link\n> >> in the above mentioned annotate/blame interface?\n> > \n> > \"br\" navigates to another branch from which\n> this file has been\n> > integrated (in p4 speak.)\n> \n> Does it mark merge commits then? Or perhaps branch points? \n> What\n> does \"branch from which this file has been\n> integrated\" mean in git\n> speak (in the terms of DAG of commits)?\n> \n> \n> If the history of a file looks like this\n> \n> ....*---*---A---M---C...\n> /\n> ....*---B-/ \n> \n> and the line comes from \"evil merge\" M git-blame\n> would return M as\n> blamed commit. If the line comes from one or the other\n> branch, from\n> commit A or B, it makes I think no difference to git-blame;\n> git tries\n> to be \"branch agnostic\" (no special meaning to\n> first parent; well,\n> besides rev~n notation and --first-parent walk option). I\n> guess it\n> is not the case in Perforce?\n\nThe whole point of git_blame2() was to show which \"previous\" commit\nchanged that line/segment and what the commit message was. This is\nimportant to me when data-mining code evolution, since often enough\nI'd like to recollect mine/other's intentions at the time of\nchanging/adding the code/commit in question.\n\n> \n> [...]\n> >> [...]. Will you want to use git-diff-tree\n> >> to mark differences from the version we came from\n> (marked by 'hp',\n> >> 'hpb' and 'fp' URI parameters), or\n> would you rather extend git-blame?\n> > \n> > I don't know. I'll look at git-diff-tree.\n> \n> What I meant here, would you plan on extending git-blame,\n> or would you\n> use patchset (textual) diff between revision we are at, and\n> revision we\n> came from. git-diff-tree just compares two trees (and have\n> to have\n> patch output explicitely enabled). Sorry for the\n> confusion.\n\nI'd rather stray away from \"git-diff-xyz\", since there is no context.\nContext is found in the commit message which changed the line/segment.\n(By \"context\" I mean the human intention, most likely encoded in the\ncommit message.)\n\nThanks everyone!\n Luben\n"},{"id":"78554","messageId":"412634.67173.qm@web31803.mail.mud.yahoo.com","threadId":"13775","inReplyTo":"200806032229.40447.jnareb@gmail.com","subject":"Re: [PATCH] Avoid errors from git-rev-parse in gitweb blame","fromName":"Luben Tuikov","fromEmail":"ltuikov@yahoo.com","sentAt":"2008-06-03T21:27:48Z","receivedAt":"2008-06-03T21:27:48Z","isPatch":true,"sender":{"key":"ltuikov@yahoo.com","avatar":null},"body":"--- On Tue, 6/3/08, Jakub Narebski wrote:\n> > > > + my $parent_commit =\n> <$dd> || '';\n> \n> By the way, here you would probably want \n> \n> + my $parent_commit = <$dd> ||\n> '--root';\n> \n> (if it works).\n\nAFAIR, I did experiment with the \"--root\" parameter. Not sure why,\nbut my command line ended up without it, and thus git_blame2()\nended up without it.\n\nIf you feel that it is, at this time having had changes commited since my\noriginal commit, more correct to include it, then please go ahead.\n\nThanks!\n Luben\n"},{"id":"78555","messageId":"200806032331.44514.jnareb@gmail.com","threadId":"13775","inReplyTo":"940824.46903.qm@web31808.mail.mud.yahoo.com","subject":"Re: [PATCH] Avoid errors from git-rev-parse in gitweb blame","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-06-03T21:31:43Z","receivedAt":"2008-06-03T21:31:43Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Tue, 3 Jan 2008, Luben Tuikov wrote:\n> On Tue, 6/3/08, Rafael Garcia-Suarez wrote:\n>> 2008/6/3 Jakub Narebski wrote:\n>>>\n>>> I was thinking about extending git-blame porcelain format (and also\n>>> incremental format, of course) by 'parents' (and perhaps\n>>> 'original-parents') header...\n>> \n>> OK, I see. That would be nice. Also: currently taking \"$full_rev^\"\n>> directs the user to the parent commit, but it would be more\n>> user-friendly to point at the previous commit where the selected file\n>> was modified instead.\n> \n> The intention was that it shouldn't necessarily be the (strict) parent\n> of the change (changed segment), since it may or may not have changed\n> in the strict parent commit. The intention was that it\n> \"starts\"/\"opens\" the parent commit so that \"git\" would start from\n> there and find the actual change/commit where that line/segment has\n> changed. And it has worked pretty fine for me when data-mining\n> (something I do quite often) code evolution.\n> \n> My commit 244a70e608204a515c214a11c43f3ecf7642533a was really derived\n> from a command line, which I had started to use quite often and had\n> been \"looking for\" for quite some time.\n\nLet us assume for a bit that history is linear, and looks like this:\n\n ...*---A---*---*---b---.---D^---D---*---x\n\nwhere 'x' is starting point (revision we start running git-blame from),\n'D' is revision given line is blamed on, 'D^' is parent of revision 'D',\n'b' is previous commit in a given file history, and 'A' is previous \ncommit which modifies given line of a given commit.\n\nIt means that the history looks like below\n $ git rev-list x\n [...]\n D\n D^\n .\n b\n [...]\nwhile history of a given file looks like this\n $ git rev-list x -- file\n [...]\n D\n b\n [...]\n\nNow for all commits in the b..D^ range (between D^ and b, including \nendpoints), given file has the same contents, and therefore 'blame' \nview would also look the same. That is why it works.\n\n\nThe only problem I can see when blamed commit is merge commit; D^ means \nD^1, ehich mesna first parent. Now, I think that merge commit might be \nblamed _only_ if it was \"evil merge\" (change/line didn't came from any \nof parents). But this is quite rare situation; additionally the bug is \nnot very visible; when clicking on link you would go to not correct \nview, but this not-correctness isn't obvious on the first glance.\n-- \nJakub Narebski\nPoland\n"},{"id":"78556","messageId":"200806032334.29769.jnareb@gmail.com","threadId":"13775","inReplyTo":"412634.67173.qm@web31803.mail.mud.yahoo.com","subject":"Re: [PATCH] Avoid errors from git-rev-parse in gitweb blame","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-06-03T21:34:29Z","receivedAt":"2008-06-03T21:34:29Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Dnia wtorek 3. czerwca 2008 23:27, Luben Tuikov napisał:\n> --- On Tue, 6/3/08, Jakub Narebski wrote:\n\n> > > > > + my $parent_commit =\n> > <$dd> || '';\n> > \n> > By the way, here you would probably want \n> > \n> > + my $parent_commit = <$dd> ||\n> > '--root';\n> > \n> > (if it works).\n> \n> AFAIR, I did experiment with the \"--root\" parameter. Not sure why,\n> but my command line ended up without it, and thus git_blame2()\n> ended up without it.\n> \n> If you feel that it is, at this time having had changes commited since my\n> original commit, more correct to include it, then please go ahead.\n\nI'm sorry, I havent thought this through. This _cannot_ work. You can\nuse '--root' as 'hp' parameter to force creation commitdiff, but not as\n'hp' parameter to blame.\n\nSorry for the confusion.\n-- \nJakub Narebski\nPoland\n"},{"id":"78568","messageId":"4845CF9F.10604@gmail.com","threadId":"13775","inReplyTo":"200806032224.08714.jnareb@gmail.com","subject":"Re: [PATCH] Avoid errors from git-rev-parse in gitweb blame","fromName":"Lea Wiemann","fromEmail":"lewiemann@gmail.com","sentAt":"2008-06-03T23:11:27Z","receivedAt":"2008-06-03T23:11:27Z","isPatch":true,"sender":{"key":"lewiemann@gmail.com","avatar":null},"body":"Jakub Narebski wrote:\n> I don't think %parent_commits hash is suitable for caching; it is only\n> intermediate step, reducing number of git command calls (and forks) [...]\n> \n> ATTENTION! This example shows where caching [parsed] data have problems \n> compared to front-end caching (caching output).\n\nATTENTION! Could we please stop having this discussion?! Your argument \nis completely bogus. If the parent commit hashes are in cache, it's an \nalmost zero-time cache lookup. The only difference it might make \ncompared front-end caching is the CPU time it takes to generate the \npage, and *I want to see benchmarks before I even start thinking about \nCPU*. Okay? Good, thanks.\n\nSorry I'm a little indignant, but you seem to be somehow trying to tell \nme what to implement, and that gets annoying after a while. I don't \nmind your input, but at some point the discussion just doesn't go any \nfurther.\n\n> Problems occur when we try to cache page with _streaming_ output, such \n> as blob view, blame view, diff part of commitdiff etc.\n\nWe can still stream backend-cache-backed data, though it's a little \nharder. It's mostly a memory, not a performance issue though -- the \nonly point where I think it actually would be performance-relevant is \nblame, and blame doesn't stream anyway (see below).\n\n> By the way, if we agree that version %parent_commits is too intrusive \n> dusring GSoC 2008,\n\nOh, I don't mind, FTR. It's not enough lines to matter.\n\n>> 2) Major point: You're still forking a lot. The Right Thing is to\n>> condense everything into a single call\n> \n> This is not a good solution for 'blame' view, which is generated \"on the \n> fly\", by streaming git-blame output via filter.\n\nNo, whether you have your \"while <$fd>\" loop or not doesn't make a \ndifference. Blame first calculates the whole blame and then dumps it \nout in zero-time, unless you use --incremental. So there's no \nperformance difference in getting all blame output and then dumping it \nout vs. reading and outputting it line-by-line. And regarding memory, \nif your blame output doesn't fit into your RAM, you have different kinds \nof issues.\n\nJFTR, I don't have any opinion about extending the porcelain output of \ngit-blame (apart from the fact that happens to not be useful for gitweb \nfor the reason I outlined in the previous paragraph).\n\n-- Lea\n"},{"id":"78575","messageId":"200806040211.29430.jnareb@gmail.com","threadId":"13775","inReplyTo":"4845CF9F.10604@gmail.com","subject":"Re: [PATCH] Avoid errors from git-rev-parse in gitweb blame","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-06-04T00:11:28Z","receivedAt":"2008-06-04T00:11:28Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Lea Wiemann wrote:\n> Jakub Narebski wrote:\n>>\n>> I don't think %parent_commits hash is suitable for caching; it is only\n>> intermediate step, reducing number of git command calls (and forks) [...]\n>> \n>> ATTENTION! This example shows where caching [parsed] data have problems \n>> compared to front-end caching (caching output).\n> \n> ATTENTION! Could we please stop having this discussion?!\n\nYeah, yeah, I know. \"Talk is cheap, show me the code\" (or at least\npseudocode).\n\n> Your argument \n> is completely bogus. If the parent commit hashes are in cache, it's an \n> almost zero-time cache lookup.\n\nYou have cut a bit too much (quoted a bit too little) for me to decide\nif I made myself clear wrt. saving %parent_commits hash into cache.\n\nWhat I wanted to say that in caching intermediate data for 'blame' view\nyou have to save to cache something like @blocks (or @lines) array.\nThis array can contain parents of blamed commits, so there is no need\nfor saving %parent_commits separately: it would be duplication of\ninformation. This hash is needed to reduce number of calls to\ngit-rev-parse, and is used to generate parsed info, which info in turn\n(I think) can be cached.\n\n> The only difference it might make \n> compared front-end caching is the CPU time it takes to generate the \n> page, and *I want to see benchmarks before I even start thinking about \n> CPU*. Okay? Good, thanks.\n\nThe only place where I think front-end caching could be better is\n'blob' view with syntax highlighting (using some external filter, like\nGNU Source Highlight)... which is not implemented yet.\n\nI thought that snapshots (if enabled) would fall in this category, but\nthis is the case where data cache is almost identical to output cache\n(the same happens for [almost] all \"raw\" / *_plain views).\n\n> Sorry I'm a little indignant, but you seem to be somehow trying to tell \n> me what to implement, and that gets annoying after a while. I don't \n> mind your input, but at some point the discussion just doesn't go any \n> further.\n> \n>> Problems occur when we try to cache page with _streaming_ output, such \n>> as blob view, blame view, diff part of commitdiff etc.\n> \n> We can still stream backend-cache-backed data, though it's a little \n> harder. It's mostly a memory, not a performance issue though -- the \n> only point where I think it actually would be performance-relevant is \n> blame, and blame doesn't stream anyway (see below).\n\nAnd snapshots. We certainly want to stream snapshots, as they can be\nquite large.\n\nAlso blob_plain view might be difficult, if there are extremely large\nbinary files in the repository (it should not happen often, but it can\nhappen).\n\n[...]\n>>> 2) Major point: You're still forking a lot. The Right Thing is to\n>>> condense everything into a single call\n>> \n>> This is not a good solution for 'blame' view, which is generated \"on the \n>> fly\", by streaming git-blame output via filter.\n> \n> No, whether you have your \"while <$fd>\" loop or not doesn't make a \n> difference.\n\nIt perhaps makes no difference performance wise (solution with\n\"git rev-list --parents --no-walk\" has one fork more), but it might\nmake code unnecessarily more complicated. In the rev-list solution\nyou have to browse git-blame output to gather all blamed commits one\nwant to find parents of; in the case of extending git-blame you can\njust process block after block of code.\n\n> Blame first calculates the whole blame and then dumps it \n> out in zero-time, unless you use --incremental.\n\nThere is some code in the mailing list archive (and perhaps used by\nrepo's gitweb, but I might be mistaken), which adds\ngit_blame_incremental and use AJAX together with \"git blame --incremental\"\nto reduce latency. It was done by having JavaScript check if browser\nis AJAX-capable, and if it was rewriting 'blame' links to\n'blame_incremental'. But if there exist cached blame, I think it would\nbe as fast (in terms of latency) to generate 'blame' from cache as to\ngenerate 'blame_incremental'.\n\n> So there's no \n> performance difference in getting all blame output and then dumping it \n> out vs. reading and outputting it line-by-line.\n\nPerformance wise, perhaps not. Memory wise, perhaps yes; better not\nto use more memory than needed, especially if memcached is to share\nmachine.\n\n> And regarding memory, \n> if your blame output doesn't fit into your RAM, you have different kinds \n> of issues.\n\nTrue.\n\n> JFTR, I don't have any opinion about extending the porcelain output of \n> git-blame (apart from the fact that happens to not be useful for gitweb \n> for the reason I outlined in the previous paragraph).\n\nIt would be/might be (I haven't examined corner cases yet) important in\nthe case of file history which both contains evil merges, and it's\nsimplified history is different than full history.\n-- \nJakub Narebski\nPoland\n"},{"id":"78578","messageId":"4845E45E.9030504@gmail.com","threadId":"13775","inReplyTo":"200806040211.29430.jnareb@gmail.com","subject":"Re: [PATCH] Avoid errors from git-rev-parse in gitweb blame","fromName":"Lea Wiemann","fromEmail":"lewiemann@gmail.com","sentAt":"2008-06-04T00:39:58Z","receivedAt":"2008-06-04T00:39:58Z","isPatch":true,"sender":{"key":"lewiemann@gmail.com","avatar":null},"body":"Jakub Narebski wrote:\n> And snapshots [and blob_plain]. We certainly want to stream snapshots, as\n> they can be quite large.\n\nYup. I suppose that those need to be cached on disk rather than in \nmemory, so they need a separate cache.\n\n> [Parents in blame output:]\n> It perhaps makes no difference performance wise (solution with\n> \"git rev-list --parents --no-walk\" has one fork more), but it might\n> make code unnecessarily more complicated.\n\nA few lines. *shrugs* Probably actually easier than adding stuff to \ngit-blame's output, but I won't argue against the latter if you want it.\n\n> use AJAX together with \"git blame --incremental\" to reduce latency.\n> It was done by having JavaScript check if browser is AJAX-capable,\n\nUnfortunately there is no such check (and I doubt it's doable without \ncookie or redirect trickery) -- you'll find that the blames on \nrepo.or.cz don't work without JavaScript.\n\n-- Lea\n"},{"id":"78585","messageId":"7v3ant213k.fsf@gitster.siamese.dyndns.org","threadId":"13775","inReplyTo":"200806032331.44514.jnareb@gmail.com","subject":"Re: [PATCH] Avoid errors from git-rev-parse in gitweb blame","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-04T05:58:07Z","receivedAt":"2008-06-04T05:58:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n>> The intention was that it shouldn't necessarily be the (strict) parent\n>> of the change (changed segment), since it may or may not have changed\n>> in the strict parent commit. The intention was that it\n>> \"starts\"/\"opens\" the parent commit so that \"git\" would start from\n>> there and find the actual change/commit where that line/segment has\n>> changed. And it has worked pretty fine for me when data-mining\n>> (something I do quite often) code evolution.\n\nYes, but the current scheme breaks down in another way. When $full_rev\nadded many lines to the file, and you are adding the link to for a line\nnear the end of the file and such a line may not exist. This cannot be\ncheaply done even inside blame itself.\n\nAnother breakage is even though $full_rev^ _may_ exist (iow, $full_rev\nmight not be the root commit), the file being blamed may not exist there\n(iow $full_rev might have introduced the file). Instead of running\n\"rev-parse $full_rev^\", you would at least need to ask \"rev-list -1\n$full_rev^ -- $path\" or something from the Porcelain layer, but\nunfortunately this is rather expensive.\n\nBecause blame already almost knows if the commit the final blame lies on\nhas a parent, it would be reasonably cheap to add that \"parent or nothing\"\ninformation to its --porcelain (and its --incremental) format if we wanted\nto.\n"},{"id":"78620","messageId":"200806041431.07494.jnareb@gmail.com","threadId":"13775","inReplyTo":"4845E45E.9030504@gmail.com","subject":"Re: [PATCH] Avoid errors from git-rev-parse in gitweb blame","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-06-04T12:31:06Z","receivedAt":"2008-06-04T12:31:06Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Lea Wiemann wrote:\n> Jakub Narebski wrote:\n> >\n> > And snapshots [and blob_plain]. We certainly want to stream snapshots, as\n> > they can be quite large.\n> \n> Yup. I suppose that those need to be cached on disk rather than in \n> memory, so they need a separate cache.\n\nOr at least (in the first implementation) to avoid caching them in\nmemory-based cache (and serve them uncached).\n\nAlthough I wonder how memory-based caches such as memcached or swifty,\nand perhaps also mmap based cache (BerkeleyDB based cache is supposedly\nfast because it fits into memory/caches in memory) deals with overly\nlarge cache entries...\n\n> > [Parents in blame output:]\n> > It perhaps makes no difference performance wise (solution with\n> > \"git rev-list --parents --no-walk\" has one fork more), but it might\n> > make code unnecessarily more complicated.\n> \n> A few lines. *shrugs* Probably actually easier than adding stuff to \n> git-blame's output, but I won't argue against the latter if you want it.\n\nWith modified (enhanced) git-blame output code would look like this\n(rough pseudocode):\n\n while (<$fd>) {\n ...\n \n ...\n }\n\nwhile using no-walk rev-list requires list of blamed parents upfront,\nso the code would have to look like this\n\n @blame_data = <$fd>;\n @commitlist = map { } grep {
} @blame_list;\n %commit_parents = get_parents(\\@commitlist); # calls git-rev-list\n foreach (@commitlist) {\n ...\n ...\n }\n\nNote that you read whole data into gitweb, inclreasing memory usage...\nwhich we want to avoid, especially when using memcached or similar\ncaching backend (git-blame itself has to keep data in memory, but no\nneed to duplicate the amount).\n\n\nBesides git-blame output needs to be extended/enhanced anyway for the\ndata mining / annotated file history navigation Luben wanted to be\nreally robust. See my response to Linus email in this thread (to be\nwritten).\n\n> > use AJAX together with \"git blame --incremental\" to reduce latency.\n> > It was done by having JavaScript check if browser is AJAX-capable,\n> \n> Unfortunately there is no such check (and I doubt it's doable without \n> cookie or redirect trickery) -- you'll find that the blames on \n> repo.or.cz don't work without JavaScript.\n\nI have in my git repository original version (well, one of original\nversions) adding incremental blame output\n\n Message-ID: <20070825222404.16967.9402.stgit@rover>\n http://permalink.gmane.org/gmane.comp.version-control.git/56657\n\nby Petr Baudis, tweaked version of Fredrik Kuivinen patch, and in the\ncommit message there is the floowing info:\n\n Compared to the original patch, this one works with pathinfo-ish URLs as\n well, and should play well with non-javascript browsers as well (the HTML\n points to the blame action, while javascript code rewrites the links to use\n the blame_incremental action; it is somewhat hackish but I couldn't think\n of a better solution).\n\nInstead of rewriting links gitweb's JavaScript could use JavaScript\nredirect trickery, using JavaScript (by setting location.href for\nexample) to redirect to blame_incremental action from blame action.\n\n\nAs to checking if browser is AJAX capable: you can at least check\nif all methods needed are available.\n\n\nP.S. You would probably want to remove old git-annotate based git_blame\n(dead code, currently not used by any action), and rename git_blame2 to\ngit_blame. A bit less code to check for caching problems etc,...\n-- \nJakub Narebski\nPoland\n"},{"id":"78621","messageId":"200806041603.49555.jnareb@gmail.com","threadId":"13775","inReplyTo":"7v3ant213k.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Avoid errors from git-rev-parse in gitweb blame","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-06-04T14:03:48Z","receivedAt":"2008-06-04T14:03:48Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Wed, 4 Jun 2008, Junio C Hamano wrote:\n> Jakub Narebski writes:\n>> On Tue, 3 Jan 2008, Luben Tuikov wrote:\n>>>\n>>> The intention was that it shouldn't necessarily be the (strict) parent\n>>> of the change (changed segment), since it may or may not have changed\n>>> in the strict parent commit. The intention was that it\n>>> \"starts\"/\"opens\" the parent commit so that \"git\" would start from\n>>> there and find the actual change/commit where that line/segment has\n>>> changed. And it has worked pretty fine for me when data-mining\n>>> (something I do quite often) code evolution.\n> \n> Yes, but the current scheme breaks down in another way. When $full_rev\n> added many lines to the file, and you are adding the link to for a line\n> near the end of the file and such a line may not exist. This cannot be\n> cheaply done even inside blame itself.\n\nI think the scheme could be fixed by proposed belo git-blame porcelain\nformat output extension. Can it be done cheaply? I don't know,\ngenerating extended info as described below should be cheap if we\nhave equivalent of textual (patch) diff between commit blamed for\ngiven line, and its parent; actually what we need is more of 'context'\ndiff than of default 'unified' diff.\n\n> Another breakage is even though $full_rev^ _may_ exist (iow, $full_rev\n> might not be the root commit), the file being blamed may not exist there\n> (iow $full_rev might have introduced the file). Instead of running\n> \"rev-parse $full_rev^\", you would at least need to ask \"rev-list -1\n> $full_rev^ -- $path\" or something from the Porcelain layer, but\n> unfortunately this is rather expensive.\n\nDoesn't blame know revision graph for history of a given file already?\n\nBut even without it (i.e. ony 'parents' header showing true, not\nrewritten parents) what we need is some info about pre-image for blamed\nline. We would need line number of the line in pre-image (or NUL\nif the page was added in blamed commit), and pre-image filename.\n\nI don't know if it could be done cheaply, and if it could be done\nsimply; currently git-diff doesn't have \"context diff\" format output,\nand what I though about by pre-image line number requires finding if\na line was added in a commit, or was modified in a commit. (If it\nwas removed, it wouldn't be in final image and hence wouldn't be\nblamed; if it was moved, it wouldn't be blamed, as blame follows\ncode movement).\n\n> Because blame already almost knows if the commit the final blame lies on\n> has a parent, it would be reasonably cheap to add that \"parent or nothing\"\n> information to its --porcelain (and its --incremental) format if we wanted\n> to.\n\nIt would be easy to add 'parents' header, perhaps empty if we blame\nroot commit, or a boundary commit (do we say 'boundary' then?) when\ndoing revision limited blaming.\n\n>From what you write it wouldn't be easy to add \"history of a given\nline begins here\", or even \"history of a given file begins there\"...\n\n-- \nJakub Narebski\nPoland\n"},{"id":"78676","messageId":"469507.93901.qm@web31804.mail.mud.yahoo.com","threadId":"13775","inReplyTo":"7v3ant213k.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Avoid errors from git-rev-parse in gitweb blame","fromName":"Luben Tuikov","fromEmail":"ltuikov@yahoo.com","sentAt":"2008-06-04T22:24:03Z","receivedAt":"2008-06-04T22:24:03Z","isPatch":true,"sender":{"key":"ltuikov@yahoo.com","avatar":null},"body":"--- On Tue, 6/3/08, Junio C Hamano wrote:\n\n> Another breakage is even though $full_rev^ _may_ exist\n> (iow, $full_rev\n> might not be the root commit), the file being blamed may\n> not exist there\n> (iow $full_rev might have introduced the file). Instead of\n> running\n> \"rev-parse $full_rev^\", you would at least need\n> to ask \"rev-list -1\n> $full_rev^ -- $path\" or something from the Porcelain\n> layer, but\n> unfortunately this is rather expensive.\n\nYes, I've seen this too, but saw no advantage to bring it up\nat the time.\n\n> Because blame already almost knows if the commit the final\n> blame lies on\n> has a parent, it would be reasonably cheap to add that\n> \"parent or nothing\"\n> information to its --porcelain (and its --incremental)\n> format if we wanted\n> to.\n\nYes, I agree. At the moment those \"checks\" are left to be\ndeduced by the person data-mining with blame. (Which isn't /that/\nbad.)\n\n Luben\n"},{"id":"78702","messageId":"7vd4mw4dpp.fsf@gitster.siamese.dyndns.org","threadId":"13775","inReplyTo":"200806041603.49555.jnareb@gmail.com","subject":"Re: [PATCH] Avoid errors from git-rev-parse in gitweb blame","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-05T06:07:14Z","receivedAt":"2008-06-05T06:07:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> On Wed, 4 Jun 2008, Junio C Hamano wrote:\n>> Yes, but the current scheme breaks down in another way. When $full_rev\n>> added many lines to the file, and you are adding the link to for a line\n>> near the end of the file and such a line may not exist. This cannot be\n>> cheaply done even inside blame itself.\n>\n> I think the scheme could be fixed by proposed belo git-blame porcelain\n> format output extension.\n\nFor the line number information, I do not think so.\n\nLuben's \"continue from the same line number in the parent commit\" is a\ncute hack, but that strategy needs a qualifying comment \"because hoping\nthat the same line number in the parent commit might have something\nrelevant would be better than stopping and giving up sometimes.\" It\ncannot reliably work (and it is not Luben's fault).\n\nBut the #l fragment is just a hint to scroll to that point after\nrestarting the blame from previous commit and jumping to the result, so it\nmay not be too big a deal. Such a line may not exist in the resulting\nblame page, but that's Ok.\n\n>> Another breakage is even though $full_rev^ _may_ exist (iow, $full_rev\n>> might not be the root commit), the file being blamed may not exist there\n>> (iow $full_rev might have introduced the file). Instead of running\n>> \"rev-parse $full_rev^\", you would at least need to ask \"rev-list -1\n>> $full_rev^ -- $path\" or something from the Porcelain layer, but\n>> unfortunately this is rather expensive.\n>\n> Doesn't blame know revision graph for history of a given file already?\n\nNot in the sense of \"rev-list -2 $full_rev -- $path | sed -e 1d\". It\nbuilds the graph as it digs deeper, and when it stops, it stopped digging,\nso all it knows at that point without further computation is $full_rev^@,\nand not \"the previous commit that touched the path\".\n\nBut as Luben explained (and you drew a simple strand of pearls history to\nillustrate), immediate parent is just for the purpose of restarting the\ncomputation.\n\n>> Because blame already almost knows if the commit the final blame lies on\n>> has a parent, it would be reasonably cheap to add that \"parent or nothing\"\n>> information to its --porcelain (and its --incremental) format if we wanted\n>> to.\n>\n> It would be easy to add 'parents' header, perhaps empty if we blame\n> root commit, or a boundary commit (do we say 'boundary' then?) when\n> doing revision limited blaming.\n\nIt shouldn't be too hard to say \"parents of the blamed commit that has the\ncorresponding preimage of the file is this\", and the history does not have\nto be limited. You need to also handle \"the commit that introduced the\npath\" case just like \"root\" and \"boundary\" that we cannot dig further than\nthat point.\n\nI'll follow this message up with two weatherballoon patches.\n"},{"id":"78703","messageId":"7v8wxk4dml.fsf_-_@gitster.siamese.dyndns.org","threadId":"13775","inReplyTo":"7vd4mw4dpp.fsf@gitster.siamese.dyndns.org","subject":"[PATCH 1/2] git-blame: refactor code to emit \"porcelain format\" output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-05T06:09:06Z","receivedAt":"2008-06-05T06:09:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Both the --porcelain and --incremental format shared the same output\nformat but implemented with two identical codepaths. This merges them\ninto one shared function.\n\nSigned-off-by: Junio C Hamano \n---\n * This is just a preparatory clean-up patch (on jc/blame topic)\n\n builtin-blame.c | 65 ++++++++++++++++++++++++++----------------------------\n 1 files changed, 31 insertions(+), 34 deletions(-)\n\ndiff --git a/builtin-blame.c b/builtin-blame.c\nindex 5c7546d..4b9c601 100644\n--- a/builtin-blame.c\n+++ b/builtin-blame.c\n@@ -1479,6 +1479,34 @@ static void write_filename_info(const char *path)\n }\n \n /*\n+ * Porcelain/Incremental format wants to show a lot of details per\n+ * commit. Instead of repeating this every line, emit it only once,\n+ * the first time each commit appears in the output.\n+ */\n+static int emit_one_suspect_detail(struct origin *suspect)\n+{\n+\tstruct commit_info ci;\n+\n+\tif (suspect->commit->object.flags & METAINFO_SHOWN)\n+\t\treturn 0;\n+\n+\tsuspect->commit->object.flags |= METAINFO_SHOWN;\n+\tget_commit_info(suspect->commit, &ci, 1);\n+\tprintf(\"author %s\\n\", ci.author);\n+\tprintf(\"author-mail %s\\n\", ci.author_mail);\n+\tprintf(\"author-time %lu\\n\", ci.author_time);\n+\tprintf(\"author-tz %s\\n\", ci.author_tz);\n+\tprintf(\"committer %s\\n\", ci.committer);\n+\tprintf(\"committer-mail %s\\n\", ci.committer_mail);\n+\tprintf(\"committer-time %lu\\n\", ci.committer_time);\n+\tprintf(\"committer-tz %s\\n\", ci.committer_tz);\n+\tprintf(\"summary %s\\n\", ci.summary);\n+\tif (suspect->commit->object.flags & UNINTERESTING)\n+\t\tprintf(\"boundary\\n\");\n+\treturn 1;\n+}\n+\n+/*\n * The blame_entry is found to be guilty for the range. Mark it\n * as such, and show it in incremental output.\n */\n@@ -1493,22 +1521,7 @@ static void found_guilty_entry(struct blame_entry *ent)\n \t\tprintf(\"%s %d %d %d\\n\",\n \t\t sha1_to_hex(suspect->commit->object.sha1),\n \t\t ent->s_lno + 1, ent->lno + 1, ent->num_lines);\n-\t\tif (!(suspect->commit->object.flags & METAINFO_SHOWN)) {\n-\t\t\tstruct commit_info ci;\n-\t\t\tsuspect->commit->object.flags |= METAINFO_SHOWN;\n-\t\t\tget_commit_info(suspect->commit, &ci, 1);\n-\t\t\tprintf(\"author %s\\n\", ci.author);\n-\t\t\tprintf(\"author-mail %s\\n\", ci.author_mail);\n-\t\t\tprintf(\"author-time %lu\\n\", ci.author_time);\n-\t\t\tprintf(\"author-tz %s\\n\", ci.author_tz);\n-\t\t\tprintf(\"committer %s\\n\", ci.committer);\n-\t\t\tprintf(\"committer-mail %s\\n\", ci.committer_mail);\n-\t\t\tprintf(\"committer-time %lu\\n\", ci.committer_time);\n-\t\t\tprintf(\"committer-tz %s\\n\", ci.committer_tz);\n-\t\t\tprintf(\"summary %s\\n\", ci.summary);\n-\t\t\tif (suspect->commit->object.flags & UNINTERESTING)\n-\t\t\t\tprintf(\"boundary\\n\");\n-\t\t}\n+\t\temit_one_suspect_detail(suspect);\n \t\twrite_filename_info(suspect->path);\n \t\tmaybe_flush_or_die(stdout, \"stdout\");\n \t}\n@@ -1615,24 +1628,8 @@ static void emit_porcelain(struct scoreboard *sb, struct blame_entry *ent)\n \t ent->s_lno + 1,\n \t ent->lno + 1,\n \t ent->num_lines);\n-\tif (!(suspect->commit->object.flags & METAINFO_SHOWN)) {\n-\t\tstruct commit_info ci;\n-\t\tsuspect->commit->object.flags |= METAINFO_SHOWN;\n-\t\tget_commit_info(suspect->commit, &ci, 1);\n-\t\tprintf(\"author %s\\n\", ci.author);\n-\t\tprintf(\"author-mail %s\\n\", ci.author_mail);\n-\t\tprintf(\"author-time %lu\\n\", ci.author_time);\n-\t\tprintf(\"author-tz %s\\n\", ci.author_tz);\n-\t\tprintf(\"committer %s\\n\", ci.committer);\n-\t\tprintf(\"committer-mail %s\\n\", ci.committer_mail);\n-\t\tprintf(\"committer-time %lu\\n\", ci.committer_time);\n-\t\tprintf(\"committer-tz %s\\n\", ci.committer_tz);\n-\t\twrite_filename_info(suspect->path);\n-\t\tprintf(\"summary %s\\n\", ci.summary);\n-\t\tif (suspect->commit->object.flags & UNINTERESTING)\n-\t\t\tprintf(\"boundary\\n\");\n-\t}\n-\telse if (suspect->commit->object.flags & MORE_THAN_ONE_PATH)\n+\tif (emit_one_suspect_detail(suspect) ||\n+\t (suspect->commit->object.flags & MORE_THAN_ONE_PATH))\n \t\twrite_filename_info(suspect->path);\n \n \tcp = nth_line(sb, ent->lno);\n-- \n1.5.6.rc1.12.g7f718\n"},{"id":"78704","messageId":"7v4p884dlh.fsf_-_@gitster.siamese.dyndns.org","threadId":"13775","inReplyTo":"7vd4mw4dpp.fsf@gitster.siamese.dyndns.org","subject":"[PATCH 2/2] blame: show \"previous\" information in --porcelain/--incremental format","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-05T06:09:46Z","receivedAt":"2008-06-05T06:09:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When the final blame is laid for a line to a pair, it also\ngives a \"previous\" information to --porcelain and --incremental output\nformat. It gives the parent commit of the blamed commit, _and_ a path in\nthat parent commit that corresponds to the blamed path --- in short, it is\nthe origin that would have been blamed (or passed blame through) for the\nline _if_ the blamed commit did not change that line.\n\nThis unfortunately makes sanity checking of refcount quite complex, so I\nripped it out for now.\n\nSigned-off-by: Junio C Hamano \n---\n builtin-blame.c | 42 ++++++++++++------------------------------\n 1 files changed, 12 insertions(+), 30 deletions(-)\n\ndiff --git a/builtin-blame.c b/builtin-blame.c\nindex 4b9c601..a46e402 100644\n--- a/builtin-blame.c\n+++ b/builtin-blame.c\n@@ -82,6 +82,7 @@ static unsigned blame_copy_score;\n */\n struct origin {\n \tint refcnt;\n+\tstruct origin *previous;\n \tstruct commit *commit;\n \tmmfile_t file;\n \tunsigned char blob_sha1[20];\n@@ -123,6 +124,8 @@ static inline struct origin *origin_incref(struct origin *o)\n static void origin_decref(struct origin *o)\n {\n \tif (o && --o->refcnt <= 0) {\n+\t\tif (o->previous)\n+\t\t\torigin_decref(o->previous);\n \t\tfree(o->file.ptr);\n \t\tfree(o);\n \t}\n@@ -1280,6 +1283,10 @@ static void pass_blame(struct scoreboard *sb, struct origin *origin, int opt)\n \t\tstruct origin *porigin = sg_origin[i];\n \t\tif (!porigin)\n \t\t\tcontinue;\n+\t\tif (!origin->previous) {\n+\t\t\torigin_incref(porigin);\n+\t\t\torigin->previous = porigin;\n+\t\t}\n \t\tif (pass_blame_to_parent(sb, origin, porigin))\n \t\t\tgoto finish;\n \t}\n@@ -1503,6 +1510,11 @@ static int emit_one_suspect_detail(struct origin *suspect)\n \tprintf(\"summary %s\\n\", ci.summary);\n \tif (suspect->commit->object.flags & UNINTERESTING)\n \t\tprintf(\"boundary\\n\");\n+\tif (suspect->previous) {\n+\t\tstruct origin *prev = suspect->previous;\n+\t\tprintf(\"previous %s \", sha1_to_hex(prev->commit->object.sha1));\n+\t\twrite_name_quoted(prev->path, stdout, '\\n');\n+\t}\n \treturn 1;\n }\n \n@@ -1866,36 +1878,6 @@ static void sanity_check_refcnt(struct scoreboard *sb)\n \t\t\tbaa = 1;\n \t\t}\n \t}\n-\tfor (ent = sb->ent; ent; ent = ent->next) {\n-\t\t/* Mark the ones that haven't been checked */\n-\t\tif (0 < ent->suspect->refcnt)\n-\t\t\tent->suspect->refcnt = -ent->suspect->refcnt;\n-\t}\n-\tfor (ent = sb->ent; ent; ent = ent->next) {\n-\t\t/*\n-\t\t * ... then pick each and see if they have the the\n-\t\t * correct refcnt.\n-\t\t */\n-\t\tint found;\n-\t\tstruct blame_entry *e;\n-\t\tstruct origin *suspect = ent->suspect;\n-\n-\t\tif (0 < suspect->refcnt)\n-\t\t\tcontinue;\n-\t\tsuspect->refcnt = -suspect->refcnt; /* Unmark */\n-\t\tfor (found = 0, e = sb->ent; e; e = e->next) {\n-\t\t\tif (e->suspect != suspect)\n-\t\t\t\tcontinue;\n-\t\t\tfound++;\n-\t\t}\n-\t\tif (suspect->refcnt != found) {\n-\t\t\tfprintf(stderr, \"%s in %s has refcnt %d, not %d\\n\",\n-\t\t\t\tent->suspect->path,\n-\t\t\t\tsha1_to_hex(ent->suspect->commit->object.sha1),\n-\t\t\t\tent->suspect->refcnt, found);\n-\t\t\tbaa = 2;\n-\t\t}\n-\t}\n \tif (baa) {\n \t\tint opt = 0160;\n \t\tfind_alignment(sb, &opt);\n-- \n1.5.6.rc1.12.g7f718\n"},{"id":"78854","messageId":"200806060226.57124.jnareb@gmail.com","threadId":"13775","inReplyTo":"7vd4mw4dpp.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Avoid errors from git-rev-parse in gitweb blame","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-06-06T00:26:54Z","receivedAt":"2008-06-06T00:26:54Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Tue, 5 Jan 2008, Junio C Hamano wrote:\n> Jakub Narebski writes:\n>> On Wed, 4 Jun 2008, Junio C Hamano wrote:\n>>>\n>>> Yes, but the current scheme breaks down in another way. When $full_rev\n>>> added many lines to the file, and you are adding the link to for a line\n>>> near the end of the file and such a line may not exist. This cannot be\n>>> cheaply done even inside blame itself.\n>>\n>> I think the scheme could be fixed by proposed below git-blame porcelain\n>> format output extension.\n> \n> For the line number information, I do not think so.\n\nI think I have not made myself clear enough. In the proposed additional\nblame format extension \"previous\" header (or \"pre-image\" header, or sth\nlike that) would contain pre-image (before change) line number\ninformation, or information that line was added in blamed commit.\n\n> Luben's \"continue from the same line number in the parent commit\" is a\n> cute hack, but that strategy needs a qualifying comment \"because hoping\n> that the same line number in the parent commit might have something\n> relevant would be better than stopping and giving up sometimes.\" It\n> cannot reliably work (and it is not Luben's fault).\n\nTherefore my proposal. I'm not sure though if it can be done cheaply.\nAnd Luben's idea is good enough in most cases; as a hint is definitely\ngood enough.\n\n> But the #l fragment is just a hint to scroll to that point after\n> restarting the blame from previous commit and jumping to the result, so it\n> may not be too big a deal. Such a line may not exist in the resulting\n> blame page, but that's Ok.\n\nLet me give an example on how I visualized proposed \"pre-image\" header\nextension would work.\n\nAssume that we started from commit 'S' and commit 'B' is to be blamed\nfor the line in question. Let's us assume that commit 'B' has only\none parent, and that \"context\" diff between B^1 and B is available to\nblame. For an example, let's use [modified] example from GNU diff\ndocumentation (info):\n\n diff --git-context a/lao-tzu b/lao-tzu\n index 55fb100..198772c 100644\n *** a/lao-tzu\n --- b/lao-tzu\n ***************\n *** 1,7 ****\n - The Way that can be told of is not the eternal Way;\n - The name that can be named is not the eternal name.\n The Nameless is the origin of Heaven and Earth;\n ! The Named is the mother of all things.\n Therefore let there always be non-being,\n so we may see their subtlety,\n And let there always be being,\n --- 1,6 ----\n The Nameless is the origin of Heaven and Earth;\n ! The named is the mother of all things.\n !\n Therefore let there always be non-being,\n so we may see their subtlety,\n And let there always be being,\n ***************\n *** 9,11 ****\n --- 8,13 ----\n The two are the same,\n But after they are produced,\n But after they are produced,\n they have different names.\n + They both may be called deep and profound.\n + Deeper and more profound,\n + The door of all subtleties!\n\nFirst example: lets assume that we want find blame (annotation) for\nthe following line:\n \"The named is the mother of all things.\"\nAssuming that block of commonly blamed lines begin with given line,\ncurrent blame output would look like the following:\n\n 2 \n author A U Thor\n author-mail \n author-time 1150613103\n author-tz -0700\n committer C O Mitter\n committer-mail \n committer-time 1150690754\n committer-tz -0700\n filename lao-tzu\n summary Be even more cryptic\n \tThe named is the mother of all things.\n\nWhat I wanted to add was the following header\n\n parents \n pre-image 2 4 lao-tzu\n\nwhere 2 is the line number in the commit given line is attributed to,\nwhere 4 is the line number of _corresponding_ line in pre-image (before\nchange that gave examined line current form), and 'lao-tzu' is the name\nof file of pre-image for this line.\n\n\nSecond example: lets assume that we want find blame (annotation) for\nthe following line:\n \"The door of all subtleties!\"\n\nThis time the line was added in the commit it is attributed to (commit\nblamed for this line), so there is no corresponding pre-image line.\nExtra headers would now look like the following:\n\n parents \n pre-image 13 - lao-tzu\n\n\nOf course 'parents' and 'pre-image' headers can be joined together in\nthe 'previous' header you proposed.\n\n>>> Another breakage is even though $full_rev^ _may_ exist (iow, $full_rev\n>>> might not be the root commit), the file being blamed may not exist there\n>>> (iow $full_rev might have introduced the file). Instead of running\n>>> \"rev-parse $full_rev^\", you would at least need to ask \"rev-list -1\n>>> $full_rev^ -- $path\" or something from the Porcelain layer, but\n>>> unfortunately this is rather expensive.\n>>\n>> Doesn't blame know revision graph for history of a given file already?\n> \n> Not in the sense of \"rev-list -2 $full_rev -- $path | sed -e 1d\". It\n> builds the graph as it digs deeper, and when it stops, it stopped digging,\n> so all it knows at that point without further computation is $full_rev^@,\n> and not \"the previous commit that touched the path\".\n> \n> But as Luben explained (and you drew a simple strand of pearls history to\n> illustrate), immediate parent is just for the purpose of restarting the\n> computation.\n\nWhat I worry about is what happens in the (rare I think) case when\n_merge_ commit is blamed, and firs-parent leg is simplified in the\n\"per-file\" history.\n\n\nFor example if git-blame output for given line looks like below:\n\n 64625efeb1f216c3811230845bb519123ea0ddc5 2 2 1\n author Jakub Narebski\n author-mail \n author-time 1212711688\n author-tz +0200\n committer Jakub Narebski\n committer-mail \n committer-time 1212711688\n committer-tz +0200\n filename foo\n summary Merge branch 'b' (Fullstop _and_ capitalization)\n \tSecond line.\n\nand \"git show 64625efeb1f216c3811230845bb519123ea0ddc5\" is:\n\n commit 64625efeb1f216c3811230845bb519123ea0ddc5\n Merge: 288f63a... ec70d8b...\n Author: Jakub Narebski \n Date: Fri Jun 6 02:21:28 2008 +0200\n \n Merge branch 'b' (Fullstop _and_ capitalization)\n\n diff --cc foo\n index 11c1e59,2636666..888af51\n --- a/foo\n +++ b/foo\n @@@ -1,3 -1,3 +1,3 @@@\n First line\n - second line.\n -Second line\n ++Second line.\n Third line.\n\nWhat should be \"previous\" line then? Or perhaps there should be _two_\n\"previous\" lines.\n\n>>> Because blame already almost knows if the commit the final blame lies on\n>>> has a parent, it would be reasonably cheap to add that \"parent or nothing\"\n>>> information to its --porcelain (and its --incremental) format if we wanted\n>>> to.\n>>\n>> It would be easy to add 'parents' header, perhaps empty if we blame\n>> root commit, or a boundary commit (do we say 'boundary' then?) when\n>> doing revision limited blaming.\n> \n> It shouldn't be too hard to say \"parents of the blamed commit that has the\n> corresponding preimage of the file is this\", and the history does not have\n> to be limited. You need to also handle \"the commit that introduced the\n> path\" case just like \"root\" and \"boundary\" that we cannot dig further than\n> that point.\n\nErrr... do your code deal with that case (no path before blamed commit)?\n \n> I'll follow this message up with two weatherballoon patches.\n\nThanks a lot. It looks like step in good direction (implementing first\nproposal), with the caveat of attributed commit being evil merge.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"78884","messageId":"200806061122.09621.jnareb@gmail.com","threadId":"13775","inReplyTo":"7v8wxk4dml.fsf_-_@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/2] git-blame: refactor code to emit \"porcelain format\" output","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-06-06T09:22:07Z","receivedAt":"2008-06-06T09:22:07Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Tue, 5 Jun 2008, Junio C Hamano wrote:\n\n> Both the --porcelain and --incremental format shared the same output\n> format but implemented with two identical codepaths. This merges them\n> into one shared function.\n\nThey have _almost_ the same, but not _exactly_ the same output:\n\n INCREMENTAL OUTPUT\n ------------------\n\n [...]\n\n The output format is similar to the Porcelain format, but it\n does not contain the actual lines from the file that is being\n annotated.\n\n [...]\n\n . Unlike Porcelain format, the filename information is always\n given and terminates the entry:\n\n \"filename\" \n\n and thus it's really quite easy to parse for some line- and\n word-oriented parser (which should be quite natural for most\n scripting languages).\n\nBut I guess this got addresssed in the patch, doesn't it?\n-- \nJakub Narebski\nPoland\n"},{"id":"78885","messageId":"200806061128.04031.jnareb@gmail.com","threadId":"13775","inReplyTo":"7v4p884dlh.fsf_-_@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/2] blame: show \"previous\" information in --porcelain/--incremental format","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-06-06T09:27:59Z","receivedAt":"2008-06-06T09:27:59Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Tue, 5 Jan 2008, Junio C Hamano wrote:\n\n> When the final blame is laid for a line to a pair, it also\n> gives a \"previous\" information to --porcelain and --incremental output\n> format. It gives the parent commit of the blamed commit, _and_ a path in\n> that parent commit that corresponds to the blamed path --- in short, it is\n> the origin that would have been blamed (or passed blame through) for the\n> line _if_ the blamed commit did not change that line.\n[...]\n> +\tif (suspect->previous) {\n> +\t\tstruct origin *prev = suspect->previous;\n> +\t\tprintf(\"previous %s \", sha1_to_hex(prev->commit->object.sha1));\n> +\t\twrite_name_quoted(prev->path, stdout, '\\n');\n> +\t}\n\nWhat happens if attributed (blamed) commit is \"evil merge\"?\nWould git-blame emit multiple \"previous \"\nheaders?\n-- \nJakub Narebski\nPoland\n"},{"id":"78919","messageId":"7vabhywq2c.fsf@gitster.siamese.dyndns.org","threadId":"13775","inReplyTo":"200806061128.04031.jnareb@gmail.com","subject":"Re: [PATCH 2/2] blame: show \"previous\" information in --porcelain/--incremental format","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-06T15:17:31Z","receivedAt":"2008-06-06T15:17:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> What happens if attributed (blamed) commit is \"evil merge\"?\n> Would git-blame emit multiple \"previous \"\n> headers?\n\nRead the code. There is only one previous pointer in each origin.\n"},{"id":"78924","messageId":"200806061744.36687.jnareb@gmail.com","threadId":"13775","inReplyTo":"7vabhywq2c.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/2] blame: show \"previous\" information in --porcelain/--incremental format","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-06-06T15:44:36Z","receivedAt":"2008-06-06T15:44:36Z","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>> What happens if attributed (blamed) commit is \"evil merge\"?\n>> Would git-blame emit multiple \"previous \"\n>> headers?\n> \n> Read the code. There is only one previous pointer in each origin.\n\nAh. True.\n\nSo the question now is: how git-blame choses one parent if commit\nwhich is blamed is an evil merge commit?\n\n-- \nJakub Narebski\nPoland\n"},{"id":"79146","messageId":"484C22BF.7040700@gmail.com","threadId":"13775","inReplyTo":"4845CF9F.10604@gmail.com","subject":"Re: [PATCH] Avoid errors from git-rev-parse in gitweb blame","fromName":"Lea Wiemann","fromEmail":"lewiemann@gmail.com","sentAt":"2008-06-08T18:19:43Z","receivedAt":"2008-06-08T18:19:43Z","isPatch":true,"sender":{"key":"lewiemann@gmail.com","avatar":null},"body":"Lea Wiemann wrote:\n> Blame first calculates the whole blame and then dumps it out in\n> zero-time [so] there's no performance difference in getting all blame\n> output and then dumping it out vs. reading and outputting it line-by-line.\n\nI haven't been following the recent discussion in detail, but here's \nanother thought: If you want to look up the parents, it's usually faster \n(at least when caching is enabled) to get them all in a single call. \nIOW, don't look up the parent for each hash as it appears, but collect \nall hashes and then get a list of all parents with a single call.\n\n-- Lea\n"},{"id":"79156","messageId":"200806082228.55495.jnareb@gmail.com","threadId":"13775","inReplyTo":"484C22BF.7040700@gmail.com","subject":"Re: [PATCH] Avoid errors from git-rev-parse in gitweb blame","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-06-08T20:28:52Z","receivedAt":"2008-06-08T20:28:52Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sun, 8 Jun 2008, Lea Wiemann wrote:\n> On Wed, 04 Jun 2008, Lea Wiemann wrote:\n>>\n>> Blame first calculates the whole blame and then dumps it out in\n>> zero-time [so] there's no performance difference in getting all blame\n>> output and then dumping it out vs. reading and outputting it line-by-line.\n> \n> I haven't been following the recent discussion in detail, but here's \n> another thought: If you want to look up the parents, it's usually faster \n> (at least when caching is enabled) to get them all in a single call. \n> IOW, don't look up the parent for each hash as it appears, but collect \n> all hashes and then get a list of all parents with a single call.\n\nIf caching is enabled, then parent info can be retrieved from cache.\nIf caching is disabled, or cache expired (cache miss) you would have\nto get whole blame output to get all revisions to get parents for.\nThis means for a short while twice amount of memory (whole blame in\ngit-blame, because thats how non-incremental blame works, and whole\nblame in gitweb, till reading last byte of blame when git-blame ends);\nand that is not good when memory-based cache (be it memcache, mmap,\nor other solution) is on the same machine (sometimes you just don't\nhave a farm of servers...).\n\nJunio's patches adding \"previous\" header to git blame result in no\nworse output (result) than current code. I have proposed improvements,\nbut I'm not sure they can be implemented cheaply (fairly sure that they\ncannot, and I'm not sure if improvements are worth the cost). I'd like\nto know what happens in Junio code when evil merge is blamed; I don't\nknow code enough (and I am a bit lazy here) to get this from code\nitself.\n\n-- \nJakub Narebski\nPoland\n"}]}