{"thread":{"id":"21287","subject":"[PATCH] blame: make sure that the last line ends in an LF","startedAt":"2009-10-20T03:06:28Z","lastAt":"2009-10-22T01:33:56Z","messageCount":9,"participants":["Sverre Rabbelier","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"125437","messageId":"1256007988-13321-1-git-send-email-srabbelier@gmail.com","threadId":"21287","inReplyTo":null,"subject":"[PATCH] blame: make sure that the last line ends in an LF","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2009-10-20T03:06:28Z","receivedAt":"2009-10-20T03:06:28Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"From: Johannes Schindelin \n\nThis is convenient when parsing multiple the blame of multiple files,\nfor example:\n\n git ls-files -z --exclude-standard -- \"*.[ch]\" |\n xargs --null -n 1 git blame -p > output\n\nand then analyzing the 'output' file using a seperate script.\n\nCurrently the parsing is difficult when not all files have a newline\nat EOF, this patch ensures that even such files have a newline at the\nend of the blame output.\n\nSigned-off-by: Sverre Rabbelier \nCC: Johannes Schindelin \n---\n\n Patch by Dscho, commit message by me. Apologies to Dscho for\n taking so long to send it :).\n\n builtin-blame.c | 6 ++++++\n 1 files changed, 6 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-blame.c b/builtin-blame.c\nindex 7512773..dd16b22 100644\n--- a/builtin-blame.c\n+++ b/builtin-blame.c\n@@ -1604,6 +1604,9 @@ static void emit_porcelain(struct scoreboard *sb, struct blame_entry *ent)\n \t\t} while (ch != '\\n' &&\n \t\t\t cp < sb->final_buf + sb->final_buf_size);\n \t}\n+\n+\tif (sb->final_buf_size && cp[-1] != '\\n')\n+\t\tputchar('\\n');\n }\n \n static void emit_other(struct scoreboard *sb, struct blame_entry *ent, int opt)\n@@ -1667,6 +1670,9 @@ static void emit_other(struct scoreboard *sb, struct blame_entry *ent, int opt)\n \t\t} while (ch != '\\n' &&\n \t\t\t cp < sb->final_buf + sb->final_buf_size);\n \t}\n+\n+\tif (sb->final_buf_size && cp[-1] != '\\n')\n+\t\tputchar('\\n');\n }\n \n static void output(struct scoreboard *sb, int option)\n-- \n1.6.5.1.123.ge01f7\n"},{"id":"125448","messageId":"7vbpk2sg6m.fsf@alter.siamese.dyndns.org","threadId":"21287","inReplyTo":"1256007988-13321-1-git-send-email-srabbelier@gmail.com","subject":"Re: [PATCH] blame: make sure that the last line ends in an LF","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-10-20T07:00:33Z","receivedAt":"2009-10-20T07:00:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sverre Rabbelier writes:\n\n> Currently the parsing is difficult when not all files have a newline\n> at EOF, this patch ensures that even such files have a newline at the\n> end of the blame output.\n\nThanks. This clearly shows that blame was somewhat sloppily written in\nthat it never considered to be fed anything but well formed text files.\n\nMy knee-jerk reaction to the issue is that there are two reasonable\napproaches to the problem:\n\n (1) Admit that the code is not prepared to take anything but well formed\n text files. This will lead to adding the necessary LF after reading\n a blob and if it does not end with LF. After all, I do not trust the\n code (iow, \"me\") if it is not prepared to take a blob with incomplete\n line to handle the internal comparison between blobs with incomplete\n lines.\n\n (2) Do the right thing, by coming up with a notation to show that the\n final line is incomplete, perhaps similar to \"\\No newline ...\"\n notation used by \"diff\".\n\nTo put the last sentence of (1) differently, does the code assign blame\ncorrectly around the last line of the original blob? What if an older\nversion ended with an incomplete line and a later version changed the line\n(without adding the terminating LF)? What if a later version changed the\nline and added the terminating LF? What if a later version only added the\nterminating LF and did nothing else? Are these three cases handled\ncorrectly?\n\nAfter thinking issues like the above, I read the patch and I see it does\nnot take neither approach. That makes me feel nervous.\n\nBy tweaking only the output routine you _might_ be getting correct output,\nbut even then it looks to me like the end result is working correctly not\nby design but by accident. IOW, the patch may be better than nothing, but\nsomehow it just feels like it is papering over the real issue than being a\nproper fix.\n\nOr am I worrying too much?\n\n> Signed-off-by: Sverre Rabbelier \n> CC: Johannes Schindelin \n> ---\n>\n> Patch by Dscho, commit message by me. Apologies to Dscho for\n> taking so long to send it :).\n>\n> builtin-blame.c | 6 ++++++\n> 1 files changed, 6 insertions(+), 0 deletions(-)\n>\n> diff --git a/builtin-blame.c b/builtin-blame.c\n> index 7512773..dd16b22 100644\n> --- a/builtin-blame.c\n> +++ b/builtin-blame.c\n> @@ -1604,6 +1604,9 @@ static void emit_porcelain(struct scoreboard *sb, struct blame_entry *ent)\n> \t\t} while (ch != '\\n' &&\n> \t\t\t cp < sb->final_buf + sb->final_buf_size);\n> \t}\n> +\n> +\tif (sb->final_buf_size && cp[-1] != '\\n')\n> +\t\tputchar('\\n');\n> }\n> \n> static void emit_other(struct scoreboard *sb, struct blame_entry *ent, int opt)\n> @@ -1667,6 +1670,9 @@ static void emit_other(struct scoreboard *sb, struct blame_entry *ent, int opt)\n> \t\t} while (ch != '\\n' &&\n> \t\t\t cp < sb->final_buf + sb->final_buf_size);\n> \t}\n> +\n> +\tif (sb->final_buf_size && cp[-1] != '\\n')\n> +\t\tputchar('\\n');\n> }\n> \n> static void output(struct scoreboard *sb, int option)\n> -- \n> 1.6.5.1.123.ge01f7\n"},{"id":"125474","messageId":"fabb9a1e0910200615x5d487cdk6f50e11c96f2cb6c@mail.gmail.com","threadId":"21287","inReplyTo":"7vbpk2sg6m.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] blame: make sure that the last line ends in an LF","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2009-10-20T13:15:26Z","receivedAt":"2009-10-20T13:15:26Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Tue, Oct 20, 2009 at 02:00, Junio C Hamano wrote:\n>  (2) Do the right thing, by coming up with a notation to show that the\n>     final line is incomplete, perhaps similar to \"\\No newline ...\"\n>     notation used by \"diff\".\n\nWhat about 'git blame -p', can we just go about changing the format like that?\n\nFor purpose of the discussion below let's assume we squash in the following:\n\n-- <8 --\n\ndiff --git a/builtin-blame.c b/builtin-blame.c\nindex dd16b22..cf492a0 100644\n--- a/builtin-blame.c\n+++ b/builtin-blame.c\n@@ -1606,7 +1606,7 @@ static void emit_porcelain(struct scoreboard\n*sb, struct blame_entry *ent)\n \t}\n\n \tif (sb->final_buf_size && cp[-1] != '\\n')\n-\t\tputchar('\\n');\n+\t\tprintf(\"\\n\\\\ No newline at end of file\\n\");\n }\n\n static void emit_other(struct scoreboard *sb, struct blame_entry *ent, int opt)\n@@ -1672,7 +1672,7 @@ static void emit_other(struct scoreboard *sb,\nstruct blame_entry *ent, int opt)\n \t}\n\n \tif (sb->final_buf_size && cp[-1] != '\\n')\n-\t\tputchar('\\n');\n+\t\tprintf(\"\\n\\\\ No newline at end of file\\n\");\n }\n\n static void output(struct scoreboard *sb, int option)\n-- \n\n-- <8 --\n\n> Does the code assign blame\n> correctly around the last line of the original blob?\n\nYes, it does, when there is no trailing newline an extra \"\\ No newline\nat end of file\" is printed, but the last line is still attributed\ncorrectly.\n\n> What if an older\n> version ended with an incomplete line and a later version changed the line\n> (without adding the terminating LF)?\n\nNothing changes, the blame on that last line is attributed correctly\nand the \"\\ No newline at end of file\" is printed.\n\n> What if a later version changed the\n> line and added the terminating LF?\n\nThe trailing \"\\ No newline at end of file\" is no longer printed and\nthe last line is correctly attributed to the commit that added the\ntrailing LF.\n\n> What if a later version only added the\n> terminating LF and did nothing else?  Are these three cases handled\n> correctly?\n\nSame as above.\n\n> After thinking issues like the above, I read the patch and I see it does\n> not take neither approach. That makes me feel nervous.\n\nReading your reply I see that if you care about the presence (or\nabsence) of a trailing newline the current patch would be problematic,\nas it makes it impossible to see in the blame output whether there was\na trailing newline or not.\n\n> By tweaking only the output routine you _might_ be getting correct output,\n> but even then it looks to me like the end result is working correctly not\n> by design but by accident. IOW, the patch may be better than nothing, but\n> somehow it just feels like it is papering over the real issue than being a\n> proper fix.\n>\n> Or am I worrying too much?\n\nNo, I think your concerns are valid, we should go with (2) and DTRT.\nDoes the updated patch address your concerns? If so I can send a new\nversion.\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"125494","messageId":"7vbpk2ovio.fsf@alter.siamese.dyndns.org","threadId":"21287","inReplyTo":"fabb9a1e0910200615x5d487cdk6f50e11c96f2cb6c@mail.gmail.com","subject":"Re: [PATCH] blame: make sure that the last line ends in an LF","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-10-20T16:55:11Z","receivedAt":"2009-10-20T16:55:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sverre Rabbelier writes:\n\n>> Does the code assign blame\n>> correctly around the last line of the original blob?\n> \n> Yes, it does,...\n\nThat is kind of surprising ;-) as I do remember that I never thought about\nthis issue of dealing with the incomplete lines while writing the blame\nalgorithm. I actually didn't even think about \"well this will not work\nbecause I am ignoring the incomplete lines\".\n\n>> Or am I worrying too much?\n>\n> No, I think your concerns are valid, we should go with (2) and DTRT.\n> Does the updated patch address your concerns? If so I can send a new\n> version.\n\nAssuming the internal blame algorithm correctly works with such an input,\nI'd be happier with an approach to allow users to tell the difference.\nThe --porcelain output was designed to be extensible, and it might make\nsense to show the \"this line is incomplete\" as a separate bit, though.\n"},{"id":"125512","messageId":"alpine.DEB.1.00.0910202153160.4985@pacific.mpi-cbg.de","threadId":"21287","inReplyTo":"7vbpk2ovio.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] blame: make sure that the last line ends in an LF","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-10-20T19:55:36Z","receivedAt":"2009-10-20T19:55:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 20 Oct 2009, Junio C Hamano wrote:\n\n> Sverre Rabbelier writes:\n> \n> >> Or am I worrying too much?\n> >\n> > No, I think your concerns are valid, we should go with (2) and DTRT. \n> > Does the updated patch address your concerns? If so I can send a new \n> > version.\n> \n> Assuming the internal blame algorithm correctly works with such an \n> input, I'd be happier with an approach to allow users to tell the \n> difference. The --porcelain output was designed to be extensible, and it \n> might make sense to show the \"this line is incomplete\" as a separate \n> bit, though.\n\nSorry, you lost me. If, say, line 614 is the last line that does not end \nin a new line, if I ask for it to be blamed, I want to know who is \nresponsible for the current form of line 614.\n\nNot whether the line ends in a new line or not.\n\nCiao,\nDscho\n"},{"id":"125515","messageId":"fabb9a1e0910201304j2730e6f3j1ebc5c17be07dd12@mail.gmail.com","threadId":"21287","inReplyTo":"7vbpk2ovio.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] blame: make sure that the last line ends in an LF","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2009-10-20T20:04:05Z","receivedAt":"2009-10-20T20:04:05Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Tue, Oct 20, 2009 at 11:55, Junio C Hamano wrote:\n> That is kind of surprising ;-) as I do remember that I never thought about\n> this issue of dealing with the incomplete lines while writing the blame\n> algorithm.  I actually didn't even think about \"well this will not work\n> because I am ignoring the incomplete lines\".\n\nI used the following repo for testing:\n\n$ git init\nInitialized empty Git repository in /home/sverre/code/test/.git/\n$ echo \"first line\" > test\n$ git add test\n$ git commit -m \"initial\"\n[master (root-commit) d573d06] initial\n 1 files changed, 1 insertions(+), 0 deletions(-)\n create mode 100644 test\n$ echo -n \"second line, no newline\" >> test\n$ git add test\n$ git commit -m \"second\"\n[master 76ad2f9] second\n 1 files changed, 1 insertions(+), 0 deletions(-)\n\nRegular output looks good:\n\n$ git blame test\n^d573d06 (Sverre Rabbelier 2009-10-20 12:30:56 -0500 1) first line\n76ad2f90 (Sverre Rabbelier 2009-10-20 12:31:57 -0500 2) second line, no newline\n\nPorcelain output looks good too:\n\n$ git blame -p test\nd573d06f0dd50148ba8e59bf8f1ef8fa7ee9fc88 1 1 1\nauthor Sverre Rabbelier\nauthor-mail \nauthor-time 1256059856\nauthor-tz -0500\ncommitter Sverre Rabbelier\ncommitter-mail \ncommitter-time 1256059856\ncommitter-tz -0500\nsummary initial\nboundary\nfilename test\n first line\n76ad2f90bde689a65715e37afd37d45942c74954 2 2 1\nauthor Sverre Rabbelier\nauthor-mail \nauthor-time 1256059917\nauthor-tz -0500\ncommitter Sverre Rabbelier\ncommitter-mail \ncommitter-time 1256059917\ncommitter-tz -0500\nsummary second\nprevious d573d06f0dd50148ba8e59bf8f1ef8fa7ee9fc88 test\nfilename test\n second line, no newline\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"125517","messageId":"7viqe9n72w.fsf@alter.siamese.dyndns.org","threadId":"21287","inReplyTo":"fabb9a1e0910201304j2730e6f3j1ebc5c17be07dd12@mail.gmail.com","subject":"Re: [PATCH] blame: make sure that the last line ends in an LF","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-10-20T20:28:23Z","receivedAt":"2009-10-20T20:28:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sverre Rabbelier writes:\n\n> On Tue, Oct 20, 2009 at 11:55, Junio C Hamano wrote:\n>> That is kind of surprising ;-) as I do remember that I never thought about\n>> this issue of dealing with the incomplete lines while writing the blame\n>> algorithm.  I actually didn't even think about \"well this will not work\n>> because I am ignoring the incomplete lines\".\n>\n> I used the following repo for testing:\n>\n> $ git init\n> Initialized empty Git repository in /home/sverre/code/test/.git/\n> $ echo \"first line\" > test\n> $ git add test\n> $ git commit -m \"initial\"\n> [master (root-commit) d573d06] initial\n> 1 files changed, 1 insertions(+), 0 deletions(-)\n> create mode 100644 test\n> $ echo -n \"second line, no newline\" >> test\n> $ git add test\n> $ git commit -m \"second\"\n> [master 76ad2f9] second\n> 1 files changed, 1 insertions(+), 0 deletions(-)\n>\n> Regular output looks good:\n>\n> $ git blame test\n> ^d573d06 (Sverre Rabbelier 2009-10-20 12:30:56 -0500 1) first line\n> 76ad2f90 (Sverre Rabbelier 2009-10-20 12:31:57 -0500 2) second line, no newline\n>\n> Porcelain output looks good too:\n>\n> $ git blame -p test\n> d573d06f0dd50148ba8e59bf8f1ef8fa7ee9fc88 1 1 1\n> author Sverre Rabbelier\n> author-mail \n> author-time 1256059856\n> author-tz -0500\n> committer Sverre Rabbelier\n> committer-mail \n> committer-time 1256059856\n> committer-tz -0500\n> summary initial\n> boundary\n> filename test\n> first line\n> 76ad2f90bde689a65715e37afd37d45942c74954 2 2 1\n> author Sverre Rabbelier\n> author-mail \n> author-time 1256059917\n> author-tz -0500\n> committer Sverre Rabbelier\n> committer-mail \n> committer-time 1256059917\n> committer-tz -0500\n> summary second\n> previous d573d06f0dd50148ba8e59bf8f1ef8fa7ee9fc88 test\n> filename test\n> second line, no newline\n\n\nThanks.\n\nFor both styles of output, adding an extra LF after \"no newline\" would be\nnecessary to make the output legible (for human) and parsable (for\nscripts).\n\nIn addition, it would help Porcelains to re-construct the final text if\nyou added a boolean \"incomplete-line\" (put it on its own line, immediately\nafter \"filename test\" line). Then they will know that LF after \"second\nline, no newline\" was not there in the original and was added for\nparsability.\n\nI am not sure what we want to do for non-porcelain output (other than\nadding the extra LF at the end). Assuming that they are meant to be read\nby humans (and casual scripts that do not bother reading --porcelain\nformat), it might be best not to add any extra marking.\n"},{"id":"125537","messageId":"7vfx9dlo8u.fsf@alter.siamese.dyndns.org","threadId":"21287","inReplyTo":"alpine.DEB.1.00.0910202153160.4985@pacific.mpi-cbg.de","subject":"Re: [PATCH] blame: make sure that the last line ends in an LF","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-10-20T22:00:33Z","receivedAt":"2009-10-20T22:00:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Hi,\n>\n> On Tue, 20 Oct 2009, Junio C Hamano wrote:\n>\n>> Sverre Rabbelier writes:\n>> \n>> >> Or am I worrying too much?\n>> >\n>> > No, I think your concerns are valid, we should go with (2) and DTRT. \n>> > Does the updated patch address your concerns? If so I can send a new \n>> > version.\n>> \n>> Assuming the internal blame algorithm correctly works with such an \n>> input, I'd be happier with an approach to allow users to tell the \n>> difference. The --porcelain output was designed to be extensible, and it \n>> might make sense to show the \"this line is incomplete\" as a separate \n>> bit, though.\n>\n> Sorry, you lost me. If, say, line 614 is the last line that does not end \n> in a new line, if I ask for it to be blamed, I want to know who is \n> responsible for the current form of line 614.\n>\n> Not whether the line ends in a new line or not.\n\nYeah, I know.\n\nI was primarily worried about making the output format into something\nPorcelains (that read from --porcelain format) cannot reconstruct the\noriginal text from.\n\nSee Message-ID: <7viqe9n72w.fsf@alter.siamese.dyndns.org> for a revised\nsuggestion.\n"},{"id":"125670","messageId":"fabb9a1e0910211833qb1315aan89f43a7b5b51c556@mail.gmail.com","threadId":"21287","inReplyTo":"7viqe9n72w.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] blame: make sure that the last line ends in an LF","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2009-10-22T01:33:56Z","receivedAt":"2009-10-22T01:33:56Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Tue, Oct 20, 2009 at 15:28, Junio C Hamano wrote:\n> For both styles of output, adding an extra LF after \"no newline\" would be\n> necessary to make the output legible (for human) and parsable (for\n> scripts).\n\nYou mean like this right?\n\n```\n+ printf(\"\\n\\\\ No newline at end of file\\n\");\n```\n\nOr does it need _another_ newline, like this\n\n```\n+ printf(\"\\n\\\\ No newline at end of file\\n\\n\");\n```\n\n> In addition, it would help Porcelains to re-construct the final text if\n> you added a boolean \"incomplete-line\" (put it on its own line, immediately\n> after \"filename test\" line).  Then they will know that LF after \"second\n> line, no newline\" was not there in the original and was added for\n> parsability.\n\nWhat do we do in the case that the last few lines are attributed to\nthe same commit? Do we just signify 'incomplete line' to mean that the\nlast one of those is incomplete?\n\n> I am not sure what we want to do for non-porcelain output (other than\n> adding the extra LF at the end).  Assuming that they are meant to be read\n> by humans (and casual scripts that do not bother reading --porcelain\n> format), it might be best not to add any extra marking.\n\nFine by me :).\n\n-- \nCheers,\n\nSverre Rabbelier\n"}]}