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

Michael J Gruber [off-list ref] 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/

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

Michael J Gruber [off-list ref] writes:

quoted

"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

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

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

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

quoted

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

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

quoted

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

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

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

Am 03.06.2010 19:07, schrieb Jeff King:

quoted

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