[PATCH 0/4] Don't warn about missing EOL for symlinks

"No newline at end of file" always confuses me when looking at a diff for
symlinks. "File? Huh? Didn't Git recognize my symlink?"

This mini series refactors the diff/xdiff code slightly to have the two
occurences of the warning both in diff.c, introduces a whitespace flag
WS_NO_EOL_AT_EOF to make this warning configurable, and actually changes
the default behaviour to what is described thorougly along with the
rationale in 3/4.

4/4 is a RFC - do we need to tweak this from config like other whitespace
warnings? 4/4 is lacking the Doc change for now.

Michael J Gruber (4):
  diff/xdiff: refactor EOF-EOL detection
  diff: make treatment of missing EOL at EOF configurable
  diff: Do not warn about missing EOL at EOF for symlinks
  RFC: add whitespace rule for no-eol-at-eof

 cache.h                  |    3 ++-
 diff.c                   |    7 ++++++-
 t/t4030-diff-textconv.sh |    1 -
 ws.c                     |    1 +
 xdiff/xutils.c           |    5 -----
 5 files changed, 9 insertions(+), 8 deletions(-)

This puts the "No newline at end of file" warning for both code paths
(emit_rewrite_lines for emit_rewrite_diff and fn_out_consume for
xdi_diff_outf) into diff.c so that it is easier to adjust later on.

Signed-off-by: Michael J Gruber <redacted>
---
 diff.c         |    2 ++
 xdiff/xutils.c |    5 -----
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/diff.c b/diff.c
index 4e07744..ff44bcc 100644
--- a/diff.c
+++ b/diff.c

@@ -917,6 +917,8 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
 		ecbdata->lno_in_postimage++;
 		emit_add_line(reset, ecbdata, line + 1, len - 1);
 	}
+	if (line[len-1] != '\n')
+		fputs("\n\\ No newline at end of file\n", ecbdata->file);
 }
 
 static char *pprint_rename(const char *a, const char *b)

diff --git a/xdiff/xutils.c b/xdiff/xutils.c
index bc12f29..feb43a9 100644
--- a/xdiff/xutils.c
+++ b/xdiff/xutils.c

@@ -51,11 +51,6 @@ int xdl_emit_diffrec(char const *rec, long size, char const *pre, long psize,
 	mb[0].size = psize;
 	mb[1].ptr = (char *) rec;
 	mb[1].size = size;
-	if (size > 0 && rec[size - 1] != '\n') {
-		mb[2].ptr = (char *) "\n\\ No newline at end of file\n";
-		mb[2].size = strlen(mb[2].ptr);
-		i++;
-	}
 	if (ecb->outf(ecb->priv, mb, i) < 0) {
 
 		return -1;

-- 
1.7.1.351.ge2633e

Symbolic links are represented by blobs without a trailing newline
(EOL). This cannot be changed because it would encumber the
representation with platform EOL issues.

But seeing a warning about "No newline at end of file" in the diff for
a symbolic link is highly distracting - in particular, it distracts from
the actual mode (120000) and gives the false impression that Git treated
the symlink as a file.

Therefore, change the default behaviour of the diff code so that there
is no warning about missing EOL at EOF if a symlink is involved AND
textconv is allowed. So, --no-textconv restores the old behaviour, and
diff users such as format-patch are not affected by this change so that
apply/am are still able to create files with no EOL at EOF - such as
symlink representations.

Note that a mode change (file<->symlink) is always rewritten as
deletion+addition by the diff engine so that replacing a file without
EOL at EOF by a symlink or vice versa displays the warning for the file.

Signed-off-by: Michael J Gruber <redacted>
---
 diff.c                   |    2 ++
 t/t4030-diff-textconv.sh |    1 -
 2 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/diff.c b/diff.c
index 7950df6..cad7339 100644
--- a/diff.c
+++ b/diff.c

@@ -1788,6 +1788,8 @@ static void builtin_diff(const char *name_a,
 		ecbdata.color_diff = DIFF_OPT_TST(o, COLOR_DIFF);
 		ecbdata.found_changesp = &o->found_changes;
 		ecbdata.ws_rule = whitespace_rule(name_b ? name_b : name_a);
+		if (DIFF_OPT_TST(o, ALLOW_TEXTCONV) && (S_ISLNK(one->mode) || S_ISLNK(two->mode)))
+			ecbdata.ws_rule &= ~WS_NO_EOL_AT_EOF;
 		if (ecbdata.ws_rule & WS_BLANK_AT_EOF)
 			check_blank_at_eof(&mf1, &mf2, &ecbdata);
 		ecbdata.file = o->file;

diff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh
index 88c5619..b6e537f 100755
--- a/t/t4030-diff-textconv.sh
+++ b/t/t4030-diff-textconv.sh

@@ -109,7 +109,6 @@ index 0000000..67be421
 +++ b/file
 @@ -0,0 +1 @@
 +frotz
-\ No newline at end of file
 EOF
 # make a symlink the hard way that works on symlink-challenged file systems
 test_expect_success 'textconv does not act on symlinks' '

-- 
1.7.1.351.ge2633e

I really don't know if it's worth to do this one.

No doc so far.
Also, as it is, -no-eol-at-eof turns around the default for files but
not for symlinks.

I really don't know...

Signed-off-by: Michael J Gruber <redacted>
---
 ws.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/ws.c b/ws.c
index d7b8c33..73502c8 100644
--- a/ws.c
+++ b/ws.c

@@ -20,6 +20,7 @@ static struct whitespace_rule {
 	{ "blank-at-eol", WS_BLANK_AT_EOL, 0 },
 	{ "blank-at-eof", WS_BLANK_AT_EOF, 0 },
 	{ "tab-in-indent", WS_TAB_IN_INDENT, 0, 1 },
+	{ "no-eol-at-eof", WS_NO_EOL_AT_EOF, 0 },
 };
 
 unsigned parse_whitespace_rule(const char *string)

-- 
1.7.1.351.ge2633e

Make this one of the WS_* flags so that we can later turn the warning
for missing EOL at EOF on and off as per config or blob type.

Signed-off-by: Michael J Gruber <redacted>
---
 cache.h |    3 ++-
 diff.c  |    5 +++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/cache.h b/cache.h
index c966023..dc72b8f 100644
--- a/cache.h
+++ b/cache.h

@@ -1048,8 +1048,9 @@ void shift_tree_by(const unsigned char *, const unsigned char *, unsigned char *
 #define WS_CR_AT_EOL           010
 #define WS_BLANK_AT_EOF        020
 #define WS_TAB_IN_INDENT       040
+#define WS_NO_EOL_AT_EOF      0100
 #define WS_TRAILING_SPACE      (WS_BLANK_AT_EOL|WS_BLANK_AT_EOF)
-#define WS_DEFAULT_RULE (WS_TRAILING_SPACE|WS_SPACE_BEFORE_TAB)
+#define WS_DEFAULT_RULE (WS_TRAILING_SPACE|WS_SPACE_BEFORE_TAB|WS_NO_EOL_AT_EOF)
 extern unsigned whitespace_rule_cfg;
 extern unsigned whitespace_rule(const char *);
 extern unsigned parse_whitespace_rule(const char *);

diff --git a/diff.c b/diff.c
index ff44bcc..7950df6 100644
--- a/diff.c
+++ b/diff.c

@@ -455,7 +455,7 @@ static void emit_rewrite_lines(struct emit_callback *ecb,
 		size -= len;
 		data += len;
 	}
-	if (!endp) {
+	if (!endp && (ecb->ws_rule & WS_NO_EOL_AT_EOF)) {
 		const char *plain = diff_get_color(ecb->color_diff,
 						   DIFF_PLAIN);
 		emit_line_0(ecb->file, plain, reset, '\\',

@@ -918,7 +918,8 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
 		emit_add_line(reset, ecbdata, line + 1, len - 1);
 	}
 	if (line[len-1] != '\n')
-		fputs("\n\\ No newline at end of file\n", ecbdata->file);
+		fputs((ecbdata->ws_rule & WS_NO_EOL_AT_EOF)
+			? "\n\\ No newline at end of file\n" : "\n", ecbdata->file);
 }
 
 static char *pprint_rename(const char *a, const char *b)

-- 
1.7.1.351.ge2633e

On Thu, Jun 03, 2010 at 04:35:22PM +0200, Michael J Gruber wrote:

I see why you would use ALLOW_TEXTCONV to control this behavior, since
textconv by definition creates patches you can't apply. But it feels a
little hack-ish to me, as textconv is really something different. You
seem to want a "--allow-diff-which-cant-be-applied".

-Peff

Michael J Gruber [off-list ref] writes:

This puts the "No newline at end of file" warning for both code paths

This is not a warning.

It is an integral part of the diff output specification that allows patch
recipients to correctly reproduce incomplete lines.  Removing this from
the output is simply out of question.

Junio C Hamano venit, vidit, dixit 03.06.2010 23:58:

Michael J Gruber [off-list ref] writes:

quoted

This puts the "No newline at end of file" warning for both code paths

This is not a warning.

It is an integral part of the diff output specification that allows patch
recipients to correctly reproduce incomplete lines.  Removing this from
the output is simply out of question.

I'm sorry, but that makes 3 out of 3 respondents who didn't seem to read
what I wrote.

This series is about the display of that line for human-consumable
output such as log -p etc.

Patch 1/4 doesn't remove anything. It just makes it simpler to follow
the different diff code paths that we have.

In 3/4, that "No NL at EOF" is suppressed for symlinks *when textconv is
in effect only*, i.e. when we don't (necessarily) produce a diff fit to
be applied anyways.

As Jeff pointed out in his 2nd response, it may make sense to have this
dependent on TERM just like color output is currently (which messes ab
diff|apply also!).

I would even go so far to say that --no-textconv should be the default
for diff,log etc. unless the output goes to the terminal, just like our
color handling.

Michael

Michael J Gruber [off-list ref] writes:

I'm sorry, but that makes 3 out of 3 respondents who didn't seem to read
what I wrote.

You seem to think three out of three people didn't read nor understood
your argument, but my take on it is that three out of three of people who
cared about the issue thought your justification was not convincing.

Symlinks are minority among the tracked contents (e.g. in git.git there is
only one), and they are almost always a single incomplete line.  When they
change, you do want to notice, and I happen to find it a good visual aid
to have these incomplete line indicators, in addition to the unusual
120000 mode on the index line.

Peff uses --textconv to show changes to the exif information on his photo
collections.  If he has any symlinks, and if he finds that removal of "\No
newline" is a regression and not an improvement, what recourse does your
patch give him?  Saying --no-textconv to work around that regression is
not a solution, isn't it?

If you start from a false premise that "\No newline" was an unnecessary
warning, it may seem that the output (which almost always is given for
symlinks) needs "improving".  But have you considered a possibility that
removal of the line from the output is not an improvement?

On Sonntag, 6. Juni 2010, Junio C Hamano wrote:

Symlinks are minority among the tracked contents (e.g. in git.git there is
only one), and they are almost always a single incomplete line.  When they
change, you do want to notice, and I happen to find it a good visual aid
to have these incomplete line indicators, in addition to the unusual
120000 mode on the index line.

You make whole lot of assumptions, don't you?

A repository cannot have many tracked symlinks? They change infrequently? 
Additional clues are needed to notice that they change?

Peff uses --textconv to show changes to the exif information on his photo
collections.  If he has any symlinks, and if he finds that removal of "\No
newline" is a regression and not an improvement, what recourse does your
patch give him?  Saying --no-textconv to work around that regression is
not a solution, isn't it?

Oh, I'm pretty sure that Peff wouldn't use --textconv on his repository if he 
cared that diffs contained complete reproducible information.

If you start from a false premise that "\No newline" was an unnecessary
warning,

That's a strawman. Michael never meant it that way although he said it 
(unfortunately).

For me, the 120000 mode is visual clue enough (and a very strong visual 
trigger, BTW) when I browse through a diff. It's appropriate that "\No 
newline" is suppressed for symbolic links so that it does not distract from 
the mode line, because "\No newline" is a much strong trigger (that makes 
alarm bells ring).

-- Hannes

On Sun, Jun 06, 2010 at 11:01:02AM +0200, Johannes Sixt wrote:

quoted

Peff uses --textconv to show changes to the exif information on his
photo collections.  If he has any symlinks, and if he finds that
removal of "\No newline" is a regression and not an improvement,
what recourse does your patch give him?  Saying --no-textconv to
work around that regression is not a solution, isn't it?

Oh, I'm pretty sure that Peff wouldn't use --textconv on his
repository if he cared that diffs contained complete reproducible
information.

How did my name get dragged into this? ;P

No, I wouldn't use textconv if I cared that my diffs contained complete
reproducible information. But I think Junio's point is that the presence
of that information is valuable to some users to read, regardless of
applying patches. That is, I took his message to mean that he would want
to have this feature off, even for human viewing of the diff. And there
would be no way to turn off this feature, but not textconv.

I don't necessarily agree with that; I don't personally care when
viewing a symlink diff whether that line is there or not. But if there
are people who do, then you have left them with no way out.

-Peff

Johannes Sixt venit, vidit, dixit 06.06.2010 11:01:
[This is more of a response to all of you, not only J6t.]
[Scroll down to CONCLUSION if you're not interested in specific
technical repsonses.]

On Sonntag, 6. Juni 2010, Junio C Hamano wrote:

quoted

Symlinks are minority among the tracked contents (e.g. in git.git there is
only one), and they are almost always a single incomplete line.  When they
change, you do want to notice, and I happen to find it a good visual aid
to have these incomplete line indicators, in addition to the unusual
120000 mode on the index line.

You make whole lot of assumptions, don't you?

A repository cannot have many tracked symlinks? They change infrequently? 
Additional clues are needed to notice that they change?

quoted

Peff uses --textconv to show changes to the exif information on his photo
collections.  If he has any symlinks, and if he finds that removal of "\No
newline" is a regression and not an improvement, what recourse does your
patch give him?  Saying --no-textconv to work around that regression is
not a solution, isn't it?

Oh, I'm pretty sure that Peff wouldn't use --textconv on his repository if he 
cared that diffs contained complete reproducible information.

quoted

If you start from a false premise that "\No newline" was an unnecessary
warning,

That's a strawman. Michael never meant it that way although he said it 
(unfortunately).

I'm not sure where I said that. I called that line a "warning", which
was unfortunate, because it is part of the diff spec. (Full disclosure:
I wasn't aware of that in the earlier stage of that series' development
either. Had I only known what alarm bells that term "warning" would ring...)

You're right about what I meant: Make the diff output (i.e. log, show
etc.) which is meant to be human consumable more human consumable.

For me, the 120000 mode is visual clue enough (and a very strong visual 
trigger, BTW) when I browse through a diff. It's appropriate that "\No 
newline" is suppressed for symbolic links so that it does not distract from 
the mode line, because "\No newline" is a much strong trigger (that makes 
alarm bells ring).

Well, it seems that Junio takes that no-eol-eof-line actually as a
visually stronger indication of "symlink", which I find strange, but
that is a matter of taste.

When I say "human consumable" above I define "human" as "Git user with
average knowledge or better". And I would think that a large fraction of
those don't know how Git represents symlinks internally, and they
*should not need to know*. They also should not need to know the diff
spec for files with no-eol-at-eof, unless they use them.

And, there's no way denying: "Now newline at end of file" does look like
a warning, like something is wrong about a file. So, it is confusing
on two counts: "wrong" and "file".

I would also think that symlinks are much more common than proper files
with no-eol-at-eof, so it's very likely people know about and use
symlinks but not the rest. But, of course, that's just my assumptions.

I thought (before) about plugging into the textconv mechanism proper and
appending a \n before feeding the diff machinery but didn't want to dupe
strbufs in there, thinking one might use this (per explicit request) for
larger blobs also, not just symlinks. But maybe that concern was
unnecessary. I'm not sure though whether the eol should platform
dependent here, or dependent on core.eol, or... Suppressing the
warning^W diff line seems easier.

I haven't looked into the matter of "attributes per file type". I agree
with Jeff that it would be conceptually nice but can't judge the degree
of overboardness, so to speak.

I've learned not to be confused by those not-warnings any more, so I
don't care personally.

CONCLUSION

In conclusion, I think this shows (again) that we have a fragile
distinction between porcelain and plumbing. A porcelain command should
not have raised all these concerns. I should be allowed to claim "This
is porcelain so any scripting reasoning (diff|apply) is invalid by
definition" but didn't even consider it. I'm not that legalistic ;)

We need a proper strategy for having a consistent "used-as-plumbing"
mode, activated not only on request by "--porcelain". I would think that
activating that mode based on istty and, maybe, also by
GIT_PLUMBING_MODE set by a caller (or git --porcellain command) would
prove beneficial on more occasions then just this one (textconv...).

Michael