Re: [PATCH v4 1/2] diff --no-index: optionally follow symlinks

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 24, 2017, 22:56 UTC
Message-ID: <xmqqziga5lnn.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170324213110.4331-2-dennis@kaarsemaker.net>

Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:

Show 9 quoted lines

> @@ -52,7 +52,7 @@ static int get_mode(const char *path, int *mode)
>  #endif
>  	else if (path == file_from_standard_input)
>  		*mode = create_ce_mode(0666);
> -	else if (lstat(path, &st))
> +	else if (dereference ? stat(path, &st) : lstat(path, &st))
>  		return error("Could not access '%s'", path);
>  	else
>  		*mode = st.st_mode;

This part makes sense---when the caller tells us to stat() we stat(), otherwise, we lstat().

Show 10 quoted lines

> diff --git a/diff.c b/diff.c
> index be11e4ef2b..2afecfb939 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -2815,7 +2815,7 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)
>  		s->size = xsize_t(st.st_size);
>  		if (!s->size)
>  			goto empty;
> -		if (S_ISLNK(st.st_mode)) {
> +		if (S_ISLNK(s->mode)) {

This change is conceptually wrong. s->mode (often) comes from the index but in this codepath, after finding that s->oid is not valid or we want to read from the working tree instead (several lines before this part), we are committed to read from the working tree and check things with st.st_* fields, not s->mode, when we decide what to do with the thing we find on the filesystem, no?

Show 11 quoted lines

> @@ -2825,6 +2825,10 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)
>  			s->should_free = 1;
>  			return 0;
>  		}
> +		if (S_ISLNK(st.st_mode)) {
> +			stat(s->path, &st);
> +			s->size = xsize_t(st.st_size);
> +		}
>  		if (size_only)
>  			return 0;
>  		if ((flags & CHECK_BINARY) &&

I suspect that this would conflict with a recent topic.

But more importantly, this inserted code feels doubly wrong.

 - what allows us to unconditionally do "ah, symbolic link on the
   disk--find the target of the link, not the symbolic link itself"?
   We do not seem to be checking '--dereference' around here.

 - does this code do a reasonable thing when the path is a symbolic
   link that points at a directory?  what does it mean to grab
   st.st_size for such a thing (and then go on to open() and xmmap()
   it)?

Puzzled.

Thanks.