About git reporting missing newline for symlinks

About git reporting missing newline for symlinks

[Ignacio Taranto]

So, both git diff and git show display "\ No newline at end of file"
for symlinks.

I think this is related to how Git renders diffs, IMO it shouldn't
display a warning about
newlines for symlinks.

Am I making any sense here?

Re: About git reporting missing newline for symlinks

[Junio C Hamano]

Ignacio Taranto <ignacio.taranto@eclypsium.com> writes:

> So, both git diff and git show display "\ No newline at end of file"
> for symlinks.
>
> I think this is related to how Git renders diffs, IMO it shouldn't
> display a warning about newlines for symlinks.
>
> Am I making any sense here?

Yes, but not really.

It is not "warning" at all.  The users want to know when compared
contents do or do not end with an incomplete line at the end, and
the "\ No newline" is the diff's way to give that single bit of
information to us.

And the contents of a symbolic link typically is a single incomplete
line, so it is expected to see "\ No newline" when comparing them.

This is important as "git diff | git -C ../some/where/else apply"
should be able to recreate the change in the current repository
(which may have change to or addition of a symbolic link) in the
other repository, and "git apply" on the receiving end must be able
to tell that the symbolic link it needs to create in the other
repository must not have an extra LF at the end.

Re: About git reporting missing newline for symlinks

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 889 bytes --]

On 2022-10-12 at 21:17:04, Ignacio Taranto wrote:
> So, both git diff and git show display "\ No newline at end of file"
> for symlinks.
> 
> I think this is related to how Git renders diffs, IMO it shouldn't
> display a warning about
> newlines for symlinks.
> 
> Am I making any sense here?

What Git shows for a symlink is the destination of the symlink.  On
Unix systems other than macOS, path names may contain any byte other
than NUL, including a newline.

While it obviously is not a good practice to include newlines in your
paths, it is possible, and Git would not be able to round-trip symlinks
where the final character of the destination is a newline if we didn't
handle trailing newlines properly.  As a result, we're not likely to
change things here because it's required for correctness.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: About git reporting missing newline for symlinks

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> On 2022-10-12 at 21:17:04, Ignacio Taranto wrote:
>> So, both git diff and git show display "\ No newline at end of file"
>> for symlinks.
>> 
>> I think this is related to how Git renders diffs, IMO it shouldn't
>> display a warning about
>> newlines for symlinks.
>> 
>> Am I making any sense here?
>
> What Git shows for a symlink is the destination of the symlink.  On
> Unix systems other than macOS, path names may contain any byte other
> than NUL, including a newline.
>
> While it obviously is not a good practice to include newlines in your
> paths, it is possible, and Git would not be able to round-trip symlinks
> where the final character of the destination is a newline if we didn't
> handle trailing newlines properly.  As a result, we're not likely to
> change things here because it's required for correctness.

Just a fun thought experiment, but I wonder what would happen if a
user chooses to add .clean and .smudge filter that adds and strips
a LF at the end ;-)

Re: About git reporting missing newline for symlinks

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 439 bytes --]

On 2022-10-12 at 21:34:55, Junio C Hamano wrote:
> Just a fun thought experiment, but I wonder what would happen if a
> user chooses to add .clean and .smudge filter that adds and strips
> a LF at the end ;-)

I don't believe those are invoked on symlinks, since I don't recall us
having to handle symlinks in Git LFS, which works using a smudge and
clean filter.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: About git reporting missing newline for symlinks

[Ignacio Taranto]

That was very well explained, thank you.

The problem seems to be that some people interpret this message as a
real warning since it's often a good practice to always end text files
with a newline (at least on *nix systems).
Of course this doesn't apply to symlinks at all.

On Wed, Oct 12, 2022 at 6:31 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Ignacio Taranto <ignacio.taranto@eclypsium.com> writes:
>
> > So, both git diff and git show display "\ No newline at end of file"
> > for symlinks.
> >
> > I think this is related to how Git renders diffs, IMO it shouldn't
> > display a warning about newlines for symlinks.
> >
> > Am I making any sense here?
>
> Yes, but not really.
>
> It is not "warning" at all.  The users want to know when compared
> contents do or do not end with an incomplete line at the end, and
> the "\ No newline" is the diff's way to give that single bit of
> information to us.
>
> And the contents of a symbolic link typically is a single incomplete
> line, so it is expected to see "\ No newline" when comparing them.
>
> This is important as "git diff | git -C ../some/where/else apply"
> should be able to recreate the change in the current repository
> (which may have change to or addition of a symbolic link) in the
> other repository, and "git apply" on the receiving end must be able
> to tell that the symbolic link it needs to create in the other
> repository must not have an extra LF at the end.
>

Re: About git reporting missing newline for symlinks

[Taylor Blau]

On Wed, Oct 12, 2022 at 09:42:12PM +0000, brian m. carlson wrote:
> On 2022-10-12 at 21:34:55, Junio C Hamano wrote:
> > Just a fun thought experiment, but I wonder what would happen if a
> > user chooses to add .clean and .smudge filter that adds and strips
> > a LF at the end ;-)
>
> I don't believe those are invoked on symlinks, since I don't recall us
> having to handle symlinks in Git LFS, which works using a smudge and
> clean filter.

That is right. It matches my memory, at least, though it has been a very
long time since I've thought that hard about the clean / smudge code.

Thanks,
Taylor

Re: About git reporting missing newline for symlinks

[Ævar Arnfjörð Bjarmason]

On Wed, Oct 12 2022, Junio C Hamano wrote:

> Ignacio Taranto <ignacio.taranto@eclypsium.com> writes:
>
>> So, both git diff and git show display "\ No newline at end of file"
>> for symlinks.
>>
>> I think this is related to how Git renders diffs, IMO it shouldn't
>> display a warning about newlines for symlinks.
>>
>> Am I making any sense here?
>
> Yes, but not really.
>
> It is not "warning" at all.  The users want to know when compared
> contents do or do not end with an incomplete line at the end, and
> the "\ No newline" is the diff's way to give that single bit of
> information to us.
>
> And the contents of a symbolic link typically is a single incomplete
> line, so it is expected to see "\ No newline" when comparing them.
>
> This is important as "git diff | git -C ../some/where/else apply"
> should be able to recreate the change in the current repository
> (which may have change to or addition of a symbolic link) in the
> other repository, and "git apply" on the receiving end must be able
> to tell that the symbolic link it needs to create in the other
> repository must not have an extra LF at the end.

In apply.c's parse_fragment() we seem to only care that we find a
"\"-line that's at least the length of "\ No newline...".

I wonder what (if any) compatibility issues we'd have if we emitted
e.g.:

	\ The filename pointed to by the symlink does not end in a newline

Re: About git reporting missing newline for symlinks

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> In apply.c's parse_fragment() we seem to only care that we find a
> "\"-line that's at least the length of "\ No newline...".

If I recall correctly, this was done very much on purpose back in
2005 on top of what I and Linus did originally (which was specific
to "No newline" written in C locale).  Because the input from "git
apply" may not necessarily be the output from "git diff", and in the
older days, "git diff" spawned platform "diff" to show diff, it would
have been subject to l10n.

With GNU diff, I can do

    $ echo -n one >/var/tmp/1; echo -n two >/var/tmp/2
    $ LC_ALL=ja_JP.utf8 diff -u /var/tmp/[12]
    --- /var/tmp/1  2022-10-13 08:40:55.393209930 -0700
    +++ /var/tmp/2  2022-10-13 08:40:59.269538397 -0700
    @@ -1 +1 @@
    -one
    \ ファイル末尾に改行がありません
    +two
    \ ファイル末尾に改行がありません

As it does not consider a file that ends with an incomplete line a
text file [*], I do not think POSIX says anything about lines that
begin with "\" in the "diff" output, but I would be surprised if GNU
patch (or anybody else's, for that matter) did not ignore text on
the line, because all of them must be able to parse the above
output.

> I wonder what (if any) compatibility issues we'd have if we emitted
> e.g.:
>
> 	\ The filename pointed to by the symlink does not end in a newline

While I do not think it would break anybody, I doubt it would give
us much value.  One line above that output is a line that any user,
who is vaguely familiar with the contents being compared, can
recognize as giving a pathname, the contents of the symbolic link.


[References]

https://pubs.opengroup.org/onlinepubs/9699919799/xrat/V4_xbd_chap03.html#tag_21_03_00_83

Re: About git reporting missing newline for symlinks

[Ævar Arnfjörð Bjarmason]

On Thu, Oct 13 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> In apply.c's parse_fragment() we seem to only care that we find a
>> "\"-line that's at least the length of "\ No newline...".
>
> If I recall correctly, this was done very much on purpose back in
> 2005 on top of what I and Linus did originally (which was specific
> to "No newline" written in C locale).  Because the input from "git
> apply" may not necessarily be the output from "git diff", and in the
> older days, "git diff" spawned platform "diff" to show diff, it would
> have been subject to l10n.
>
> With GNU diff, I can do
>
>     $ echo -n one >/var/tmp/1; echo -n two >/var/tmp/2
>     $ LC_ALL=ja_JP.utf8 diff -u /var/tmp/[12]
>     --- /var/tmp/1  2022-10-13 08:40:55.393209930 -0700
>     +++ /var/tmp/2  2022-10-13 08:40:59.269538397 -0700
>     @@ -1 +1 @@
>     -one
>     \ ファイル末尾に改行がありません
>     +two
>     \ ファイル末尾に改行がありません
>
> As it does not consider a file that ends with an incomplete line a
> text file [*], I do not think POSIX says anything about lines that
> begin with "\" in the "diff" output, but I would be surprised if GNU
> patch (or anybody else's, for that matter) did not ignore text on
> the line, because all of them must be able to parse the above
> output.

That's really informative, thanks.

>> I wonder what (if any) compatibility issues we'd have if we emitted
>> e.g.:
>>
>> 	\ The filename pointed to by the symlink does not end in a newline
>
> While I do not think it would break anybody, I doubt it would give
> us much value.  One line above that output is a line that any user,
> who is vaguely familiar with the contents being compared, can
> recognize as giving a pathname, the contents of the symbolic link.

Clearly it confused the initial reporter upthread :) I'm not going to
pursue it, but it seems to me that we're being sloppy here in including
a message that makes more sense for a normal diff when we could consider
the mode(s) involved, as it's not abnormal for a filename to be missing
a trailing "\n" (having a "\n" being the odd case).

Anyway, I'm not going to pursue changing the message, but maybe if
someone else is interested the above compatibility notes would be
valuable, thanks!

Re: About git reporting missing newline for symlinks

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>>> 	\ The filename pointed to by the symlink does not end in a newline
>>
>> While I do not think it would break anybody, I doubt it would give
>> us much value.  One line above that output is a line that any user,
>> who is vaguely familiar with the contents being compared, can
>> recognize as giving a pathname, the contents of the symbolic link.
>
> Clearly it confused the initial reporter upthread :)

But to such a user, I highly suspect that the rephased message above
still looks like a warning, and will result in the same reaction.

IOW, you want to explain why "does not end in a newline" is worth
expressing in the output.  Saying "does not end in a newline" alone
would tell the user what they already know (i.e. the symlink stores
the target filename without an extra LF at the end).

Re: About git reporting missing newline for symlinks

[Ævar Arnfjörð Bjarmason]

On Thu, Oct 13 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>>>> 	\ The filename pointed to by the symlink does not end in a newline
>>>
>>> While I do not think it would break anybody, I doubt it would give
>>> us much value.  One line above that output is a line that any user,
>>> who is vaguely familiar with the contents being compared, can
>>> recognize as giving a pathname, the contents of the symbolic link.
>>
>> Clearly it confused the initial reporter upthread :)
>
> But to such a user, I highly suspect that the rephased message above
> still looks like a warning, and will result in the same reaction.
>
> IOW, you want to explain why "does not end in a newline" is worth
> expressing in the output.  Saying "does not end in a newline" alone
> would tell the user what they already know (i.e. the symlink stores
> the target filename without an extra LF at the end).

Yes, but isn't the point of the report/confusion that we're inserting
what looks like the warning you get when you forget a \n at the end of a
source file, so a user might wonder why they're seeing it at all.

Whereas what we're *really* doing there is not really about that at all,
but just inserting a bit of magic so that the diff format & its
consumers grok that this line we're inserting there isn't supposed to
have a \n, as we're working with a filename.

Maybe e.g.:
	
	diff --git a/RelNotes b/RelNotes
	index d505db645be..758368388a4 120000
	--- a/RelNotes
	+++ b/RelNotes
	@@ -1 +1 @@
	-Documentation/RelNotes/2.38.0.txt
	\ No newline at end of file
	+Documentation/RelNotes/2.39.0.txt
	\ No newline at end of file

Would, for those users, be less confusing as:

	diff --git a/RelNotes b/RelNotes
	index d505db645be..758368388a4 120000
	--- a/RelNotes
	+++ b/RelNotes
	@@ -1 +1 @@
	-Documentation/RelNotes/2.38.0.txt
	\ The symlink above has no trailing NL in its filename
	+Documentation/RelNotes/2.39.0.txt
	\ The symlink above has no trailing NL in its filename

*dunno* :)
	

Re: About git reporting missing newline for symlinks

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> Would, for those users, be less confusing as:
>
> 	diff --git a/RelNotes b/RelNotes
> 	index d505db645be..758368388a4 120000
> 	--- a/RelNotes
> 	+++ b/RelNotes
> 	@@ -1 +1 @@
> 	-Documentation/RelNotes/2.38.0.txt
> 	\ The symlink above has no trailing NL in its filename
> 	+Documentation/RelNotes/2.39.0.txt
> 	\ The symlink above has no trailing NL in its filename
>
> *dunno* :)

I do not think so, but "The symlink lacks the terminating LF and it
is perfectly normal" would be an OK thing to say.  It would make it
clear that it is not a warning message.

Re: About git reporting missing newline for symlinks

[Philip Oakley]

On 13/10/2022 20:33, Ævar Arnfjörð Bjarmason wrote:
> On Thu, Oct 13 2022, Junio C Hamano wrote:
>
>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>>
>>>>> 	\ The filename pointed to by the symlink does not end in a newline
>>>> While I do not think it would break anybody, I doubt it would give
>>>> us much value.  One line above that output is a line that any user,
>>>> who is vaguely familiar with the contents being compared, can
>>>> recognize as giving a pathname, the contents of the symbolic link.
>>> Clearly it confused the initial reporter upthread :)
>> But to such a user, I highly suspect that the rephased message above
>> still looks like a warning, and will result in the same reaction.
>>
>> IOW, you want to explain why "does not end in a newline" is worth
>> expressing in the output.  Saying "does not end in a newline" alone
>> would tell the user what they already know (i.e. the symlink stores
>> the target filename without an extra LF at the end).
> Yes, but isn't the point of the report/confusion that we're inserting
> what looks like the warning you get when you forget a \n at the end of a
> source file, so a user might wonder why they're seeing it at all.
>
> Whereas what we're *really* doing there is not really about that at all,
> but just inserting a bit of magic so that the diff format & its
> consumers grok that this line we're inserting there isn't supposed to
> have a \n, as we're working with a filename.
>
> Maybe e.g.:
> 	
> 	diff --git a/RelNotes b/RelNotes
> 	index d505db645be..758368388a4 120000
> 	--- a/RelNotes
> 	+++ b/RelNotes
> 	@@ -1 +1 @@
> 	-Documentation/RelNotes/2.38.0.txt
> 	\ No newline at end of file
> 	+Documentation/RelNotes/2.39.0.txt
> 	\ No newline at end of file
>
> Would, for those users, be less confusing as:
>
> 	diff --git a/RelNotes b/RelNotes
> 	index d505db645be..758368388a4 120000
> 	--- a/RelNotes
> 	+++ b/RelNotes
> 	@@ -1 +1 @@
> 	-Documentation/RelNotes/2.38.0.txt
> 	\ The symlink above has no trailing NL in its filename
> 	+Documentation/RelNotes/2.39.0.txt
> 	\ The symlink above has no trailing NL in its filename
>
> *dunno* :)
> 	
Pondering on why it is thought of as a warning to be treated as an
error, maybe the message implies something is missing via the negative
"No" assertion, rather than simply being informative [1].

Perhaps swapping out the "No" and instead using something like "\
Without a newline at end of file" would better convey that it's just
information about the file's format, and in no way a real problem. Just
a thought.
--
Philip
[1] My comment in Git Users discussion:
https://groups.google.com/d/msgid/git-users/c4f53bb2-cff6-4a76-8fa7-cd34ca88ce63n%40googlegroups.com
<https://groups.google.com/d/msgid/git-users/c4f53bb2-cff6-4a76-8fa7-cd34ca88ce63n%40googlegroups.com?utm_medium=email&utm_source=footer>

Re: About git reporting missing newline for symlinks

[Erik Cervin Edin]

On Thu, Oct 13, 2022 at 10:55 PM Philip Oakley <philipoakley@iee.email> wrote:
>
> Pondering on why it is thought of as a warning to be treated as an
> error, maybe the message implies something is missing via the negative
> "No" assertion, rather than simply being informative [1].

Very likely. I imagine most users use diff to view the
changes they made without knowledge of the use of diffs in git-apply.
Without that background it's more likely to be interpreted as an
opinionated comment as opposed to being informative.

> Perhaps swapping out the "No" and instead using something like "\
> Without a newline at end of file" would better convey that it's just
> information about the file's format, and in no way a real problem. Just
> a thought.

That's probably less likely to be read as a warning