Jeff King <peff@xxxxxxxx> 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. > +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/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html