Re: [BUG, PATCH 0/3] Fix {blame,cat-file} --textconv for cases with symlinks
- From
Jeff King <peff@peff.net>
- Date
- Sep 21, 2010, 18:56 UTC
- Message-ID
- <20100921185613.GB28567@sigill.intra.peff.net>
- In-Reply-To
- <20100921184241.GA28567@sigill.intra.peff.net>
On Tue, Sep 21, 2010 at 02:42:41PM -0400, Jeff King wrote:
Show 29 quoted lines
> diff --git a/diff.c b/diff.c
> index 6fb18ae..58c4477 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -1771,8 +1771,14 @@ static void emit_binary_diff(FILE *file, mmfile_t *one, mmfile_t *two, char *pre
>
> static void diff_filespec_load_driver(struct diff_filespec *one)
> {
> - if (!one->driver)
> + if (one->driver)
> + return;
> +
> + if (S_ISLNK(one->mode))
> + one->driver = userdiff_find_by_name("SYMLINK");
> + else
> one->driver = userdiff_find_by_path(one->path);
> +
> if (!one->driver)
> one->driver = userdiff_find_by_name("default");
> }
> @@ -1820,7 +1826,7 @@ struct userdiff_driver *get_textconv(struct diff_filespec *one)
> {
> if (!DIFF_FILE_VALID(one))
> return NULL;
> - if (!S_ISREG(one->mode))
> + if (!S_ISREG(one->mode) && !S_ISLNK(one->mode))
> return NULL;
> diff_filespec_load_driver(one);
> if (!one->driver->textconv)Actually, thinking about this a little more, I wonder if the test for S_ISREG should perhaps have been in diff_filespec_load_driver from the very beginning. With the current code, I can do:
$ echo 'binary content' >file.bin $ ln -s file.bin link.bin $ echo '*.bin diff=bin' >.gitattributes $ git config diff.bin.binary true $ git add -N . $ git diff diff --git a/.gitattributes b/.gitattributes index e69de29..d38ad2e 100644 --- a/.gitattributes +++ b/.gitattributes @@ -0,0 +1 @@ +*.bin diff=bin diff --git a/file.bin b/file.bin index e69de29..f55142d 100644 Binary files a/file.bin and b/file.bin differ diff --git a/link.bin b/link.bin index e69de29..dce41ec 120000 Binary files a/link.bin and b/link.bin differ
which does not seem right. We are again acting on the symlink's contents, which are a text pathname, as if they represented the content of that pathname.
So I think that is a bug, albeit one that is relatively uncommon to trigger. S_ISGITLINK files may have the same issues, but perhaps not, as I think they get routed out of the regular diff codepath early.
-Peff