From: Junio C Hamano <gitster@pobox.com>
To: Dennis Kaarsemaker <dennis@kaarsemaker.net>
Cc: git@vger.kernel.org
Subject: Re: [PATCH v4 1/2] diff --no-index: optionally follow symlinks
Date: Fri, 24 Mar 2017 15:56:28 -0700 [thread overview]
Message-ID: <xmqqziga5lnn.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <20170324213110.4331-2-dennis@kaarsemaker.net> (Dennis Kaarsemaker's message of "Fri, 24 Mar 2017 22:31:09 +0100")
Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
> @@ -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().
> 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?
> @@ -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.
next prev parent reply other threads:[~2017-03-24 22:56 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-03-24 21:31 [PATCH v4 0/2] diff --no-index: support symlinks and pipes Dennis Kaarsemaker
2017-03-24 21:31 ` [PATCH v4 1/2] diff --no-index: optionally follow symlinks Dennis Kaarsemaker
2017-03-24 22:56 ` Junio C Hamano [this message]
2017-03-25 21:30 ` Dennis Kaarsemaker
2017-03-26 3:42 ` Junio C Hamano
2017-03-24 21:31 ` [PATCH v4 2/2] diff --no-index: support reading from pipes Dennis Kaarsemaker
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqziga5lnn.fsf@gitster.mtv.corp.google.com \
--to=gitster@pobox.com \
--cc=dennis@kaarsemaker.net \
--cc=git@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).