Dennis Kaarsemaker <dennis@xxxxxxxxxxxxxxx> writes:
> + /*
> + * In --no-index mode, we support reading from pipes. canon_mode, called by
> + * fill_filespec, gets confused by this and thinks we now have subprojects.
> + * Detect this and tell the rest of the diff machinery to treat pipes as
> + * normal files.
> + */
> + if (S_ISGITLINK(s->mode))
> + s->mode = S_IFREG | ce_permissions(mode);
Hmph. Pipes on your system may satisfy S_ISGITLINK() and confuse
later code, and this hack may work it around. But a proper gitlink
that was thrown at this codepath (probably by mistake) will also be
caught and pretend as if it were a regular file. Do we know for
certain that pipes everywhere will be munged to appear as
S_ISGITLINK()? Is it possible to do the "are we looking at an end
of a pipe?" check _before_ canon_mode() munges and stores the result
in s->mode in diff-no-index.c somewhere, perhaps inside get_mode()?
> diff --git a/diff.c b/diff.c
> index 2fc0226338..bb04eab331 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -2839,9 +2839,18 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)
> fd = open(s->path, O_RDONLY);
> if (fd < 0)
> goto err_empty;
> - s->data = xmmap(NULL, s->size, PROT_READ, MAP_PRIVATE, fd, 0);
> + if (!S_ISREG(st.st_mode)) {
> + struct strbuf sb = STRBUF_INIT;
> + strbuf_read(&sb, fd, 0);
> + s->size = sb.len;
> + s->data = strbuf_detach(&sb, NULL);
> + s->should_free = 1;
> + }
> + else {
> + s->data = xmmap(NULL, s->size, PROT_READ, MAP_PRIVATE, fd, 0);
> + s->should_munmap = 1;
> + }
> close(fd);
> - s->should_munmap = 1;
I like the fact that, by extending the !S_ISREG() check this patch
introduces, we can later use the new "do not mmap but allocate to
read" codepath for small files, which may be more efficient. We may
want to have a small helper
static int should_mmap_file_contents(struct stat *st)
{
return S_ISREG(st->st_mode);
}
so that we can do such an enhancement later more easily.
So, I am skeptical with the "do we have pipe" check in the earlier
hunk, but otherwise I think what this patch wanted to solve is a
reasonable problem to tackle.
Thanks.