Re: [PATCH] diff: add support for reading files literally with --no-index

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jan 4, 2019, 02:18 UTC
Message-ID: <20190104021855.GB97188@google.com>
In-Reply-To: <20181220002610.43832-1-sandals@crustytoothpaste.net>

Hi,

brian m. carlson wrote:

Show 8 quoted lines

> In some shells, such as bash and zsh, it's possible to use a command
> substitution to provide the output of a command as a file argument to
> another process, like so:
>
>   diff -u <(printf "a\nb\n") <(printf "a\nc\n")
>
> However, this syntax does not produce useful results with git diff
> --no-index.

Thanks much for fixing this.  It's something I've run into, too.

[...]

Show 10 quoted lines

> --- a/Documentation/git-diff.txt
> +++ b/Documentation/git-diff.txt
> @@ -111,6 +111,11 @@ include::diff-options.txt[]
>  	"Unmerged".  Can be used only when comparing the working tree
>  	with the index.
>  
> +--literally::
> +  Read the specified files literally, as `diff` would,
> +  dereferencing any symlinks and reading data from pipes.
> +  This option only works with `--no-index`.

I may be a minority in this opinion, but I had trouble understanding what --literally would do from its name. I suspect we can come up with a better name.

Unfortunately, I'm terrible at coming up with names. :-P

--dereference would be a good name when it comes to symlinks, but it's not a good name for reading what is on the other side of a pipe. On the plus side, it matches "diff" and "cp"'s name for the "follow symbolic links" option.

--plain captures the desire a little better --- we want a plain read(2) from the file instead of trying to be smart and look at whether it is e.g. a block device. But in the context of "diff", that would seem more like an option that affects the output.

What would you think of

 - always reading from fifos instead of describing them, since I've
   never encountered a use case where people want the latter

 - --dereference to control whether to follow symlinks

[...]

Show 21 quoted lines

> --- a/diff-no-index.c
> +++ b/diff-no-index.c
> @@ -75,7 +75,25 @@ static int populate_from_stdin(struct diff_filespec *s)
>  	return 0;
>  }
>  
> -static struct diff_filespec *noindex_filespec(const char *name, int mode)
> +static int populate_literally(struct diff_filespec *s)
> +{
> +	struct strbuf buf = STRBUF_INIT;
> +	size_t size = 0;
> +	int fd = xopen(s->path, O_RDONLY);
> +
> +	if (strbuf_read(&buf, fd, 0) < 0)
> +		return error_errno("error while reading from '%s'", s->path);
> +
> +	s->should_munmap = 0;
> +	s->data = strbuf_detach(&buf, &size);
> +	s->size = size;
> +	s->should_free = 1;
> +	s->read_literally = 1;

Oh!  --read-literally works perfectly for me as a name. :)

Jonathan