{"thread":{"id":"35690","subject":"[PATCH 0/2] Two janitorial patches for builtin/blame.c","startedAt":"2014-01-19T18:57:48Z","lastAt":"2014-01-21T23:02:45Z","messageCount":17,"participants":["David Kastrup","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"233386","messageId":"1390157870-29795-1-git-send-email-dak@gnu.org","threadId":"35690","inReplyTo":null,"subject":"[PATCH 0/2] Two janitorial patches for builtin/blame.c","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2014-01-19T18:57:48Z","receivedAt":"2014-01-19T18:57:48Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"This is more a warmup than anything else: I'm actually doing a quite\nmore involved rewrite of git-blame right now.  But it's been a long\ntime since I sent patches for Git, so I'm starting out with something\nreasonably uncontroversial.  Patch 1 is a no-brainer: maintaining\nreverse links is not worth the trouble if you are not going to use\nthem.  Now one can be \"conservative\" and say \"but git-blame is awfully\ninefficient and maybe we'll need them in a more efficient version\".\nI can answer this with \"no\": the kind of stuff that would make things\nmore efficient does not require backlinks.\n\nPatch 2 is a bit more tricky: basically its contention is that\nI understand some implications of the code better than its author\nappeared to do.  Which is somewhat optimistic.  Since my followup work\ndepends on my understanding this correctly, it's better to make sure\nof that by handing in a nicely isolated patch for review.  It's\nconceivable that my understanding of the commit->util cache is not\nfully satisfactory.  I don't use it in my followup work anyway, but it\nstill would be nice to get this code path cleaned out in advance.\n\nI don't expect measurable performance improvements from these two\npatches: their main purpose is to get some cruft out of the way so\nthat the heavy-duty patches actually dealing with the performance\nsinks will be a bit more focused.\n\nAnd of course, getting the ball rolling again.\n\nDavid Kastrup (2):\n  builtin/blame.c: struct blame_entry does not need a prev link\n  Eliminate same_suspect function in builtin/blame.c\n\n builtin/blame.c | 38 ++++++++++----------------------------\n 1 file changed, 10 insertions(+), 28 deletions(-)\n\n-- \n1.8.3.2\n"},{"id":"233388","messageId":"1390157870-29795-2-git-send-email-dak@gnu.org","threadId":"35690","inReplyTo":"1390157870-29795-1-git-send-email-dak@gnu.org","subject":"[PATCH 1/2] builtin/blame.c: struct blame_entry does not need a prev link","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2014-01-19T18:57:49Z","receivedAt":"2014-01-19T18:57:49Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"---\n builtin/blame.c | 13 ++-----------\n 1 file changed, 2 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex e44a6bb..2195595 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -197,7 +197,6 @@ static void drop_origin_blob(struct origin *o)\n  * scoreboard structure, sorted by the target line number.\n  */\n struct blame_entry {\n-\tstruct blame_entry *prev;\n \tstruct blame_entry *next;\n \n \t/* the first line of this group in the final image;\n@@ -282,8 +281,6 @@ static void coalesce(struct scoreboard *sb)\n \t\t    ent->s_lno + ent->num_lines == next->s_lno) {\n \t\t\tent->num_lines += next->num_lines;\n \t\t\tent->next = next->next;\n-\t\t\tif (ent->next)\n-\t\t\t\tent->next->prev = ent;\n \t\t\torigin_decref(next->suspect);\n \t\t\tfree(next);\n \t\t\tent->score = 0;\n@@ -534,7 +531,7 @@ static void add_blame_entry(struct scoreboard *sb, struct blame_entry *e)\n \t\tprev = ent;\n \n \t/* prev, if not NULL, is the last one that is below e */\n-\te->prev = prev;\n+\n \tif (prev) {\n \t\te->next = prev->next;\n \t\tprev->next = e;\n@@ -543,8 +540,6 @@ static void add_blame_entry(struct scoreboard *sb, struct blame_entry *e)\n \t\te->next = sb->ent;\n \t\tsb->ent = e;\n \t}\n-\tif (e->next)\n-\t\te->next->prev = e;\n }\n \n /*\n@@ -555,14 +550,12 @@ static void add_blame_entry(struct scoreboard *sb, struct blame_entry *e)\n  */\n static void dup_entry(struct blame_entry *dst, struct blame_entry *src)\n {\n-\tstruct blame_entry *p, *n;\n+\tstruct blame_entry *n;\n \n-\tp = dst->prev;\n \tn = dst->next;\n \torigin_incref(src->suspect);\n \torigin_decref(dst->suspect);\n \tmemcpy(dst, src, sizeof(*src));\n-\tdst->prev = p;\n \tdst->next = n;\n \tdst->score = 0;\n }\n@@ -2502,8 +2495,6 @@ parse_done:\n \t\tent->suspect = o;\n \t\tent->s_lno = bottom;\n \t\tent->next = next;\n-\t\tif (next)\n-\t\t\tnext->prev = ent;\n \t\torigin_incref(o);\n \t}\n \torigin_decref(o);\n-- \n1.8.3.2\n"},{"id":"233387","messageId":"1390157870-29795-3-git-send-email-dak@gnu.org","threadId":"35690","inReplyTo":"1390157870-29795-1-git-send-email-dak@gnu.org","subject":"[PATCH 2/2] Eliminate same_suspect function in builtin/blame.c","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2014-01-19T18:57:50Z","receivedAt":"2014-01-19T18:57:50Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Since the origin pointers are \"interned\" and reference-counted, comparing\nthe pointers rather than the content is enough.  The only uninterned\norigins are cached values kept in commit->util, but same_suspect is not\ncalled on them.\n---\n builtin/blame.c | 25 ++++++++-----------------\n 1 file changed, 8 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 2195595..ead6148 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -255,15 +255,6 @@ struct scoreboard {\n \tint *lineno;\n };\n \n-static inline int same_suspect(struct origin *a, struct origin *b)\n-{\n-\tif (a == b)\n-\t\treturn 1;\n-\tif (a->commit != b->commit)\n-\t\treturn 0;\n-\treturn !strcmp(a->path, b->path);\n-}\n-\n static void sanity_check_refcnt(struct scoreboard *);\n \n /*\n@@ -276,7 +267,7 @@ static void coalesce(struct scoreboard *sb)\n \tstruct blame_entry *ent, *next;\n \n \tfor (ent = sb->ent; ent && (next = ent->next); ent = next) {\n-\t\tif (same_suspect(ent->suspect, next->suspect) &&\n+\t\tif (ent->suspect == next->suspect &&\n \t\t    ent->guilty == next->guilty &&\n \t\t    ent->s_lno + ent->num_lines == next->s_lno) {\n \t\t\tent->num_lines += next->num_lines;\n@@ -735,7 +726,7 @@ static int find_last_in_target(struct scoreboard *sb, struct origin *target)\n \tint last_in_target = -1;\n \n \tfor (e = sb->ent; e; e = e->next) {\n-\t\tif (e->guilty || !same_suspect(e->suspect, target))\n+\t\tif (e->guilty || e->suspect != target)\n \t\t\tcontinue;\n \t\tif (last_in_target < e->s_lno + e->num_lines)\n \t\t\tlast_in_target = e->s_lno + e->num_lines;\n@@ -755,7 +746,7 @@ static void blame_chunk(struct scoreboard *sb,\n \tstruct blame_entry *e;\n \n \tfor (e = sb->ent; e; e = e->next) {\n-\t\tif (e->guilty || !same_suspect(e->suspect, target))\n+\t\tif (e->guilty || e->suspect != target)\n \t\t\tcontinue;\n \t\tif (same <= e->s_lno)\n \t\t\tcontinue;\n@@ -985,7 +976,7 @@ static int find_move_in_parent(struct scoreboard *sb,\n \twhile (made_progress) {\n \t\tmade_progress = 0;\n \t\tfor (e = sb->ent; e; e = e->next) {\n-\t\t\tif (e->guilty || !same_suspect(e->suspect, target) ||\n+\t\t\tif (e->guilty || e->suspect != target ||\n \t\t\t    ent_score(sb, e) < blame_move_score)\n \t\t\t\tcontinue;\n \t\t\tfind_copy_in_blob(sb, e, parent, split, &file_p);\n@@ -1020,14 +1011,14 @@ static struct blame_list *setup_blame_list(struct scoreboard *sb,\n \n \tfor (e = sb->ent, num_ents = 0; e; e = e->next)\n \t\tif (!e->scanned && !e->guilty &&\n-\t\t    same_suspect(e->suspect, target) &&\n+\t\t    e->suspect == target &&\n \t\t    min_score < ent_score(sb, e))\n \t\t\tnum_ents++;\n \tif (num_ents) {\n \t\tblame_list = xcalloc(num_ents, sizeof(struct blame_list));\n \t\tfor (e = sb->ent, i = 0; e; e = e->next)\n \t\t\tif (!e->scanned && !e->guilty &&\n-\t\t\t    same_suspect(e->suspect, target) &&\n+\t\t\t    e->suspect == target &&\n \t\t\t    min_score < ent_score(sb, e))\n \t\t\t\tblame_list[i++].ent = e;\n \t}\n@@ -1171,7 +1162,7 @@ static void pass_whole_blame(struct scoreboard *sb,\n \t\torigin->file.ptr = NULL;\n \t}\n \tfor (e = sb->ent; e; e = e->next) {\n-\t\tif (!same_suspect(e->suspect, origin))\n+\t\tif (e->suspect != origin)\n \t\t\tcontinue;\n \t\torigin_incref(porigin);\n \t\torigin_decref(e->suspect);\n@@ -1560,7 +1551,7 @@ static void assign_blame(struct scoreboard *sb, int opt)\n \n \t\t/* Take responsibility for the remaining entries */\n \t\tfor (ent = sb->ent; ent; ent = ent->next)\n-\t\t\tif (same_suspect(ent->suspect, suspect))\n+\t\t\tif (ent->suspect == suspect)\n \t\t\t\tfound_guilty_entry(ent);\n \t\torigin_decref(suspect);\n \n-- \n1.8.3.2\n"},{"id":"233440","messageId":"87d2jlqp7x.fsf@fencepost.gnu.org","threadId":"35690","inReplyTo":"1390157870-29795-1-git-send-email-dak@gnu.org","subject":"Re: [PATCH 0/2] Two janitorial patches for builtin/blame.c","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2014-01-21T16:22:42Z","receivedAt":"2014-01-21T16:22:42Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"David Kastrup <dak@gnu.org> writes:\n\n> This is more a warmup than anything else: I'm actually doing a quite\n> more involved rewrite of git-blame right now.  But it's been a long\n> time since I sent patches for Git, so I'm starting out with something\n> reasonably uncontroversial.\n\nPing?\n\nNow I might have sent at an unopportune time: blame.c is mostly\nattributed to Junio who seems to have been a few days absent now.\n\nI also have seen quite a few mails and patch submissions on the list go\nbasically unanswered in the last few days.\n\nSo it might just be that this is business as usual.  However, since\nI have not been on this list for quite a while, I would want to avoid\ncausing large delays by some oversight.\n\nI have not so far signed off on the patches: it would appear that this\nis required.  The submission guidelines in\nDocumentation/SubmittingPatches state for signing off:\n\n        (a) The contribution was created in whole or in part by me and I\n            have the right to submit it under the open source license\n            indicated in the file; or\n\n[...]\n\n        (d) I understand and agree that this project and the contribution\n            are public and that a record of the contribution (including all\n            personal information I submit with it, including my sign-off) is\n            maintained indefinitely and may be redistributed consistent with\n            this project or the open source license(s) involved.\n\nNow the file involved (builtin/blame.c) itself does not state _any_\nlicense.  Instead it states\n\n    /*\n     * Blame\n     *\n     * Copyright (c) 2006, Junio C Hamano\n     */\n\nI do not intend my contribution to constitute a copyright assignment\n(and it hardly could be one).  The top file COPYING in Git states\n\n     Note that the only valid version of the GPL as far as this project\n     is concerned is _this_ particular version of the license (ie v2, not\n     v2.2 or v3.x or whatever), unless explicitly otherwise stated.\n\n     HOWEVER, in order to allow a migration to GPLv3 if that seems like\n     a good idea, I also ask that people involved with the project make\n     their preferences known. In particular, if you trust me to make that\n     decision, you might note so in your copyright message, ie something\n     like\n\n            This file is licensed under the GPL v2, or a later version\n            at the discretion of Linus.\n\n      might avoid issues. But we can also just decide to synchronize and\n      contact all copyright holders on record if/when the occasion arises.\n\nAs far as I am concerned, I am willing to license my work under the\nGPLv2 or any later version at the discretion of whoever wants to work\nwith it.  I think that should be compatible with the project goals.\n\nNow the above passage states \"you might note so in your copyright\nmessage\", but my patches do not even contain a copyright message and it\nis not clear to me that they should, or that there is a sensible place\nto place such \"copyright messages\".\n\nSo any guidance about that?\n\n-- \nDavid Kastrup\n"},{"id":"233441","messageId":"20140121165546.GE18964@google.com","threadId":"35690","inReplyTo":"87d2jlqp7x.fsf@fencepost.gnu.org","subject":"Re: [PATCH 0/2] Two janitorial patches for builtin/blame.c","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-01-21T16:55:46Z","receivedAt":"2014-01-21T16:55:46Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"David Kastrup wrote:\n\n> Now I might have sent at an unopportune time: blame.c is mostly\n> attributed to Junio who seems to have been a few days absent now.\n>\n> I also have seen quite a few mails and patch submissions on the list go\n> basically unanswered in the last few days.\n\nIn the U.S., yesterday was a federal holiday (Martin Luther King, Jr.\nday) and the two days before were the weekend.\n\n[...]\n>             maintained indefinitely and may be redistributed consistent with\n>             this project or the open source license(s) involved.\n>\n> Now the file involved (builtin/blame.c) itself does not state _any_\n> license.\n\nMost of git is GPLv2-only.  (As an aside, if there's interest then I'd\nbe happy to see most of it change to GPLv2-or-later since that makes\nit possible to link to code under the Apache License.  But I'm also\nhappy with the status quo.)\n\n[...]\n> As far as I am concerned, I am willing to license my work under the\n> GPLv2 or any later version at the discretion of whoever wants to work\n> with it.  I think that should be compatible with the project goals.\n>\n> Now the above passage states \"you might note so in your copyright\n> message\", but my patches do not even contain a copyright message and it\n> is not clear to me that they should, or that there is a sensible place\n> to place such \"copyright messages\".\n\nYeah, since these patches aren't adding a large new chunk of code\nthere's no need for a new copyright notice and so no place to put that\nkind of thing unless Junio wants to relicense blame (which would be\northogonal to these patches).\n\nThanks and hope that helps,\nJonathan\n"},{"id":"233444","messageId":"874n4xqlly.fsf@fencepost.gnu.org","threadId":"35690","inReplyTo":"20140121165546.GE18964@google.com","subject":"Re: [PATCH 0/2] Two janitorial patches for builtin/blame.c","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2014-01-21T17:40:41Z","receivedAt":"2014-01-21T17:40:41Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> David Kastrup wrote:\n>\n>> Now I might have sent at an unopportune time: blame.c is mostly\n>> attributed to Junio who seems to have been a few days absent now.\n>>\n>> I also have seen quite a few mails and patch submissions on the list go\n>> basically unanswered in the last few days.\n>\n> In the U.S., yesterday was a federal holiday (Martin Luther King, Jr.\n> day) and the two days before were the weekend.\n\nI see.\n\n>> Now the file involved (builtin/blame.c) itself does not state _any_\n>> license.\n>\n> Most of git is GPLv2-only.\n\nDoes that include builtin/blame.c?\n\n> Yeah, since these patches aren't adding a large new chunk of code\n\nWell, _significant_ chunks of code are in the works already (and my\nquestion was really more about them).\n\n> there's no need for a new copyright notice and so no place to put that\n> kind of thing unless Junio wants to relicense blame (which would be\n> orthogonal to these patches).\n\nSo my understanding is that when we are talking about _significant_\nadditions to builtin/blame.c (the current patches don't qualify as such\nreally) that\n\na) builtin/blame.c is licensed under GPLv2\nb) significant contributions to it will not be relicensed under\ndifferent licenses without the respective contributors' explicit\nconsent.\n\nThis question is not academical to me.  I don't have any source of\nincome apart from people paying me to write free software (mainly\nLilyPond users), so if I'm writing significant pieces of code, I don't\nwant to see them distributed as proprietary software (for example, by\ntraveling through the very unrestrictively licensed libgit2) without\nbeing in the situation of negotiating recompensation for that.\n\nThe combination of the SubmittingPatches text with the file notices in\nbuiltin/blame.c is not really painting a full picture of the situation.\n\n-- \nDavid Kastrup\n"},{"id":"233446","messageId":"20140121174448.GG18964@google.com","threadId":"35690","inReplyTo":"874n4xqlly.fsf@fencepost.gnu.org","subject":"Re: [PATCH 0/2] Two janitorial patches for builtin/blame.c","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-01-21T17:44:48Z","receivedAt":"2014-01-21T17:44:48Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"David Kastrup wrote:\n\n> So my understanding is that when we are talking about _significant_\n> additions to builtin/blame.c (the current patches don't qualify as such\n> really) that\n>\n> a) builtin/blame.c is licensed under GPLv2\n> b) significant contributions to it will not be relicensed under\n> different licenses without the respective contributors' explicit\n> consent.\n\nYep, that's how it works.\n\n[...]\n> The combination of the SubmittingPatches text with the file notices in\n> builtin/blame.c is not really painting a full picture of the situation.\n\nAny idea how this could be made more clear?  E.g., maybe we should\nbite the bullet and add a line to all source files that don't already\nstate a license:\n\n\t/*\n\t * License: GPLv2.  See COPYING for details.\n\t */\n"},{"id":"233447","messageId":"87zjmpp672.fsf@fencepost.gnu.org","threadId":"35690","inReplyTo":"20140121174448.GG18964@google.com","subject":"Re: [PATCH 0/2] Two janitorial patches for builtin/blame.c","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2014-01-21T17:58:57Z","receivedAt":"2014-01-21T17:58:57Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> David Kastrup wrote:\n>\n>> So my understanding is that when we are talking about _significant_\n>> additions to builtin/blame.c (the current patches don't qualify as such\n>> really) that\n>>\n>> a) builtin/blame.c is licensed under GPLv2\n>> b) significant contributions to it will not be relicensed under\n>> different licenses without the respective contributors' explicit\n>> consent.\n>\n> Yep, that's how it works.\n>\n> [...]\n>> The combination of the SubmittingPatches text with the file notices in\n>> builtin/blame.c is not really painting a full picture of the situation.\n>\n> Any idea how this could be made more clear?  E.g., maybe we should\n> bite the bullet and add a line to all source files that don't already\n> state a license:\n>\n> \t/*\n> \t * License: GPLv2.  See COPYING for details.\n> \t */\n\nProbably somewhat more verbose like \"This file may be distributed under\nthe conditions of the GPLv2.  See the file COPYING for details\".\nI think there are boilerplate texts for that.\n\nWhatever the exact wording, that would be the cleanest way I think.  The\nrespective Documentation/SubmittingPatches text looks like it is quoted\nfrom somewhere else, so adapting it to the realities of files without\nclear copyright statement seems less straightforward.\n\n-- \nDavid Kastrup\n"},{"id":"233449","messageId":"20140121191531.GH18964@google.com","threadId":"35690","inReplyTo":"87zjmpp672.fsf@fencepost.gnu.org","subject":"Re: [PATCH 0/2] Two janitorial patches for builtin/blame.c","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-01-21T19:15:31Z","receivedAt":"2014-01-21T19:15:31Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"David Kastrup wrote:\n> Jonathan Nieder <jrnieder@gmail.com> writes:\n\n>> Any idea how this could be made more clear?  E.g., maybe we should\n>> bite the bullet and add a line to all source files that don't already\n>> state a license:\n>>\n>> \t/*\n>> \t * License: GPLv2.  See COPYING for details.\n>> \t */\n>\n> Probably somewhat more verbose like \"This file may be distributed under\n> the conditions of the GPLv2.  See the file COPYING for details\".\n> I think there are boilerplate texts for that.\n\nAll else being equal, longer is worse.\n\n> Whatever the exact wording, that would be the cleanest way I think.  The\n> respective Documentation/SubmittingPatches text looks like it is quoted\n> from somewhere else, so adapting it to the realities of files without\n> clear copyright statement seems less straightforward.\n\nHm, the wording comes from the Linux kernel project, where it's also\npretty normal not to have a license notice in every file (and where\nthe default license is also GPLv2).\n\nIs the problem the phrase \"indicated in the file\", or is the problem\ne.g. the lack of a pointer to\nhttps://github.com/libgit2/libgit2/blob/development/git.git-authors?\n\nJonathan\n"},{"id":"233454","messageId":"20140121195353.GI18964@google.com","threadId":"35690","inReplyTo":"874n4xqlly.fsf@fencepost.gnu.org","subject":"Re: [PATCH 0/2] Two janitorial patches for builtin/blame.c","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-01-21T19:53:53Z","receivedAt":"2014-01-21T19:53:53Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"David Kastrup wrote:\n\n> The combination of the SubmittingPatches text with the file notices in\n> builtin/blame.c is not really painting a full picture of the situation.\n\nBTW, thanks for bringing this up.  It last came up at [1].  Perhaps we\ncan do better by adding a note to README or some similar file\nexplaining that git is under the GPLv2 and files use that license when\nnot otherwise specified.\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/234705/focus=234709\n"},{"id":"233455","messageId":"87r481p0rt.fsf@fencepost.gnu.org","threadId":"35690","inReplyTo":"20140121191531.GH18964@google.com","subject":"Re: [PATCH 0/2] Two janitorial patches for builtin/blame.c","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2014-01-21T19:56:06Z","receivedAt":"2014-01-21T19:56:06Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> David Kastrup wrote:\n>> Jonathan Nieder <jrnieder@gmail.com> writes:\n>\n>>> Any idea how this could be made more clear?  E.g., maybe we should\n>>> bite the bullet and add a line to all source files that don't already\n>>> state a license:\n>>>\n>>> \t/*\n>>> \t * License: GPLv2.  See COPYING for details.\n>>> \t */\n>>\n>> Probably somewhat more verbose like \"This file may be distributed under\n>> the conditions of the GPLv2.  See the file COPYING for details\".\n>> I think there are boilerplate texts for that.\n>\n> All else being equal, longer is worse.\n\nI am not sure that all else is equal.\n\n>> Whatever the exact wording, that would be the cleanest way I think.  The\n>> respective Documentation/SubmittingPatches text looks like it is quoted\n>> from somewhere else, so adapting it to the realities of files without\n>> clear copyright statement seems less straightforward.\n>\n> Hm, the wording comes from the Linux kernel project, where it's also\n> pretty normal not to have a license notice in every file (and where\n> the default license is also GPLv2).\n>\n> Is the problem the phrase \"indicated in the file\",\n\nAt least that's what I perceive as a problem in combination with the\ncomplete absence of any such notice in the file I am contributing to.\n\ngit grep -i license\n\nactually shows a dearth of licensing information outside of subprojects\nand contrib.  The README file states\n\n    Git is an Open Source project covered by the GNU General Public\n    License version 2 (some parts of it are under different licenses,\n    compatible with the GPLv2). It was originally written by Linus\n    Torvalds with help of a group of hackers around the net.\n\nwithout mentioning _which_ parts are under different licenses.  The\nlicense file COPYING itself does not specify which files are covered,\nand there is _also_ LGPL-2.1 which has a statement\n\n     While most of this project is under the GPL (see COPYING), the\n     xdiff/ library and some libc code from compat/ are licensed under\n     the GNU LGPL, version 2.1 or (at your option) any later version and\n     some other files are under other licenses.  Check the individual\n     files to be sure.\n\nWell, and when checking the individual files, there is really nothing\nto be found for \"being sure\".\n\nThe net result is that when signing off on a patch according to the\nrules in Documentation/SubmittingPatches, for most files you don't\nreally have a definite statement just _what_ license you are agreeing\nyour work to be distributed under.\n\n> or is the problem\n> e.g. the lack of a pointer to\n> https://github.com/libgit2/libgit2/blob/development/git.git-authors?\n\nNo, not at all.  libgit2 is not in any way special among projects that\nmight want to have access to Git code under different licenses.  It\nwould be possible to state something like \"Unless indicated otherwise,\nconsent will be assumed for contributions to Git as being\nredistributable in the libgit2 project under its respective licenses\" or\nsomething, but I think that would be seriously surprising, and not\nnoticing such a clause could not be construed as implying consent.\n\n-- \nDavid Kastrup\n"},{"id":"233456","messageId":"20140121200149.GJ18964@google.com","threadId":"35690","inReplyTo":"87r481p0rt.fsf@fencepost.gnu.org","subject":"Re: [PATCH 0/2] Two janitorial patches for builtin/blame.c","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-01-21T20:01:49Z","receivedAt":"2014-01-21T20:01:49Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"David Kastrup wrote:\n\n> and contrib.  The README file states\n>\n>     Git is an Open Source project covered by the GNU General Public\n>     License version 2 (some parts of it are under different licenses,\n>     compatible with the GPLv2). It was originally written by Linus\n>     Torvalds with help of a group of hackers around the net.\n>\n> without mentioning _which_ parts are under different licenses.\n\nOkay, how about this patch?\n\ndiff --git i/README w/README\nindex 15a8e23..6745db5 100644\n--- i/README\n+++ w/README\n@@ -21,8 +21,9 @@ and full access to internals.\n \n Git is an Open Source project covered by the GNU General Public\n License version 2 (some parts of it are under different licenses,\n-compatible with the GPLv2). It was originally written by Linus\n-Torvalds with help of a group of hackers around the net.\n+compatible with the GPLv2, and have notices to that effect). It was\n+originally written by Linus Torvalds with help of a group of hackers\n+around the net.\n \n Please read the file INSTALL for installation instructions.\n \n"},{"id":"233458","messageId":"xmqqa9epulwq.fsf@gitster.dls.corp.google.com","threadId":"35690","inReplyTo":"20140121174448.GG18964@google.com","subject":"Re: [PATCH 0/2] Two janitorial patches for builtin/blame.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-21T20:20:37Z","receivedAt":"2014-01-21T20:20:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> David Kastrup wrote:\n>\n>> So my understanding is that when we are talking about _significant_\n>> additions to builtin/blame.c (the current patches don't qualify as such\n>> really) that\n>>\n>> a) builtin/blame.c is licensed under GPLv2\n>> b) significant contributions to it will not be relicensed under\n>> different licenses without the respective contributors' explicit\n>> consent.\n>\n> Yep, that's how it works.\n>\n> [...]\n>> The combination of the SubmittingPatches text with the file notices in\n>> builtin/blame.c is not really painting a full picture of the situation.\n>\n> Any idea how this could be made more clear?  E.g., maybe we should\n> bite the bullet and add a line to all source files that don't already\n> state a license:\n>\n> \t/*\n> \t * License: GPLv2.  See COPYING for details.\n> \t */\n\nI vaguely recall that jgit folks at one point wanted to lift this\nimplementation and were interested in seeing it to be dual licensed\nto BSD but that was a long time ago.\n\n  http://git.661346.n2.nabble.com/JGIT-Blame-functionality-for-jgit-td2142726.html\n"},{"id":"233462","messageId":"87mwipowep.fsf@fencepost.gnu.org","threadId":"35690","inReplyTo":"20140121200149.GJ18964@google.com","subject":"Re: [PATCH 0/2] Two janitorial patches for builtin/blame.c","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2014-01-21T21:30:22Z","receivedAt":"2014-01-21T21:30:22Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> David Kastrup wrote:\n>\n>> and contrib.  The README file states\n>>\n>>     Git is an Open Source project covered by the GNU General Public\n>>     License version 2 (some parts of it are under different licenses,\n>>     compatible with the GPLv2). It was originally written by Linus\n>>     Torvalds with help of a group of hackers around the net.\n>>\n>> without mentioning _which_ parts are under different licenses.\n>\n> Okay, how about this patch?\n>\n> diff --git i/README w/README\n> index 15a8e23..6745db5 100644\n> --- i/README\n> +++ w/README\n> @@ -21,8 +21,9 @@ and full access to internals.\n>  \n>  Git is an Open Source project covered by the GNU General Public\n>  License version 2 (some parts of it are under different licenses,\n> -compatible with the GPLv2). It was originally written by Linus\n> -Torvalds with help of a group of hackers around the net.\n> +compatible with the GPLv2, and have notices to that effect). It was\n> +originally written by Linus Torvalds with help of a group of hackers\n> +around the net.\n\nClearer.  I think it would be most accurate to state:\n\n    Git is an Open Source project covered by the GNU General Public\n    License version 2.  Those parts of it which may be also be\n    distributed under other licenses contain notices to that effect.\n\nThe point is that as a whole, the software is distributed under GPLv2\n(that's what \"compatible\" licensing actually means since the GPL demands\ndistribution of the software \"as a whole\" under the GPL) but parts of it\nmay optionally be distributed under other licenses.\n\n-- \nDavid Kastrup\n"},{"id":"233465","messageId":"xmqqlhy9t2z1.fsf@gitster.dls.corp.google.com","threadId":"35690","inReplyTo":"1390157870-29795-2-git-send-email-dak@gnu.org","subject":"Re: [PATCH 1/2] builtin/blame.c: struct blame_entry does not need a prev link","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-21T21:54:58Z","receivedAt":"2014-01-21T21:54:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Kastrup <dak@gnu.org> writes:\n\n> ---\n\nThanks.  At some point during its development I must have thought\nthat having it as a dual-linked list may make it easier when we have\nto split a block into pieces, but it seems that split_overlap() does\nnot need to look at this information.\n\nNeeds sign-off.\n\n\n>  builtin/blame.c | 13 ++-----------\n>  1 file changed, 2 insertions(+), 11 deletions(-)\n>\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index e44a6bb..2195595 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -197,7 +197,6 @@ static void drop_origin_blob(struct origin *o)\n>   * scoreboard structure, sorted by the target line number.\n>   */\n>  struct blame_entry {\n> -\tstruct blame_entry *prev;\n>  \tstruct blame_entry *next;\n>  \n>  \t/* the first line of this group in the final image;\n> @@ -282,8 +281,6 @@ static void coalesce(struct scoreboard *sb)\n>  \t\t    ent->s_lno + ent->num_lines == next->s_lno) {\n>  \t\t\tent->num_lines += next->num_lines;\n>  \t\t\tent->next = next->next;\n> -\t\t\tif (ent->next)\n> -\t\t\t\tent->next->prev = ent;\n>  \t\t\torigin_decref(next->suspect);\n>  \t\t\tfree(next);\n>  \t\t\tent->score = 0;\n> @@ -534,7 +531,7 @@ static void add_blame_entry(struct scoreboard *sb, struct blame_entry *e)\n>  \t\tprev = ent;\n>  \n>  \t/* prev, if not NULL, is the last one that is below e */\n> -\te->prev = prev;\n> +\n>  \tif (prev) {\n>  \t\te->next = prev->next;\n>  \t\tprev->next = e;\n> @@ -543,8 +540,6 @@ static void add_blame_entry(struct scoreboard *sb, struct blame_entry *e)\n>  \t\te->next = sb->ent;\n>  \t\tsb->ent = e;\n>  \t}\n> -\tif (e->next)\n> -\t\te->next->prev = e;\n>  }\n>  \n>  /*\n> @@ -555,14 +550,12 @@ static void add_blame_entry(struct scoreboard *sb, struct blame_entry *e)\n>   */\n>  static void dup_entry(struct blame_entry *dst, struct blame_entry *src)\n>  {\n> -\tstruct blame_entry *p, *n;\n> +\tstruct blame_entry *n;\n>  \n> -\tp = dst->prev;\n>  \tn = dst->next;\n>  \torigin_incref(src->suspect);\n>  \torigin_decref(dst->suspect);\n>  \tmemcpy(dst, src, sizeof(*src));\n> -\tdst->prev = p;\n>  \tdst->next = n;\n>  \tdst->score = 0;\n>  }\n> @@ -2502,8 +2495,6 @@ parse_done:\n>  \t\tent->suspect = o;\n>  \t\tent->s_lno = bottom;\n>  \t\tent->next = next;\n> -\t\tif (next)\n> -\t\t\tnext->prev = ent;\n>  \t\torigin_incref(o);\n>  \t}\n>  \torigin_decref(o);\n"},{"id":"233473","messageId":"87iotdosey.fsf@fencepost.gnu.org","threadId":"35690","inReplyTo":"xmqqa9epulwq.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 0/2] Two janitorial patches for builtin/blame.c","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2014-01-21T22:56:37Z","receivedAt":"2014-01-21T22:56:37Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Jonathan Nieder <jrnieder@gmail.com> writes:\n>\n>> David Kastrup wrote:\n>>\n>>> So my understanding is that when we are talking about _significant_\n>>> additions to builtin/blame.c (the current patches don't qualify as such\n>>> really) that\n>>>\n>>> a) builtin/blame.c is licensed under GPLv2\n>>> b) significant contributions to it will not be relicensed under\n>>> different licenses without the respective contributors' explicit\n>>> consent.\n>>\n>> Yep, that's how it works.\n>>\n>> [...]\n>>> The combination of the SubmittingPatches text with the file notices in\n>>> builtin/blame.c is not really painting a full picture of the situation.\n>>\n>> Any idea how this could be made more clear?  E.g., maybe we should\n>> bite the bullet and add a line to all source files that don't already\n>> state a license:\n>>\n>> \t/*\n>> \t * License: GPLv2.  See COPYING for details.\n>> \t */\n>\n> I vaguely recall that jgit folks at one point wanted to lift this\n> implementation and were interested in seeing it to be dual licensed\n> to BSD but that was a long time ago.\n>\n>   http://git.661346.n2.nabble.com/JGIT-Blame-functionality-for-jgit-td2142726.html\n\nOk, let me state quite clearly before we waste time, energy and\ngoodwill:\n\na) I am reworking the core logic of blame.c to make it produce the same\nresults while being orders of magnitude faster.  Git's current\nimplementation is a roadblock for serious use.  Keeping its current core\nalgorithms and data flow, it would have been reasonably easy to speed\nthe current code up by a factor of 2 or more by doing local\noptimizations.  But I've chosen _not_ to keep the current logic and data\nflow.  That means quite a bit more work, and it means completely\nunderstanding the existing code before being able to replace it.\n\nThe core part of blame.c spends literally billions of iterations in\nreal-life situations leafing through one large linear list for tiny bits\nof information.  One could use a better searchable data structure and\nspeed up the access in that manner, but better than a fast search is no\nsearch at all.  I am separating the data so that at any given time I am\nonly accessing actually relevant data.  O(n) beats O(n lg n), and the\ncode remains almost as readable as the current O(n^2).\n\nb) This will require thoroughly reworking the core parts of the\nalgorithm which will then be about 50/50 old and new code that cannot\nsensibly be separated since significant parts of the previous code will\nbe gone completely as the data flow is fundamentally different.\n\nc) The \"fine points\" of blame.c, in particular all the various command\nline options and the implementation of their exact meaning would stay\nthe same.  I hope I can avoid touching more than 50% of the code.\n\nd) I am fine with distributing my work under the GPLv2 or later, but no\nother license will be implied.  While this does not affect the core Git\ndistribution itself: for distribution under more permissive licenses for\nthe purpose of making inclusion in proprietary software possible, I'd\nprobably attach a big price tag that reflects the amount of work and\nquality of code going in and the fact that I have no other source of\nincome.\n\ne) No idea whether this would affect JGIT: it depends on how much JGIT\nwould be a literal translation of blame.c into Java (?) or a\nfunctionally equivalent rewrite employing different and/or native data\nstructures to achieve the same effect.  To me it's irritating that\nsomething like the fine but boring points of option parsing might be\nmore susceptible to copyright protection than doing a careful\nalgorithmic design, but that's the way the world is wired.\n\nAt any rate: JGIT or not, I'll be contributing work with the\nunderstanding that it will be licensed under the _current_ licensing\nscheme of Git.  And I think that's a reasonable expectation.\n\n-- \nDavid Kastrup\n"},{"id":"233475","messageId":"87eh41os4q.fsf@fencepost.gnu.org","threadId":"35690","inReplyTo":"xmqqlhy9t2z1.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/2] builtin/blame.c: struct blame_entry does not need a prev link","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2014-01-21T23:02:45Z","receivedAt":"2014-01-21T23:02:45Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> David Kastrup <dak@gnu.org> writes:\n>\n>> ---\n>\n> Thanks.  At some point during its development I must have thought\n> that having it as a dual-linked list may make it easier when we have\n> to split a block into pieces, but it seems that split_overlap() does\n> not need to look at this information.\n>\n> Needs sign-off.\n\nWell, as I said: it's quite possible that the double-linking might be\nuseful for some particular hypothetical rewrite of the code.  It isn't\nfor the current code, and it's not useful for my own rewrite.\n\nWill be posting a signed-off version presently.\n\n-- \nDavid Kastrup\n"}]}