Re: [PATCH 1/2] diff: don't use pathname-based diff drivers for symlinks

From: Jeff King <peff@peff.net>
Date: Sep 22, 2010, 05:50 UTC
Message-ID: <20100922055006.GA2980@sigill.intra.peff.net>
In-Reply-To: <vpqvd5ycp3w.fsf@bauges.imag.fr>

On Wed, Sep 22, 2010 at 07:40:51AM +0200, Matthieu Moy wrote:

Show 8 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > We can drop the check in the textconv code, which is now redundant.
> 
> Am I correct if I say that this makes "[PATCH 3/3] RFC: blame,cat-file
> --textconv: Don't assume mode is ``S_IFREF | 0664''" obsolete?
> 
> (but patches 1 and 2 are still useful to check the behavior)

No. This just moves the check of the mode further down in the callstack. But the problem that Kirill's patch fixes is that we were feeding a bogus mode in the first place. So we still need to start actually sending the correct mode.

Show 10 quoted lines

> > Technically, this could be breaking somebody's setup if:
> >
> >   1. They found some use for userdiff config on symlinks. Textconv is
> >      already disabled. A custom diff driver might work.
> 
> I'm wondering about cases other than regular files and symlinks here.
> Directories are not a problem, since for Git, they somehow don't
> exist, but for example, "git diff" shows diff for subprojects too.
> After little testing, it seem the textconv is never applied on
> subprojects, but I couldn't say why.

Yeah, gitlinks were the only example I could come up with, too. The current textconv code already checked for S_ISREG, so you couldn't textconv them. I'm not sure if you could do an external diff, or mark them as binary. I suspect not, since we tend to treat them specially in the diff code, but I didn't actually test. At any rate, those things are explicitly disallowed by my change.

> > +cat >expect <<'EOF'
> [...]
> It's recommanded to put these cat <<'EOF' within the
> test_expect_success, but otherwise, the code looks good.

I didn't think we had actually reached consensus on that.

-Peff