{"thread":{"id":"42465","subject":"[PATCH] blame.c: don't drop origin blobs as eagerly","startedAt":"2016-05-27T13:35:41Z","lastAt":"2016-05-28T14:00:21Z","messageCount":7,"participants":["David Kastrup","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"287672","messageId":"1464356141-3797-1-git-send-email-dak@gnu.org","threadId":"42465","inReplyTo":null,"subject":"[PATCH] blame.c: don't drop origin blobs as eagerly","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2016-05-27T13:35:41Z","receivedAt":"2016-05-27T13:35:41Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"When a parent blob already has chunks queued up for blaming, dropping\nthe blob at the end of one blame step will cause it to get reloaded\nright away, doubling the amount of I/O and unpacking when processing a\nlinear history.\n\nKeeping such parent blobs in memory seems like a reasonable\noptimization.  It's conceivable that this may incur additional memory\npressure particularly when the history contains lots of merges from\nlong-diverged branches.  In practice, this optimization appears to\nbehave quite benignly, and a viable strategy for limiting the total\namount of cached blobs in a useful manner seems rather hard to\nimplement.  In addition, calling git-blame with -C leads to similar\nmemory retention patterns.\n---\n builtin/blame.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 21f42b0..2596fbc 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -1556,7 +1556,8 @@ finish:\n \t}\n \tfor (i = 0; i < num_sg; i++) {\n \t\tif (sg_origin[i]) {\n-\t\t\tdrop_origin_blob(sg_origin[i]);\n+\t\t\tif (!sg_origin[i]->suspects)\n+\t\t\t\tdrop_origin_blob(sg_origin[i]);\n \t\t\torigin_decref(sg_origin[i]);\n \t\t}\n \t}\n-- \n2.7.4\n"},{"id":"287681","messageId":"alpine.DEB.2.20.1605271633230.4449@virtualbox","threadId":"42465","inReplyTo":"1464356141-3797-1-git-send-email-dak@gnu.org","subject":"Re: [PATCH] blame.c: don't drop origin blobs as eagerly","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-05-27T15:00:25Z","receivedAt":"2016-05-27T15:00:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi David,\n\nit is good practice to Cc: the original author of the code in question, in\nthis case Junio. I guess he sees this anyway, but that is really just an\nassumption.\n\nOn Fri, 27 May 2016, David Kastrup wrote:\n\n> When a parent blob already has chunks queued up for blaming, dropping\n> the blob at the end of one blame step will cause it to get reloaded\n> right away, doubling the amount of I/O and unpacking when processing a\n> linear history.\n\nIt is obvious from your commit message that you have studied the code\nquite deeply. To make it easier for the reader (which might be your future\nself), it is advisable to give at least a little bit of introduction, e.g.\nwhat the \"parent blob\" is.\n\nI would *guess* that it is the blob corresponding to the same path in the\nparent of the current revision, but that should be spelled out explicitly.\n\n> Keeping such parent blobs in memory seems like a reasonable\n> optimization.  It's conceivable that this may incur additional memory\n\nThis sentence would be easier to read if \"It's conceivable that\" was\nsimply deleted.\n\n> pressure particularly when the history contains lots of merges from\n> long-diverged branches.  In practice, this optimization appears to\n> behave quite benignly,\n\nWhy not just stop here? I say that because...\n\n> and a viable strategy for limiting the total amount of cached blobs in a\n> useful manner seems rather hard to implement.\n\n... this sounds awfully handwaving. Since we already have reference\ncounting, it sounds fishy to claim that simply piggybacking a global\ncounter on top of it would be \"rather hard\".\n\n> In addition, calling git-blame with -C leads to similar memory retention\n> patterns.\n\nThis is a red herring. Just delete it. I, for one, being a heavy user of\n`git blame`, could count the number of times I used blame's -C option\nwithout any remaining hands. Zero times.\n\nBesides, -C is *supposed* to look harder. By that argument, you could read\nall blobs in rev-list even when the user did not specify --objects\n\"because --objects leads to similar memory retention patterns\". So: let's\njust forget about that statement.\n\nThe commit message is missing your sign-off.\n\nAlso: is there an easy way to reproduce your claims of better I/O\ncharacteristics? Something like a command-line, ideally with a file in\ngit.git's own history, that demonstrates the I/O before and after the\npatch, would be an excellent addition to the commit message.\n\nFurther: I would have at least expected some rudimentary discussion why\nthis patch -- which seems to at least partially contradict 7c3c796 (blame:\ndrop blob data after passing blame to the parent, 2007-12-11) -- is not\nregressing on the intent of said commit.\n\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index 21f42b0..2596fbc 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -1556,7 +1556,8 @@ finish:\n>  \t}\n>  \tfor (i = 0; i < num_sg; i++) {\n>  \t\tif (sg_origin[i]) {\n> -\t\t\tdrop_origin_blob(sg_origin[i]);\n> +\t\t\tif (!sg_origin[i]->suspects)\n> +\t\t\t\tdrop_origin_blob(sg_origin[i]);\n>  \t\t\torigin_decref(sg_origin[i]);\n>  \t\t}\n\nIt would be good to mention in the commit message that this patch does not\nchange anything for blobs with only one remaining reference (the current\none) because origin_decref() would do the same job as drop_origin_blob\nwhen decrementing the reference counter to 0.\n\nIn fact, I suspect that simply removing the drop_origin_blob() call might\nresult in the exact same I/O pattern.\n\nCiao,\nJohannes\n"},{"id":"287683","messageId":"87d1o7pkyy.fsf@fencepost.gnu.org","threadId":"42465","inReplyTo":"alpine.DEB.2.20.1605271633230.4449@virtualbox","subject":"Re: [PATCH] blame.c: don't drop origin blobs as eagerly","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2016-05-27T15:41:09Z","receivedAt":"2016-05-27T15:41:09Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> On Fri, 27 May 2016, David Kastrup wrote:\n>\n>> pressure particularly when the history contains lots of merges from\n>> long-diverged branches.  In practice, this optimization appears to\n>> behave quite benignly,\n>\n> Why not just stop here?\n\nBecause there is a caveat.\n\n> I say that because...\n>\n>> and a viable strategy for limiting the total amount of cached blobs in a\n>> useful manner seems rather hard to implement.\n>\n> ... this sounds awfully handwaving.\n\nBecause it is.\n\n> Since we already have reference counting, it sounds fishy to claim\n> that simply piggybacking a global counter on top of it would be\n> \"rather hard\".\n\nYou'll see that the patch is from 2014.  When I actively worked on it,\nI found no convincing/feasible way to enforce a reasonable hard limit.\nI am not picking up work on this again but am merely flushing my queue\nso that the patch going to waste is not on my conscience.\n\n>> In addition, calling git-blame with -C leads to similar memory retention\n>> patterns.\n>\n> This is a red herring. Just delete it. I, for one, being a heavy user of\n> `git blame`, could count the number of times I used blame's -C option\n> without any remaining hands. Zero times.\n>\n> Besides, -C is *supposed* to look harder.\n\nWe are not talking about \"looking harder\" but \"taking more memory than\nthe set limit\".\n\n> Also: is there an easy way to reproduce your claims of better I/O\n> characteristics? Something like a command-line, ideally with a file in\n> git.git's own history, that demonstrates the I/O before and after the\n> patch, would be an excellent addition to the commit message.\n\nI've used it on the wortliste repository and system time goes down from\nabout 70 seconds to 50 seconds (this is a flash drive).  User time from\nabout 4:20 to 4:00.  It is a rather degenerate repository (predominantly\nsmall changes in one humongous text file) so savings for more typical\ncases might end up less than that.  But then it is degenerate\nrepositories that are most costly to blame.\n\n> Further: I would have at least expected some rudimentary discussion\n> why this patch -- which seems to at least partially contradict 7c3c796\n> (blame: drop blob data after passing blame to the parent, 2007-12-11)\n> -- is not regressing on the intent of said commit.\n\nIt is regressing on the intent of said commit by _retaining_ blob data\nthat it is _sure_ to look at again because of already having this data\nasked for again in the priority queue.  That's the point.  It only drops\nthe blob data for which it has no request queued yet.  But there is no\nhandle on when the request is going to pop up until it actually leaves\nthe priority queue: the priority queue is a heap IIRC and thus only\nprovides partial orderings.\n\n>> diff --git a/builtin/blame.c b/builtin/blame.c\n>> index 21f42b0..2596fbc 100644\n>> --- a/builtin/blame.c\n>> +++ b/builtin/blame.c\n>> @@ -1556,7 +1556,8 @@ finish:\n>>  \t}\n>>  \tfor (i = 0; i < num_sg; i++) {\n>>  \t\tif (sg_origin[i]) {\n>> -\t\t\tdrop_origin_blob(sg_origin[i]);\n>> +\t\t\tif (!sg_origin[i]->suspects)\n>> +\t\t\t\tdrop_origin_blob(sg_origin[i]);\n>>  \t\t\torigin_decref(sg_origin[i]);\n>>  \t\t}\n>\n> It would be good to mention in the commit message that this patch does not\n> change anything for blobs with only one remaining reference (the current\n> one) because origin_decref() would do the same job as drop_origin_blob\n> when decrementing the reference counter to 0.\n\nA sizable number of references but not blobs are retained and needed for\nproducing the information in the final printed output (at least when\nusing the default sequential output instead of --incremental or\n--porcelaine or similar).\n\n> In fact, I suspect that simply removing the drop_origin_blob() call\n> might result in the exact same I/O pattern.\n\nIt's been years since I actually worked on the code but I'm still pretty\nsure you are wrong.\n\n-- \nDavid Kastrup\n"},{"id":"287723","messageId":"alpine.DEB.2.20.1605280815040.4449@virtualbox","threadId":"42465","inReplyTo":"87d1o7pkyy.fsf@fencepost.gnu.org","subject":"Re: [PATCH] blame.c: don't drop origin blobs as eagerly","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-05-28T06:37:14Z","receivedAt":"2016-05-28T06:37:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi David,\n\nOn Fri, 27 May 2016, David Kastrup wrote:\n\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> \n> > On Fri, 27 May 2016, David Kastrup wrote:\n> >\n> >> pressure particularly when the history contains lots of merges from\n> >> long-diverged branches.  In practice, this optimization appears to\n> >> behave quite benignly,\n> >\n> > Why not just stop here?\n> \n> Because there is a caveat.\n> \n> > I say that because...\n> >\n> >> and a viable strategy for limiting the total amount of cached blobs in a\n> >> useful manner seems rather hard to implement.\n> >\n> > ... this sounds awfully handwaving.\n> \n> Because it is.\n\nHrm. Are you really happy with this, on public record?\n\n> > Since we already have reference counting, it sounds fishy to claim\n> > that simply piggybacking a global counter on top of it would be\n> > \"rather hard\".\n> \n> You'll see that the patch is from 2014.\n\nNo I do not. There was no indication of that.\n\n> When I actively worked on it, I found no convincing/feasible way to\n> enforce a reasonable hard limit.  I am not picking up work on this again\n> but am merely flushing my queue so that the patch going to waste is not\n> on my conscience.\n\nHmm. Speaking from my point of view as a maintainer, this raises a yellow\nalert. Sure, I do not maintain git.git, but if I were, I would want my\nconfidence in the quality of this patch, and in the patch author being\naround when things go south with it, strengthened. This paragraph achieves\nthe opposite.\n\n> >> In addition, calling git-blame with -C leads to similar memory retention\n> >> patterns.\n> >\n> > This is a red herring. Just delete it. I, for one, being a heavy user of\n> > `git blame`, could count the number of times I used blame's -C option\n> > without any remaining hands. Zero times.\n> >\n> > Besides, -C is *supposed* to look harder.\n> \n> We are not talking about \"looking harder\" but \"taking more memory than\n> the set limit\".\n\nI meant to say: *of course* it uses more memory, it has a much harder job.\n\n> > Also: is there an easy way to reproduce your claims of better I/O\n> > characteristics? Something like a command-line, ideally with a file in\n> > git.git's own history, that demonstrates the I/O before and after the\n> > patch, would be an excellent addition to the commit message.\n> \n> I've used it on the wortliste repository and system time goes down from\n> about 70 seconds to 50 seconds (this is a flash drive).  User time from\n> about 4:20 to 4:00.  It is a rather degenerate repository (predominantly\n> small changes in one humongous text file) so savings for more typical\n> cases might end up less than that.  But then it is degenerate\n> repositories that are most costly to blame.\n\nWell, obviously I did not mean to measure time, but I/O. Something like an\nstrace call that pipes into some grep that in turn pipes into wc.\n\n> > Further: I would have at least expected some rudimentary discussion\n> > why this patch -- which seems to at least partially contradict 7c3c796\n> > (blame: drop blob data after passing blame to the parent, 2007-12-11)\n> > -- is not regressing on the intent of said commit.\n> \n> It is regressing on the intent of said commit by _retaining_ blob data\n> that it is _sure_ to look at again because of already having this data\n> asked for again in the priority queue.  That's the point.  It only drops\n> the blob data for which it has no request queued yet.  But there is no\n> handle on when the request is going to pop up until it actually leaves\n> the priority queue: the priority queue is a heap IIRC and thus only\n> provides partial orderings.\n\nAgain, this lowers my confidence in the patch. Mind you, the patch could\nbe totally sound! Yet its commit message, and unfortunately even more so\nthe discussion we are having right now, offer no adequate reason to assume\nthat. If you, as the patch author, state that you are not sure what this\nthing does exactly, we must conservatively assume that it contains flaws.\n\n> >> diff --git a/builtin/blame.c b/builtin/blame.c\n> >> index 21f42b0..2596fbc 100644\n> >> --- a/builtin/blame.c\n> >> +++ b/builtin/blame.c\n> >> @@ -1556,7 +1556,8 @@ finish:\n> >>  \t}\n> >>  \tfor (i = 0; i < num_sg; i++) {\n> >>  \t\tif (sg_origin[i]) {\n> >> -\t\t\tdrop_origin_blob(sg_origin[i]);\n> >> +\t\t\tif (!sg_origin[i]->suspects)\n> >> +\t\t\t\tdrop_origin_blob(sg_origin[i]);\n> >>  \t\t\torigin_decref(sg_origin[i]);\n> >>  \t\t}\n> >\n> > It would be good to mention in the commit message that this patch does not\n> > change anything for blobs with only one remaining reference (the current\n> > one) because origin_decref() would do the same job as drop_origin_blob\n> > when decrementing the reference counter to 0.\n> \n> A sizable number of references but not blobs are retained and needed for\n> producing the information in the final printed output (at least when\n> using the default sequential output instead of --incremental or\n> --porcelaine or similar).\n\nSorry, please help me understand how that response relates to my\nsuggestion to improve the commit message.\n\n> > In fact, I suspect that simply removing the drop_origin_blob() call\n> > might result in the exact same I/O pattern.\n> \n> It's been years since I actually worked on the code but I'm still pretty\n> sure you are wrong.\n\nThe short version of your answer is that you will leave this patch in its\ncurrent form and address none of my concerns because you moved on,\ncorrect? If so, that's totally okay, it just needs to be spelled out.\n\nCiao,\nJohannes"},{"id":"287728","messageId":"87shx2oaaw.fsf@fencepost.gnu.org","threadId":"42465","inReplyTo":"alpine.DEB.2.20.1605280815040.4449@virtualbox","subject":"Re: [PATCH] blame.c: don't drop origin blobs as eagerly","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2016-05-28T08:29:11Z","receivedAt":"2016-05-28T08:29:11Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> On Fri, 27 May 2016, David Kastrup wrote:\n>\n>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n>> \n>> > On Fri, 27 May 2016, David Kastrup wrote:\n>> >\n>> >> pressure particularly when the history contains lots of merges from\n>> >> long-diverged branches.  In practice, this optimization appears to\n>> >> behave quite benignly,\n>> >\n>> > Why not just stop here?\n>> \n>> Because there is a caveat.\n>> \n>> > I say that because...\n>> >\n>> >> and a viable strategy for limiting the total amount of cached blobs in a\n>> >> useful manner seems rather hard to implement.\n>> >\n>> > ... this sounds awfully handwaving.\n>> \n>> Because it is.\n>\n> Hrm. Are you really happy with this, on public record?\n\nShrug.  git-blame has limits for its memory usage which it generally\nheeds, throwing away potentially useful data to do so.  git-blame -C has\na store of data retained outside of those limits because nobody bothered\nreining them in, and this patch similarly retains data outside of those\nlimits, though strictly limited to the pending priority queue size which\ncontrols the descent in reverse commit time order.\n\nIn actual measurements on actual heavy-duty repositories I could not\ndiscern a difference in memory usage after the patch.  The difference is\nlikely only noticeable when you have lots of old long branches which is\nnot a typical development model.\n\nThe current data structures don't make harder constraints on memory\npressure feasible, and enforcing such constraints would incur a lot of\nswapping (not by the operating system which does it more efficiently\nparticularly since it does not need to decompress every time, but by the\napplication constantly rereading and recompressing data).\n\ngit-blame -C cares jack-shit about that (and it's used by git-gui's\nsecond pass if I remember correctly) and nobody raised concerns about\nits memory usage ever.\n\nI've taken out a lot of hand-waving of the \"eh, good enough\" kind from\ngit-blame.  If one wanted to have the memory limits enforced more\nstringently than they currently are, one would need a whole new layer of\nFIFO and accounting.  This is not done in the current code base for\nexisting functionality.\n\nAnd nobody bothered implementing it in decades or complaining about it\nin spite of it affecting git-blame -C (and consequently git-gui blame).\n\nYes, this patch is another hand waving beside an already waving crowd.\nI am not going to lie about it but I am also not going to invest the\ntime to fix yet another part of git-blame that has in decades not shown\nto be a problem anybody considered worth reporting let alone fixing.\n\nThe limits for git-blame are set quite conservative: on actual developer\nsystems, exceeding them by a factor of 10 will not exhaust the available\nmemory.  And I could not even measure a difference in memory usage in a\nsmall sample set of large examples.\n\nThat's good enough for me (and better than the shape most of git-blame\nwas in before I started on it and also after I finished), but it's still\nhand-waving.  And it's not like it doesn't have a lot of company.\n\n>> > Since we already have reference counting, it sounds fishy to claim\n>> > that simply piggybacking a global counter on top of it would be\n>> > \"rather hard\".\n>> \n>> You'll see that the patch is from 2014.\n>\n> No I do not. There was no indication of that.\n\nI thought that git-send-email/git-am retained most patch metadata as\nopposed to git-patch, but you are entirely correct that the author date\nis nowhere to be found.  Sorry for the presumption.\n\n>> When I actively worked on it, I found no convincing/feasible way to\n>> enforce a reasonable hard limit.  I am not picking up work on this\n>> again but am merely flushing my queue so that the patch going to\n>> waste is not on my conscience.\n>\n> Hmm. Speaking from my point of view as a maintainer, this raises a\n> yellow alert. Sure, I do not maintain git.git, but if I were, I would\n> want my confidence in the quality of this patch, and in the patch\n> author being around when things go south with it, strengthened. This\n> paragraph achieves the opposite.\n\nIt's completely reasonable to apply the same standards here as with an\nauthor having terminal cancer.  I am not going to be around when things\ngo South with git-blame but I should be very much surprised if this\npatch were significantly involved.\n\n>> > Besides, -C is *supposed* to look harder.\n>> \n>> We are not talking about \"looking harder\" but \"taking more memory\n>> than the set limit\".\n>\n> I meant to say: *of course* it uses more memory, it has a much harder\n> job.\n\nStill a non-sequitur.  If you apply memory limits harder, it will still\nachieve the same job but finish later due to (decompressing) swapping.\n\n>> > Also: is there an easy way to reproduce your claims of better I/O\n>> > characteristics? Something like a command-line, ideally with a file\n>> > in git.git's own history, that demonstrates the I/O before and\n>> > after the patch, would be an excellent addition to the commit\n>> > message.\n>> \n>> I've used it on the wortliste repository and system time goes down\n>> from about 70 seconds to 50 seconds (this is a flash drive).  User\n>> time from about 4:20 to 4:00.  It is a rather degenerate repository\n>> (predominantly small changes in one humongous text file) so savings\n>> for more typical cases might end up less than that.  But then it is\n>> degenerate repositories that are most costly to blame.\n>\n> Well, obviously I did not mean to measure time, but I/O.\n\nSystem time on a SSD is highly correlated with I/O for this task.\nActually, so is system time on a hard drive, it just takes more real\ntime (waiting for I/O to complete) then as well.\n\n>> > Further: I would have at least expected some rudimentary discussion\n>> > why this patch -- which seems to at least partially contradict\n>> > 7c3c796 (blame: drop blob data after passing blame to the parent,\n>> > 2007-12-11) -- is not regressing on the intent of said commit.\n>> \n>> It is regressing on the intent of said commit by _retaining_ blob\n>> data that it is _sure_ to look at again because of already having\n>> this data asked for again in the priority queue.  That's the point.\n>> It only drops the blob data for which it has no request queued yet.\n>> But there is no handle on when the request is going to pop up until\n>> it actually leaves the priority queue: the priority queue is a heap\n>> IIRC and thus only provides partial orderings.\n>\n> Again, this lowers my confidence in the patch. Mind you, the patch\n> could be totally sound! Yet its commit message, and unfortunately even\n> more so the discussion we are having right now, offer no adequate\n> reason to assume that. If you, as the patch author, state that you are\n> not sure what this thing does exactly,\n\nUh, I very definitely know what this thing does exactly.  I never stated\notherwise.\n\n> we must conservatively assume that it contains flaws.\n\nIt has consequences without a theoretical hard limit on memory usage but\nwhere every additional retained blob will provably be needed.\n\nBasically, you don't want to apply this patch because I know what I'm\ndoing and documenting it and the underlying rationale without\nsugar-coating.\n\nThat's perfectly fine with me.  It matches my expectations and\nexperience.\n\n>> >> diff --git a/builtin/blame.c b/builtin/blame.c\n>> >> index 21f42b0..2596fbc 100644\n>> >> --- a/builtin/blame.c\n>> >> +++ b/builtin/blame.c\n>> >> @@ -1556,7 +1556,8 @@ finish:\n>> >>  \t}\n>> >>  \tfor (i = 0; i < num_sg; i++) {\n>> >>  \t\tif (sg_origin[i]) {\n>> >> -\t\t\tdrop_origin_blob(sg_origin[i]);\n>> >> +\t\t\tif (!sg_origin[i]->suspects)\n>> >> +\t\t\t\tdrop_origin_blob(sg_origin[i]);\n>> >>  \t\t\torigin_decref(sg_origin[i]);\n>> >>  \t\t}\n>> >\n>> > It would be good to mention in the commit message that this patch does not\n>> > change anything for blobs with only one remaining reference (the current\n>> > one) because origin_decref() would do the same job as drop_origin_blob\n>> > when decrementing the reference counter to 0.\n>> \n>> A sizable number of references but not blobs are retained and needed for\n>> producing the information in the final printed output (at least when\n>> using the default sequential output instead of --incremental or\n>> --porcelaine or similar).\n>\n> Sorry, please help me understand how that response relates to my\n> suggestion to improve the commit message.\n\nBlobs and origins don't have separate reference counts.  This patch has\nthe same effect as maintaining such separate counts explicitly.  Since\nthe majority of commits lead to some lines in the current source (apart\nfrom commits being weeded out by later refactoring/reimplementation),\nthis means that the majority of origins never reach a reference count of\n0 before the end of the run.\n\n>> > In fact, I suspect that simply removing the drop_origin_blob() call\n>> > might result in the exact same I/O pattern.\n>> \n>> It's been years since I actually worked on the code but I'm still\n>> pretty sure you are wrong.\n>\n> The short version of your answer is that you will leave this patch in\n> its current form and address none of my concerns because you moved on,\n> correct? If so, that's totally okay, it just needs to be spelled out.\n\nYes, that's it.  You'll notice that the code change itself is both\nminuscule as well purely functional, so it contains nothing\ncopyrightable.  I'm perfectly fine with anybody else writing whatever\ncommit message he wants based on whatever additional testing he wants\nand claiming whole credit for what indeed amounts to being the actually\nhard work, given the Git maintainership which is dependent on judging\nquality by how a patch author advertises his code when the author is not\na frequent contributor.  I've not authored more than a dozen or two\npatches spread out over more of a decade.  If you do git-blame on them,\nI doubt you'll find any that ever caused problems or that called for\nrefactoring/replacement.\n\nNevertheless, I clearly am not a regular and different standards apply.\nI'm fine with that.  It's off my lawn, my responsibility, and my\nconscience.\n\n-- \nDavid Kastrup\n"},{"id":"287732","messageId":"alpine.DEB.2.20.1605281430440.4449@virtualbox","threadId":"42465","inReplyTo":"87shx2oaaw.fsf@fencepost.gnu.org","subject":"Re: [PATCH] blame.c: don't drop origin blobs as eagerly","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-05-28T12:34:35Z","receivedAt":"2016-05-28T12:34:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi David,\n\nOn Sat, 28 May 2016, David Kastrup wrote:\n\n> > The short version of your answer is that you will leave this patch in\n> > its current form and address none of my concerns because you moved on,\n> > correct? If so, that's totally okay, it just needs to be spelled out.\n> \n> Yes, that's it.  You'll notice that the code change itself is both\n> minuscule as well purely functional, so it contains nothing\n> copyrightable.\n\nThat is unfortunately an interpretation of the law that would need to come\nfrom a professional lawyer. As it is, the patch was clearly authored by\nyou, and anybody else who would claim authorship would most likely be\nbreaking the law.\n\nSo I won't touch it.\n\nSorry,\nJohannes\n"},{"id":"287735","messageId":"87bn3qnuyy.fsf@fencepost.gnu.org","threadId":"42465","inReplyTo":"alpine.DEB.2.20.1605281430440.4449@virtualbox","subject":"Re: [PATCH] blame.c: don't drop origin blobs as eagerly","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2016-05-28T14:00:21Z","receivedAt":"2016-05-28T14:00:21Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> Hi David,\n>\n> On Sat, 28 May 2016, David Kastrup wrote:\n>\n>> > The short version of your answer is that you will leave this patch in\n>> > its current form and address none of my concerns because you moved on,\n>> > correct? If so, that's totally okay, it just needs to be spelled out.\n>> \n>> Yes, that's it.  You'll notice that the code change itself is both\n>> minuscule as well purely functional, so it contains nothing\n>> copyrightable.\n>\n> That is unfortunately an interpretation of the law that would need to\n> come from a professional lawyer.\n\nA professional lawyer would laugh at \"Signed-off-by:\" being of more\nlegal significance than a written record of intent but of course you\nknow that.  This is mere bluster.\n\n> As it is, the patch was clearly authored by you, and anybody else who\n> would claim authorship would most likely be breaking the law.\n\nThe _diff_ is not \"clearly authored\" by me but just the simplest\nexpression of the intent.  The commit message is clearly authored by me\nbut is not acceptable anyway.  Whoever gets to write an acceptable\ncommit message is up for all copyrightable credit in my book.  Feel free\nto keep the authorship if you really want to, but when replacing the\ncommit message it is not a particularly accurate attribution.\n\n> So I won't touch it.\n\nSigned-off-by: David Kastrup <dak@gnu.org>\n\nYou don't get more than that for other patches either and it's a few\nbytes compared to a mail conversation.  Here is a PGP signature on top.\n\nAs I said: I am not going to put more work into it anyway and if it's an\noccasion for theatralics, it has at least accomplished something.\n\n-- \nDavid Kastrup\n"}]}