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

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Sep 22, 2010, 05:40 UTC
Message-ID: <vpqvd5ycp3w.fsf@bauges.imag.fr>
In-Reply-To: <20100921210124.GA1188@sigill.intra.peff.net>

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)

> 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.

> I find it unlikely, and given the potential breakage, it seems more like
> exploiting a bug to get what you want.

Agreed.

Show 19 quoted lines

> +cat >expect <<'EOF'
> +diff --git a/file.bin b/file.bin
> +index e69de29..d95f3ad 100644
> +Binary files a/file.bin and b/file.bin differ
> +diff --git a/link.bin b/link.bin
> +index e69de29..dce41ec 120000
> +--- a/link.bin
> ++++ b/link.bin
> +@@ -0,0 +1 @@
> ++file.bin
> +\ No newline at end of file
> +EOF
> +test_expect_success SYMLINKS 'symlinks do not respect userdiff config by path' '
> +	git config diff.bin.binary true &&
> +	git diff file.bin link.bin >actual &&
> +	test_cmp expect actual
> +'
> +
>  test_done

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

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/