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.

---
 diff.c         |    2 ++
 xdiff/xutils.c |    5 -----
 2 files changed, 2 insertions(+), 5 deletions(-)

index 4e07744..ff44bcc 100644
@@ -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)

index bc12f29..feb43a9 100644
@@ -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

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:

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

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?

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?

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

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

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.]

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.

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

Sorry, that doesn't change a thing at all. The presense of incomplete line is part of the information diff gives you.

It can be done by defining a custom textconv filter that adds a trailing LF to a blob that ends in an incomplete line, and what your patch 3/4 does is essentially to create such a built-in textconv filter and *force* users to use it unconditionally for all paths unless the user explicitly asks not to use *any* textconv.

Side note: it would be kind of cool to have .gitattributes selectable on file mode, so you could implement this feature entirely as a textconv on symlinks. And then you could pretty-print them however you liked.

That may be going overboard, though.

-Peff

Junio C Hamano venit, vidit, dixit 05.06.2010 08:38:

No, not for all paths, only for symlinks. And, as not only I have pointed out, diffs with textconv (which is the default) may produce "incomplete" diffs anyways. The only difference is that people would have to set up textconv filters before.

That is why there seems to be consensus in statu nascendi (on that other branch of the thread) that we should protect against textconv in the same way as we do for color, i.e. based on whether stdout = tty. Then my symlink treatment would be conditional in the same way as it is conditional on textconv.

Michael

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.

---
 cache.h |    3 ++-
 diff.c  |    5 +++--
 2 files changed, 5 insertions(+), 3 deletions(-)

index c966023..dc72b8f 100644
@@ -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 *);

index ff44bcc..7950df6 100644
@@ -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

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.

---
 diff.c                   |    2 ++
 t/t4030-diff-textconv.sh |    1 -
 2 files changed, 2 insertions(+), 1 deletions(-)

index 7950df6..cad7339 100644
@@ -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;

index 88c5619..b6e537f 100755
@@ -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...

---
 ws.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

index d7b8c33..73502c8 100644
@@ -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

For interactive use, I do understand. But how do you deal with the (improbable) case of a user actually adding a newline at the end of the target of the symlink, and then using format-patch and am to apply the changes somewhere else?

You probably want to make sure your patch doesn't modify format-patch.

BTW, I disagree that the message is a "warning": it's actually a piece of information, part of the patch, but that we find annoying in this case.

Matthieu Moy
http://www-verimag.imag.fr/~moy/

Matthieu Moy venit, vidit, dixit 03.06.2010 16:47:

May I kindly direct you to the next parts you cut out, especially the one talking about "described thorougly along with the rationale in 3/4", and to the commit message of 3/4? :)

I'm not breaking existing tests, of course, which also test format-patch/apply cycles with symlinks.

Michael

Yes, but you are breaking "git diff | git apply", aren't you? It is already broken with textconv, but that is a new feature that people opt into by using it. Symlink patches are a feature that has worked fine until now with the above command.

I don't think "but they should be using plumbing to generate patches" is the right answer, either. Yes, we expect the diff porcelain to behave differently depending on configuration, but with the exception of textconv, it always produces an actual applicable patch.

-Peff

Jeff King venit, vidit, dixit 03.06.2010 19:07:

We don't have any tests for that then. I ran all tests with my patch.

...which is why you need to use diff --no-textconv for scripting, which is why I use that to decide about the symlink warnings!

One could introduce a separate config for that, of course, if you mind unguarded diff|apply. But don't you think that those "No newline" warnings are just plain stupid for symlinks?

Michael

That doesn't mean people don't do it, or that we don't want to support it. The tests _can_ be wrong or incomplete. :)

But until now, people who didn't use textconv _didn't_ need to use diff --no-textconv. So you are breaking things for them.

Plus, it is not about scripting versus not scripting. I may use "git diff >foo.diff" manually to generate a diff. I think most users would expect the result to be applicable by git-apply.

Perhaps textconv opened a can of worms here that we should not have. I think it is slightly less bad in the case of textconv for two reasons:

1. You at least have to start using the textconv feature to shoot yourself in the foot, so hopefully you are aware of it and its implications (or at least feel a little dumb when the implications bite you. :) ).

2. Trying to apply a textconv patch is _obviously_ wrong. The application will fail, you'll look at it and say "Oh, this isn't even close to right. Clearly the binary part is missing." Somebody who tries to apply a symlink patch which has suppressed the "no newline" message will silently get a newline in their symlink value, won't they (or even if it does get rejected, it is not obvious that it is the missing newline indicator that is the problem).

I do think they make the output harder for humans to read. But I also think that producing working, correct diffs is an important job for "git diff". Unfortunately because the diff format serves the dual purpose of being human and machine-readable, these sorts of conflicts are inevitable.

Another source of this conflict is colorization. The solution we took there is show it only when output goes to the terminal or pager. Perhaps we should do the same here (and arguably, that would be the right thing for textconv, too).

-Peff

Am 03.06.2010 19:07, schrieb Jeff King:

I don't by into that argument: You have to give --binary if you have changes in binary files. With Michael's patch, you have to give --no-textonv (too). I'm in favor of the patch.

-- Hannes

OK, I'll accept that "git diff | git apply" does have some special cases which need to be considered. But every special case is a possible place for users to make a mistake. So we have to consider whether adding another one is worthwhile. Specifically:

1. Are symlinks as unusual an occurrence as binary files? Do users perceive them as different enough from regular text files that they will remember to use special command line options?

2. Traditionally, symlinks have not been such a special case. Is a behavior change between versions worth it?

I am not necessarily against the patch. I'm just trying to think through all of the possible negative ramifications. I think I would prefer the approach to treat it like color (do it only when requested explicitly, or when outputting to a terminal or pager).

-Peff