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

From: Jeff King <peff@peff.net>
Date: Jun 3, 2010, 20:17 UTC
Message-ID: <20100603201759.GA20120@sigill.intra.peff.net>
In-Reply-To: <4C0808CE.2000506@drmicha.warpmail.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