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

[Jeff King <peff@peff.net>]

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