{"thread":{"id":"23992","subject":"[PATCH 0/4] Don't warn about missing EOL for symlinks","startedAt":"2010-06-03T14:35:19Z","lastAt":"2010-06-07T08:10:40Z","messageCount":22,"participants":["Michael J Gruber","Matthieu Moy","Jeff King","Junio C Hamano","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"142885","messageId":"cover.1275575236.git.git@drmicha.warpmail.net","threadId":"23992","inReplyTo":null,"subject":"[PATCH 0/4] Don't warn about missing EOL for symlinks","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2010-06-03T14:35:19Z","receivedAt":"2010-06-03T14:35:19Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"\"No newline at end of file\" always confuses me when looking at a diff for\nsymlinks. \"File? Huh? Didn't Git recognize my symlink?\"\n\nThis mini series refactors the diff/xdiff code slightly to have the two\noccurences of the warning both in diff.c, introduces a whitespace flag\nWS_NO_EOL_AT_EOF to make this warning configurable, and actually changes\nthe default behaviour to what is described thorougly along with the\nrationale in 3/4.\n\n4/4 is a RFC - do we need to tweak this from config like other whitespace\nwarnings? 4/4 is lacking the Doc change for now.\n\nMichael J Gruber (4):\n diff/xdiff: refactor EOF-EOL detection\n diff: make treatment of missing EOL at EOF configurable\n diff: Do not warn about missing EOL at EOF for symlinks\n RFC: add whitespace rule for no-eol-at-eof\n\n cache.h | 3 ++-\n diff.c | 7 ++++++-\n t/t4030-diff-textconv.sh | 1 -\n ws.c | 1 +\n xdiff/xutils.c | 5 -----\n 5 files changed, 9 insertions(+), 8 deletions(-)\n"},{"id":"142886","messageId":"08e635cee993d97e2a38d7766ced11c064ef7d87.1275575236.git.git@drmicha.warpmail.net","threadId":"23992","inReplyTo":"cover.1275575236.git.git@drmicha.warpmail.net","subject":"[PATCH 1/4] diff/xdiff: refactor EOF-EOL detection","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2010-06-03T14:35:20Z","receivedAt":"2010-06-03T14:35:20Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"This puts the \"No newline at end of file\" warning for both code paths\n(emit_rewrite_lines for emit_rewrite_diff and fn_out_consume for\nxdi_diff_outf) into diff.c so that it is easier to adjust later on.\n\nSigned-off-by: Michael J Gruber \n---\n diff.c | 2 ++\n xdiff/xutils.c | 5 -----\n 2 files changed, 2 insertions(+), 5 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 4e07744..ff44bcc 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -917,6 +917,8 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)\n \t\tecbdata->lno_in_postimage++;\n \t\temit_add_line(reset, ecbdata, line + 1, len - 1);\n \t}\n+\tif (line[len-1] != '\\n')\n+\t\tfputs(\"\\n\\\\ No newline at end of file\\n\", ecbdata->file);\n }\n \n static char *pprint_rename(const char *a, const char *b)\ndiff --git a/xdiff/xutils.c b/xdiff/xutils.c\nindex bc12f29..feb43a9 100644\n--- a/xdiff/xutils.c\n+++ b/xdiff/xutils.c\n@@ -51,11 +51,6 @@ int xdl_emit_diffrec(char const *rec, long size, char const *pre, long psize,\n \tmb[0].size = psize;\n \tmb[1].ptr = (char *) rec;\n \tmb[1].size = size;\n-\tif (size > 0 && rec[size - 1] != '\\n') {\n-\t\tmb[2].ptr = (char *) \"\\n\\\\ No newline at end of file\\n\";\n-\t\tmb[2].size = strlen(mb[2].ptr);\n-\t\ti++;\n-\t}\n \tif (ecb->outf(ecb->priv, mb, i) < 0) {\n \n \t\treturn -1;\n-- \n1.7.1.351.ge2633e\n"},{"id":"142889","messageId":"77f4ffc1e89c4a5e34c74882dcf4761b486f5b48.1275575236.git.git@drmicha.warpmail.net","threadId":"23992","inReplyTo":"cover.1275575236.git.git@drmicha.warpmail.net","subject":"[PATCH 2/4] diff: make treatment of missing EOL at EOF configurable","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2010-06-03T14:35:21Z","receivedAt":"2010-06-03T14:35:21Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Make this one of the WS_* flags so that we can later turn the warning\nfor missing EOL at EOF on and off as per config or blob type.\n\nSigned-off-by: Michael J Gruber \n---\n cache.h | 3 ++-\n diff.c | 5 +++--\n 2 files changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex c966023..dc72b8f 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1048,8 +1048,9 @@ void shift_tree_by(const unsigned char *, const unsigned char *, unsigned char *\n #define WS_CR_AT_EOL 010\n #define WS_BLANK_AT_EOF 020\n #define WS_TAB_IN_INDENT 040\n+#define WS_NO_EOL_AT_EOF 0100\n #define WS_TRAILING_SPACE (WS_BLANK_AT_EOL|WS_BLANK_AT_EOF)\n-#define WS_DEFAULT_RULE (WS_TRAILING_SPACE|WS_SPACE_BEFORE_TAB)\n+#define WS_DEFAULT_RULE (WS_TRAILING_SPACE|WS_SPACE_BEFORE_TAB|WS_NO_EOL_AT_EOF)\n extern unsigned whitespace_rule_cfg;\n extern unsigned whitespace_rule(const char *);\n extern unsigned parse_whitespace_rule(const char *);\ndiff --git a/diff.c b/diff.c\nindex ff44bcc..7950df6 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -455,7 +455,7 @@ static void emit_rewrite_lines(struct emit_callback *ecb,\n \t\tsize -= len;\n \t\tdata += len;\n \t}\n-\tif (!endp) {\n+\tif (!endp && (ecb->ws_rule & WS_NO_EOL_AT_EOF)) {\n \t\tconst char *plain = diff_get_color(ecb->color_diff,\n \t\t\t\t\t\t DIFF_PLAIN);\n \t\temit_line_0(ecb->file, plain, reset, '\\\\',\n@@ -918,7 +918,8 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)\n \t\temit_add_line(reset, ecbdata, line + 1, len - 1);\n \t}\n \tif (line[len-1] != '\\n')\n-\t\tfputs(\"\\n\\\\ No newline at end of file\\n\", ecbdata->file);\n+\t\tfputs((ecbdata->ws_rule & WS_NO_EOL_AT_EOF)\n+\t\t\t? \"\\n\\\\ No newline at end of file\\n\" : \"\\n\", ecbdata->file);\n }\n \n static char *pprint_rename(const char *a, const char *b)\n-- \n1.7.1.351.ge2633e\n"},{"id":"142887","messageId":"c1e0646449aae50fb962108d16b5428fb7d73bae.1275575236.git.git@drmicha.warpmail.net","threadId":"23992","inReplyTo":"cover.1275575236.git.git@drmicha.warpmail.net","subject":"[PATCH 3/4] diff: Do not warn about missing EOL at EOF for symlinks","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2010-06-03T14:35:22Z","receivedAt":"2010-06-03T14:35:22Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Symbolic links are represented by blobs without a trailing newline\n(EOL). This cannot be changed because it would encumber the\nrepresentation with platform EOL issues.\n\nBut seeing a warning about \"No newline at end of file\" in the diff for\na symbolic link is highly distracting - in particular, it distracts from\nthe actual mode (120000) and gives the false impression that Git treated\nthe symlink as a file.\n\nTherefore, change the default behaviour of the diff code so that there\nis no warning about missing EOL at EOF if a symlink is involved AND\ntextconv is allowed. So, --no-textconv restores the old behaviour, and\ndiff users such as format-patch are not affected by this change so that\napply/am are still able to create files with no EOL at EOF - such as\nsymlink representations.\n\nNote that a mode change (file<->symlink) is always rewritten as\ndeletion+addition by the diff engine so that replacing a file without\nEOL at EOF by a symlink or vice versa displays the warning for the file.\n\nSigned-off-by: Michael J Gruber \n---\n diff.c | 2 ++\n t/t4030-diff-textconv.sh | 1 -\n 2 files changed, 2 insertions(+), 1 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 7950df6..cad7339 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1788,6 +1788,8 @@ static void builtin_diff(const char *name_a,\n \t\tecbdata.color_diff = DIFF_OPT_TST(o, COLOR_DIFF);\n \t\tecbdata.found_changesp = &o->found_changes;\n \t\tecbdata.ws_rule = whitespace_rule(name_b ? name_b : name_a);\n+\t\tif (DIFF_OPT_TST(o, ALLOW_TEXTCONV) && (S_ISLNK(one->mode) || S_ISLNK(two->mode)))\n+\t\t\tecbdata.ws_rule &= ~WS_NO_EOL_AT_EOF;\n \t\tif (ecbdata.ws_rule & WS_BLANK_AT_EOF)\n \t\t\tcheck_blank_at_eof(&mf1, &mf2, &ecbdata);\n \t\tecbdata.file = o->file;\ndiff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh\nindex 88c5619..b6e537f 100755\n--- a/t/t4030-diff-textconv.sh\n+++ b/t/t4030-diff-textconv.sh\n@@ -109,7 +109,6 @@ index 0000000..67be421\n +++ b/file\n @@ -0,0 +1 @@\n +frotz\n-\\ No newline at end of file\n EOF\n # make a symlink the hard way that works on symlink-challenged file systems\n test_expect_success 'textconv does not act on symlinks' '\n-- \n1.7.1.351.ge2633e\n"},{"id":"142888","messageId":"5ef4da3db5914e9161edcf3933263f936ca56702.1275575236.git.git@drmicha.warpmail.net","threadId":"23992","inReplyTo":"cover.1275575236.git.git@drmicha.warpmail.net","subject":"[PATCH 4/4] RFC: add whitespace rule for no-eol-at-eof","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2010-06-03T14:35:23Z","receivedAt":"2010-06-03T14:35:23Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"I really don't know if it's worth to do this one.\n\nNo doc so far.\nAlso, as it is, -no-eol-at-eof turns around the default for files but\nnot for symlinks.\n\nI really don't know...\n\nSigned-off-by: Michael J Gruber \n---\n ws.c | 1 +\n 1 files changed, 1 insertions(+), 0 deletions(-)\n\ndiff --git a/ws.c b/ws.c\nindex d7b8c33..73502c8 100644\n--- a/ws.c\n+++ b/ws.c\n@@ -20,6 +20,7 @@ static struct whitespace_rule {\n \t{ \"blank-at-eol\", WS_BLANK_AT_EOL, 0 },\n \t{ \"blank-at-eof\", WS_BLANK_AT_EOF, 0 },\n \t{ \"tab-in-indent\", WS_TAB_IN_INDENT, 0, 1 },\n+\t{ \"no-eol-at-eof\", WS_NO_EOL_AT_EOF, 0 },\n };\n \n unsigned parse_whitespace_rule(const char *string)\n-- \n1.7.1.351.ge2633e\n"},{"id":"142891","messageId":"vpq1vco41go.fsf@bauges.imag.fr","threadId":"23992","inReplyTo":"cover.1275575236.git.git@drmicha.warpmail.net","subject":"Re: [PATCH 0/4] Don't warn about missing EOL for symlinks","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2010-06-03T14:47:51Z","receivedAt":"2010-06-03T14:47:51Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Michael J Gruber writes:\n\n> \"No newline at end of file\" always confuses me when looking at a diff for\n> symlinks. \"File? Huh? Didn't Git recognize my symlink?\"\n\nFor interactive use, I do understand. But how do you deal with the\n(improbable) case of a user actually adding a newline at the end of\nthe target of the symlink, and then using format-patch and am to apply\nthe changes somewhere else?\n\nYou probably want to make sure your patch doesn't modify format-patch.\n\nBTW, I disagree that the message is a \"warning\": it's actually a piece\nof information, part of the patch, but that we find annoying in this\ncase.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"142894","messageId":"4C07C2E8.508@drmicha.warpmail.net","threadId":"23992","inReplyTo":"vpq1vco41go.fsf@bauges.imag.fr","subject":"Re: [PATCH 0/4] Don't warn about missing EOL for symlinks","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2010-06-03T14:57:44Z","receivedAt":"2010-06-03T14:57:44Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Matthieu Moy venit, vidit, dixit 03.06.2010 16:47:\n> Michael J Gruber writes:\n> \n>> \"No newline at end of file\" always confuses me when looking at a diff for\n>> symlinks. \"File? Huh? Didn't Git recognize my symlink?\"\n> \n> For interactive use, I do understand. But how do you deal with the\n> (improbable) case of a user actually adding a newline at the end of\n> the target of the symlink, and then using format-patch and am to apply\n> the changes somewhere else?\n> \n> You probably want to make sure your patch doesn't modify format-patch.\n> \n> BTW, I disagree that the message is a \"warning\": it's actually a piece\n> of information, part of the patch, but that we find annoying in this\n> case.\n> \n\nMay I kindly direct you to the next parts you cut out, especially the\none talking about \"described thorougly along with the\nrationale in 3/4\", and to the commit message of 3/4? :)\n\nI'm not breaking existing tests, of course, which also test\nformat-patch/apply cycles with symlinks.\n\nMichael\n"},{"id":"142911","messageId":"20100603170244.GA22779@coredump.intra.peff.net","threadId":"23992","inReplyTo":"c1e0646449aae50fb962108d16b5428fb7d73bae.1275575236.git.git@drmicha.warpmail.net","subject":"Re: [PATCH 3/4] diff: Do not warn about missing EOL at EOF for symlinks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-06-03T17:02:44Z","receivedAt":"2010-06-03T17:02:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 03, 2010 at 04:35:22PM +0200, Michael J Gruber wrote:\n\n> diff --git a/diff.c b/diff.c\n> index 7950df6..cad7339 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -1788,6 +1788,8 @@ static void builtin_diff(const char *name_a,\n> \t\tecbdata.color_diff = DIFF_OPT_TST(o, COLOR_DIFF);\n> \t\tecbdata.found_changesp = &o->found_changes;\n> \t\tecbdata.ws_rule = whitespace_rule(name_b ? name_b : name_a);\n> +\t\tif (DIFF_OPT_TST(o, ALLOW_TEXTCONV) && (S_ISLNK(one->mode) || S_ISLNK(two->mode)))\n> +\t\t\tecbdata.ws_rule &= ~WS_NO_EOL_AT_EOF;\n\nI see why you would use ALLOW_TEXTCONV to control this behavior, since\ntextconv by definition creates patches you can't apply. But it feels a\nlittle hack-ish to me, as textconv is really something different. You\nseem to want a \"--allow-diff-which-cant-be-applied\".\n\n-Peff\n"},{"id":"142912","messageId":"20100603170724.GB22779@coredump.intra.peff.net","threadId":"23992","inReplyTo":"4C07C2E8.508@drmicha.warpmail.net","subject":"Re: [PATCH 0/4] Don't warn about missing EOL for symlinks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-06-03T17:07:24Z","receivedAt":"2010-06-03T17:07:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 03, 2010 at 04:57:44PM +0200, Michael J Gruber wrote:\n\n> May I kindly direct you to the next parts you cut out, especially the\n> one talking about \"described thorougly along with the\n> rationale in 3/4\", and to the commit message of 3/4? :)\n> \n> I'm not breaking existing tests, of course, which also test\n> format-patch/apply cycles with symlinks.\n\nYes, but you are breaking \"git diff | git apply\", aren't you? It is\nalready broken with textconv, but that is a new feature that people opt\ninto by using it. Symlink patches are a feature that has worked fine\nuntil now with the above command.\n\nI don't think \"but they should be using plumbing to generate patches\"\nis the right answer, either. Yes, we expect the diff porcelain to behave\ndifferently depending on configuration, but with the exception of\ntextconv, it always produces an actual applicable patch.\n\n-Peff\n"},{"id":"142922","messageId":"4C0808CE.2000506@drmicha.warpmail.net","threadId":"23992","inReplyTo":"20100603170724.GB22779@coredump.intra.peff.net","subject":"Re: [PATCH 0/4] Don't warn about missing EOL for symlinks","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2010-06-03T19:55:58Z","receivedAt":"2010-06-03T19:55:58Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jeff King venit, vidit, dixit 03.06.2010 19:07:\n> On Thu, Jun 03, 2010 at 04:57:44PM +0200, Michael J Gruber wrote:\n> \n>> May I kindly direct you to the next parts you cut out, especially the\n>> one talking about \"described thorougly along with the\n>> rationale in 3/4\", and to the commit message of 3/4? :)\n>>\n>> I'm not breaking existing tests, of course, which also test\n>> format-patch/apply cycles with symlinks.\n> \n> Yes, but you are breaking \"git diff | git apply\", aren't you? It is\n\nWe don't have any tests for that then. I ran all tests with my patch.\n\n> already broken with textconv, but that is a new feature that people opt\n> into by using it. Symlink patches are a feature that has worked fine\n> until now with the above command.\n> \n> I don't think \"but they should be using plumbing to generate patches\"\n> is the right answer, either. Yes, we expect the diff porcelain to behave\n> differently depending on configuration, but with the exception of\n> textconv, it always produces an actual applicable patch.\n\n...which is why you need to use diff --no-textconv for scripting, which\nis why I use that to decide about the symlink warnings!\n\nOne could introduce a separate config for that, of course, if you mind\nunguarded diff|apply. But don't you think that those \"No newline\"\nwarnings are just plain stupid for symlinks?\n\nMichael\n"},{"id":"142923","messageId":"20100603201759.GA20120@sigill.intra.peff.net","threadId":"23992","inReplyTo":"4C0808CE.2000506@drmicha.warpmail.net","subject":"Re: [PATCH 0/4] Don't warn about missing EOL for symlinks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-06-03T20:17:59Z","receivedAt":"2010-06-03T20:17:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 03, 2010 at 09:55:58PM +0200, Michael J Gruber wrote:\n\n> > Yes, but you are breaking \"git diff | git apply\", aren't you? It is\n> \n> We don't have any tests for that then. I ran all tests with my patch.\n\nThat doesn't mean people don't do it, or that we don't want to support\nit. The tests _can_ be wrong or incomplete. :)\n\n> ...which is why you need to use diff --no-textconv for scripting, which\n> is why I use that to decide about the symlink warnings!\n\nBut until now, people who didn't use textconv _didn't_ need to use diff\n--no-textconv. So you are breaking things for them.\n\nPlus, it is not about scripting versus not scripting. I may use \"git\ndiff >foo.diff\" manually to generate a diff. I think most users would\nexpect the result to be applicable by git-apply.\n\nPerhaps textconv opened a can of worms here that we should not have. I\nthink it is slightly less bad in the case of textconv for two reasons:\n\n 1. You at least have to start using the textconv feature to shoot\n yourself in the foot, so hopefully you are aware of it and its\n implications (or at least feel a little dumb when the implications\n bite you. :) ).\n\n 2. Trying to apply a textconv patch is _obviously_ wrong. The\n application will fail, you'll look at it and say \"Oh, this isn't\n even close to right. Clearly the binary part is missing.\" Somebody\n who tries to apply a symlink patch which has suppressed the \"no\n newline\" message will silently get a newline in their symlink\n value, won't they (or even if it does get rejected, it is not\n obvious that it is the missing newline indicator that is the\n problem).\n\n> One could introduce a separate config for that, of course, if you mind\n> unguarded diff|apply. But don't you think that those \"No newline\"\n> warnings are just plain stupid for symlinks?\n\nI do think they make the output harder for humans to read. But I also\nthink that producing working, correct diffs is an important job for \"git\ndiff\". Unfortunately because the diff format serves the dual purpose of\nbeing human and machine-readable, these sorts of conflicts are\ninevitable.\n\nAnother source of this conflict is colorization. The solution we took\nthere is show it only when output goes to the terminal or pager. Perhaps\nwe should do the same here (and arguably, that would be the right thing\nfor textconv, too).\n\n-Peff\n"},{"id":"142931","messageId":"7vsk537p8k.fsf@alter.siamese.dyndns.org","threadId":"23992","inReplyTo":"08e635cee993d97e2a38d7766ced11c064ef7d87.1275575236.git.git@drmicha.warpmail.net","subject":"Re: [PATCH 1/4] diff/xdiff: refactor EOF-EOL detection","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-06-03T21:58:19Z","receivedAt":"2010-06-03T21:58:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n> This puts the \"No newline at end of file\" warning for both code paths\n\nThis is not a warning.\n\nIt is an integral part of the diff output specification that allows patch\nrecipients to correctly reproduce incomplete lines. Removing this from\nthe output is simply out of question.\n"},{"id":"142945","messageId":"4C08AD75.6040307@drmicha.warpmail.net","threadId":"23992","inReplyTo":"7vsk537p8k.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/4] diff/xdiff: refactor EOF-EOL detection","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2010-06-04T07:38:29Z","receivedAt":"2010-06-04T07:38:29Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 03.06.2010 23:58:\n> Michael J Gruber writes:\n> \n>> This puts the \"No newline at end of file\" warning for both code paths\n> \n> This is not a warning.\n> \n> It is an integral part of the diff output specification that allows patch\n> recipients to correctly reproduce incomplete lines. Removing this from\n> the output is simply out of question.\n> \n\nI'm sorry, but that makes 3 out of 3 respondents who didn't seem to read\nwhat I wrote.\n\nThis series is about the display of that line for human-consumable\noutput such as log -p etc.\n\nPatch 1/4 doesn't remove anything. It just makes it simpler to follow\nthe different diff code paths that we have.\n\nIn 3/4, that \"No NL at EOF\" is suppressed for symlinks *when textconv is\nin effect only*, i.e. when we don't (necessarily) produce a diff fit to\nbe applied anyways.\n\nAs Jeff pointed out in his 2nd response, it may make sense to have this\ndependent on TERM just like color output is currently (which messes ab\ndiff|apply also!).\n\nI would even go so far to say that --no-textconv should be the default\nfor diff,log etc. unless the output goes to the terminal, just like our\ncolor handling.\n\nMichael\n"},{"id":"142982","messageId":"4C090A84.20209@kdbg.org","threadId":"23992","inReplyTo":"20100603170724.GB22779@coredump.intra.peff.net","subject":"Re: [PATCH 0/4] Don't warn about missing EOL for symlinks","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-06-04T14:15:32Z","receivedAt":"2010-06-04T14:15:32Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 03.06.2010 19:07, schrieb Jeff King:\n> I don't think \"but they should be using plumbing to generate patches\"\n> is the right answer, either. Yes, we expect the diff porcelain to behave\n> differently depending on configuration, but with the exception of\n> textconv, it always produces an actual applicable patch.\n\nI don't by into that argument: You have to give --binary if you have \nchanges in binary files. With Michael's patch, you have to give \n--no-textonv (too). I'm in favor of the patch.\n\n-- Hannes\n"},{"id":"142988","messageId":"20100604152519.GA22844@sigill.intra.peff.net","threadId":"23992","inReplyTo":"4C090A84.20209@kdbg.org","subject":"Re: [PATCH 0/4] Don't warn about missing EOL for symlinks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-06-04T15:25:19Z","receivedAt":"2010-06-04T15:25:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 04, 2010 at 04:15:32PM +0200, Johannes Sixt wrote:\n\n> Am 03.06.2010 19:07, schrieb Jeff King:\n> >I don't think \"but they should be using plumbing to generate patches\"\n> >is the right answer, either. Yes, we expect the diff porcelain to behave\n> >differently depending on configuration, but with the exception of\n> >textconv, it always produces an actual applicable patch.\n> \n> I don't by into that argument: You have to give --binary if you have\n> changes in binary files. With Michael's patch, you have to give\n> --no-textonv (too). I'm in favor of the patch.\n\nOK, I'll accept that \"git diff | git apply\" does have some special\ncases which need to be considered. But every special case is a possible\nplace for users to make a mistake. So we have to consider whether adding\nanother one is worthwhile. Specifically:\n\n 1. Are symlinks as unusual an occurrence as binary files? Do users\n perceive them as different enough from regular text files that they\n will remember to use special command line options?\n\n 2. Traditionally, symlinks have not been such a special case. Is a\n behavior change between versions worth it?\n\nI am not necessarily against the patch. I'm just trying to think through\nall of the possible negative ramifications. I think I would prefer the\napproach to treat it like color (do it only when requested explicitly,\nor when outputting to a terminal or pager).\n\n-Peff\n"},{"id":"143021","messageId":"7vpr060ys0.fsf@alter.siamese.dyndns.org","threadId":"23992","inReplyTo":"4C08AD75.6040307@drmicha.warpmail.net","subject":"Re: [PATCH 1/4] diff/xdiff: refactor EOF-EOL detection","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-06-05T06:38:39Z","receivedAt":"2010-06-05T06:38:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n> In 3/4, that \"No NL at EOF\" is suppressed for symlinks *when textconv is\n> in effect only*, i.e. when we don't (necessarily) produce a diff fit to\n> be applied anyways.\n\nSorry, that doesn't change a thing at all. The presense of incomplete\nline is part of the information diff gives you.\n\nIt can be done by defining a custom textconv filter that adds a trailing\nLF to a blob that ends in an incomplete line, and what your patch 3/4 does\nis essentially to create such a built-in textconv filter and *force* users\nto use it unconditionally for all paths unless the user explicitly asks\nnot to use *any* textconv.\n"},{"id":"143050","messageId":"4C0A9E54.50703@drmicha.warpmail.net","threadId":"23992","inReplyTo":"7vpr060ys0.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/4] diff/xdiff: refactor EOF-EOL detection","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2010-06-05T18:58:28Z","receivedAt":"2010-06-05T18:58:28Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 05.06.2010 08:38:\n> Michael J Gruber writes:\n> \n>> In 3/4, that \"No NL at EOF\" is suppressed for symlinks *when textconv is\n>> in effect only*, i.e. when we don't (necessarily) produce a diff fit to\n>> be applied anyways.\n> \n> Sorry, that doesn't change a thing at all. The presense of incomplete\n> line is part of the information diff gives you.\n> \n> It can be done by defining a custom textconv filter that adds a trailing\n> LF to a blob that ends in an incomplete line, and what your patch 3/4 does\n> is essentially to create such a built-in textconv filter and *force* users\n> to use it unconditionally for all paths unless the user explicitly asks\n> not to use *any* textconv.\n\nNo, not for all paths, only for symlinks. And, as not only I have\npointed out, diffs with textconv (which is the default) may produce\n\"incomplete\" diffs anyways. The only difference is that people would\nhave to set up textconv filters before.\n\nThat is why there seems to be consensus in statu nascendi (on that other\nbranch of the thread) that we should protect against textconv in the\nsame way as we do for color, i.e. based on whether stdout = tty. Then my\nsymlink treatment would be conditional in the same way as it is\nconditional on textconv.\n\nMichael\n"},{"id":"143070","messageId":"7vfx10yfmn.fsf@alter.siamese.dyndns.org","threadId":"23992","inReplyTo":"4C08AD75.6040307@drmicha.warpmail.net","subject":"Re: [PATCH 1/4] diff/xdiff: refactor EOF-EOL detection","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-06-06T04:00:32Z","receivedAt":"2010-06-06T04:00:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n> I'm sorry, but that makes 3 out of 3 respondents who didn't seem to read\n> what I wrote.\n\nYou seem to think three out of three people didn't read nor understood\nyour argument, but my take on it is that three out of three of people who\ncared about the issue thought your justification was not convincing.\n\nSymlinks are minority among the tracked contents (e.g. in git.git there is\nonly one), and they are almost always a single incomplete line. When they\nchange, you do want to notice, and I happen to find it a good visual aid\nto have these incomplete line indicators, in addition to the unusual\n120000 mode on the index line.\n\nPeff uses --textconv to show changes to the exif information on his photo\ncollections. If he has any symlinks, and if he finds that removal of \"\\No\nnewline\" is a regression and not an improvement, what recourse does your\npatch give him? Saying --no-textconv to work around that regression is\nnot a solution, isn't it?\n\nIf you start from a false premise that \"\\No newline\" was an unnecessary\nwarning, it may seem that the output (which almost always is given for\nsymlinks) needs \"improving\". But have you considered a possibility that\nremoval of the line from the output is not an improvement?\n"},{"id":"143074","messageId":"201006061101.02156.j6t@kdbg.org","threadId":"23992","inReplyTo":"7vfx10yfmn.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/4] diff/xdiff: refactor EOF-EOL detection","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-06-06T09:01:02Z","receivedAt":"2010-06-06T09:01:02Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Sonntag, 6. Juni 2010, Junio C Hamano wrote:\n> Symlinks are minority among the tracked contents (e.g. in git.git there is\n> only one), and they are almost always a single incomplete line. When they\n> change, you do want to notice, and I happen to find it a good visual aid\n> to have these incomplete line indicators, in addition to the unusual\n> 120000 mode on the index line.\n\nYou make whole lot of assumptions, don't you?\n\nA repository cannot have many tracked symlinks? They change infrequently? \nAdditional clues are needed to notice that they change?\n\n> Peff uses --textconv to show changes to the exif information on his photo\n> collections. If he has any symlinks, and if he finds that removal of \"\\No\n> newline\" is a regression and not an improvement, what recourse does your\n> patch give him? Saying --no-textconv to work around that regression is\n> not a solution, isn't it?\n\nOh, I'm pretty sure that Peff wouldn't use --textconv on his repository if he \ncared that diffs contained complete reproducible information.\n\n> If you start from a false premise that \"\\No newline\" was an unnecessary\n> warning,\n\nThat's a strawman. Michael never meant it that way although he said it \n(unfortunately).\n\nFor me, the 120000 mode is visual clue enough (and a very strong visual \ntrigger, BTW) when I browse through a diff. It's appropriate that \"\\No \nnewline\" is suppressed for symbolic links so that it does not distract from \nthe mode line, because \"\\No newline\" is a much strong trigger (that makes \nalarm bells ring).\n\n-- Hannes\n"},{"id":"143113","messageId":"20100606220304.GC6993@coredump.intra.peff.net","threadId":"23992","inReplyTo":"7vpr060ys0.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/4] diff/xdiff: refactor EOF-EOL detection","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-06-06T22:03:05Z","receivedAt":"2010-06-06T22:03:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 04, 2010 at 11:38:39PM -0700, Junio C Hamano wrote:\n\n> It can be done by defining a custom textconv filter that adds a trailing\n> LF to a blob that ends in an incomplete line, and what your patch 3/4 does\n> is essentially to create such a built-in textconv filter and *force* users\n> to use it unconditionally for all paths unless the user explicitly asks\n> not to use *any* textconv.\n\nSide note: it would be kind of cool to have .gitattributes selectable on\nfile mode, so you could implement this feature entirely as a textconv on\nsymlinks. And then you could pretty-print them however you liked.\n\nThat may be going overboard, though.\n\n-Peff\n"},{"id":"143114","messageId":"20100606220800.GD6993@coredump.intra.peff.net","threadId":"23992","inReplyTo":"201006061101.02156.j6t@kdbg.org","subject":"Re: [PATCH 1/4] diff/xdiff: refactor EOF-EOL detection","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-06-06T22:08:00Z","receivedAt":"2010-06-06T22:08:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 06, 2010 at 11:01:02AM +0200, Johannes Sixt wrote:\n\n> > Peff uses --textconv to show changes to the exif information on his\n> > photo collections. If he has any symlinks, and if he finds that\n> > removal of \"\\No newline\" is a regression and not an improvement,\n> > what recourse does your patch give him? Saying --no-textconv to\n> > work around that regression is not a solution, isn't it?\n> \n> Oh, I'm pretty sure that Peff wouldn't use --textconv on his\n> repository if he cared that diffs contained complete reproducible\n> information.\n\nHow did my name get dragged into this? ;P\n\nNo, I wouldn't use textconv if I cared that my diffs contained complete\nreproducible information. But I think Junio's point is that the presence\nof that information is valuable to some users to read, regardless of\napplying patches. That is, I took his message to mean that he would want\nto have this feature off, even for human viewing of the diff. And there\nwould be no way to turn off this feature, but not textconv.\n\nI don't necessarily agree with that; I don't personally care when\nviewing a symlink diff whether that line is there or not. But if there\nare people who do, then you have left them with no way out.\n\n-Peff\n"},{"id":"143146","messageId":"4C0CA980.6090706@drmicha.warpmail.net","threadId":"23992","inReplyTo":"201006061101.02156.j6t@kdbg.org","subject":"Re: [PATCH 1/4] diff/xdiff: refactor EOF-EOL detection","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2010-06-07T08:10:40Z","receivedAt":"2010-06-07T08:10:40Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Johannes Sixt venit, vidit, dixit 06.06.2010 11:01:\n[This is more of a response to all of you, not only J6t.]\n[Scroll down to CONCLUSION if you're not interested in specific\ntechnical repsonses.]\n> On Sonntag, 6. Juni 2010, Junio C Hamano wrote:\n>> Symlinks are minority among the tracked contents (e.g. in git.git there is\n>> only one), and they are almost always a single incomplete line. When they\n>> change, you do want to notice, and I happen to find it a good visual aid\n>> to have these incomplete line indicators, in addition to the unusual\n>> 120000 mode on the index line.\n> \n> You make whole lot of assumptions, don't you?\n> \n> A repository cannot have many tracked symlinks? They change infrequently? \n> Additional clues are needed to notice that they change?\n> \n>> Peff uses --textconv to show changes to the exif information on his photo\n>> collections. If he has any symlinks, and if he finds that removal of \"\\No\n>> newline\" is a regression and not an improvement, what recourse does your\n>> patch give him? Saying --no-textconv to work around that regression is\n>> not a solution, isn't it?\n> \n> Oh, I'm pretty sure that Peff wouldn't use --textconv on his repository if he \n> cared that diffs contained complete reproducible information.\n> \n>> If you start from a false premise that \"\\No newline\" was an unnecessary\n>> warning,\n> \n> That's a strawman. Michael never meant it that way although he said it \n> (unfortunately).\n\nI'm not sure where I said that. I called that line a \"warning\", which\nwas unfortunate, because it is part of the diff spec. (Full disclosure:\nI wasn't aware of that in the earlier stage of that series' development\neither. Had I only known what alarm bells that term \"warning\" would ring...)\n\nYou're right about what I meant: Make the diff output (i.e. log, show\netc.) which is meant to be human consumable more human consumable.\n\n> For me, the 120000 mode is visual clue enough (and a very strong visual \n> trigger, BTW) when I browse through a diff. It's appropriate that \"\\No \n> newline\" is suppressed for symbolic links so that it does not distract from \n> the mode line, because \"\\No newline\" is a much strong trigger (that makes \n> alarm bells ring).\n\nWell, it seems that Junio takes that no-eol-eof-line actually as a\nvisually stronger indication of \"symlink\", which I find strange, but\nthat is a matter of taste.\n\nWhen I say \"human consumable\" above I define \"human\" as \"Git user with\naverage knowledge or better\". And I would think that a large fraction of\nthose don't know how Git represents symlinks internally, and they\n*should not need to know*. They also should not need to know the diff\nspec for files with no-eol-at-eof, unless they use them.\n\nAnd, there's no way denying: \"Now newline at end of file\" does look like\na warning, like something is wrong about a file. So, it is confusing\non two counts: \"wrong\" and \"file\".\n\nI would also think that symlinks are much more common than proper files\nwith no-eol-at-eof, so it's very likely people know about and use\nsymlinks but not the rest. But, of course, that's just my assumptions.\n\nI thought (before) about plugging into the textconv mechanism proper and\nappending a \\n before feeding the diff machinery but didn't want to dupe\nstrbufs in there, thinking one might use this (per explicit request) for\nlarger blobs also, not just symlinks. But maybe that concern was\nunnecessary. I'm not sure though whether the eol should platform\ndependent here, or dependent on core.eol, or... Suppressing the\nwarning^W diff line seems easier.\n\nI haven't looked into the matter of \"attributes per file type\". I agree\nwith Jeff that it would be conceptually nice but can't judge the degree\nof overboardness, so to speak.\n\nI've learned not to be confused by those not-warnings any more, so I\ndon't care personally.\n\nCONCLUSION\n\nIn conclusion, I think this shows (again) that we have a fragile\ndistinction between porcelain and plumbing. A porcelain command should\nnot have raised all these concerns. I should be allowed to claim \"This\nis porcelain so any scripting reasoning (diff|apply) is invalid by\ndefinition\" but didn't even consider it. I'm not that legalistic ;)\n\nWe need a proper strategy for having a consistent \"used-as-plumbing\"\nmode, activated not only on request by \"--porcelain\". I would think that\nactivating that mode based on istty and, maybe, also by\nGIT_PLUMBING_MODE set by a caller (or git --porcellain command) would\nprove beneficial on more occasions then just this one (textconv...).\n\nMichael\n"}]}