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

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

[Michael J Gruber]

"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(-)

[PATCH 1/4] diff/xdiff: refactor EOF-EOL detection

[Michael J Gruber]

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 <git@drmicha.warpmail.net>
---
 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

Re: [PATCH 1/4] diff/xdiff: refactor EOF-EOL detection

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> 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.

Re: [PATCH 1/4] diff/xdiff: refactor EOF-EOL detection

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 03.06.2010 23:58:
> Michael J Gruber <git@drmicha.warpmail.net> 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.
> 

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

Re: [PATCH 1/4] diff/xdiff: refactor EOF-EOL detection

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

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

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.

Re: [PATCH 1/4] diff/xdiff: refactor EOF-EOL detection

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 05.06.2010 08:38:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> 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.
> 
> 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.

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

Re: [PATCH 1/4] diff/xdiff: refactor EOF-EOL detection

[Jeff King]

On Fri, Jun 04, 2010 at 11:38:39PM -0700, Junio C Hamano wrote:

> 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

Re: [PATCH 1/4] diff/xdiff: refactor EOF-EOL detection

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> 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?

Re: [PATCH 1/4] diff/xdiff: refactor EOF-EOL detection

[Johannes Sixt]

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

Re: [PATCH 1/4] diff/xdiff: refactor EOF-EOL detection

[Jeff King]

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

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

Re: [PATCH 1/4] diff/xdiff: refactor EOF-EOL detection

[Michael J Gruber]

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:
>> 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).

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

[PATCH 2/4] diff: make treatment of missing EOL at EOF configurable

[Michael J Gruber]

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 <git@drmicha.warpmail.net>
---
 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

[PATCH 3/4] diff: Do not warn about missing EOL at EOF for symlinks

[Michael J Gruber]

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 <git@drmicha.warpmail.net>
---
 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

Re: [PATCH 3/4] diff: Do not warn about missing EOL at EOF for symlinks

[Jeff King]

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

> 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;

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

[PATCH 4/4] RFC: add whitespace rule for no-eol-at-eof

[Michael J Gruber]

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 <git@drmicha.warpmail.net>
---
 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

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

[Matthieu Moy]

Michael J Gruber <git@drmicha.warpmail.net> writes:

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

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/

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

[Michael J Gruber]

Matthieu Moy venit, vidit, dixit 03.06.2010 16:47:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> "No newline at end of file" always confuses me when looking at a diff for
>> symlinks. "File? Huh? Didn't Git recognize my symlink?"
> 
> 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.
> 

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

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

[Jeff King]

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

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

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

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

[Michael J Gruber]

Jeff King venit, vidit, dixit 03.06.2010 19:07:
> On Thu, Jun 03, 2010 at 04:57:44PM +0200, Michael J Gruber wrote:
> 
>> 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.
> 
> Yes, but you are breaking "git diff | git apply", aren't you? It is

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

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

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

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

[Jeff King]

On Thu, Jun 03, 2010 at 09:55:58PM +0200, Michael J Gruber wrote:

> > Yes, but you are breaking "git diff | git apply", aren't you? It is
> 
> We don't have any tests for that then. I ran all tests with my patch.

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. :)

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

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

> 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?

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

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

[Johannes Sixt]

Am 03.06.2010 19:07, schrieb Jeff King:
> 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.

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

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

[Jeff King]

On Fri, Jun 04, 2010 at 04:15:32PM +0200, Johannes Sixt wrote:

> Am 03.06.2010 19:07, schrieb Jeff King:
> >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.
> 
> 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.

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