[PATCH 4/8] diff: support reading a file from stdin via "-"

[PATCH 4/8] diff: support reading a file from stdin via "-"

[Johannes Schindelin]

This allows you to say

	echo Hello World | git diff x -

to compare the contents of file "x" with the line "Hello World".
This automatically switches to --no-index mode.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	Since the revision machinery checks for the presence of files,
	diff_populate_filespec() will only change behaviour when there
	is a file "-"... I have yet to think of an elegant fix for that.

 diff-lib.c |   16 +++++++++++-----
 diff.c     |   20 ++++++++++++++++++++
 2 files changed, 31 insertions(+), 5 deletions(-)

diff --git a/diff-lib.c b/diff-lib.c
index 6678e22..089c94c 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -37,14 +37,20 @@ static int queue_diff(struct diff_options *o,
 	int mode1 = 0, mode2 = 0;
 
 	if (name1) {
-		if (stat(name1, &st))
+		if (!strcmp(name1, "-"))
+			mode1 = 0644;
+		else if (stat(name1, &st))
 			return error("Could not access '%s'", name1);
-		mode1 = st.st_mode;
+		else
+			mode1 = st.st_mode;
 	}
 	if (name2) {
-		if (stat(name2, &st))
+		if (!strcmp(name2, "-"))
+			mode2 = 0644;
+		else if (stat(name2, &st))
 			return error("Could not access '%s'", name2);
-		mode2 = st.st_mode;
+		else
+			mode2 = st.st_mode;
 	}
 
 	if (mode1 && mode2 && S_ISDIR(mode1) != S_ISDIR(mode2))
@@ -224,7 +230,7 @@ int setup_diff_no_index(struct rev_info *revs,
 {
 	int i;
 	for (i = 1; i < argc; i++)
-		if (argv[i][0] != '-')
+		if (argv[i][0] != '-' || argv[i][1] == '\0')
 			break;
 		else if (!strcmp(argv[i], "--")) {
 			i++;
diff --git a/diff.c b/diff.c
index 5651152..31129d1 100644
--- a/diff.c
+++ b/diff.c
@@ -1389,6 +1389,22 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only)
 		char *buf;
 		unsigned long size;
 
+		if (!strcmp(s->path, "-")) {
+#define INCREMENT 1024
+			int i = INCREMENT;
+			size = 0;
+			buf = NULL;
+			while (i == INCREMENT) {
+				buf = xrealloc(buf, size + INCREMENT);
+				i = xread(0, buf + size, INCREMENT);
+				size += i;
+			}
+			s->should_munmap = 0;
+			s->data = buf;
+			s->size = size;
+			s->should_free = 1;
+			return 0;
+		}
 		if (lstat(s->path, &st) < 0) {
 			if (errno == ENOENT) {
 			err_empty:
@@ -1688,6 +1704,10 @@ static void diff_fill_sha1_info(struct diff_filespec *one)
 	if (DIFF_FILE_VALID(one)) {
 		if (!one->sha1_valid) {
 			struct stat st;
+			if (!strcmp(one->path, "-")) {
+				hashcpy(one->sha1, null_sha1);
+				return;
+			}
 			if (lstat(one->path, &st) < 0)
 				die("stat %s", one->path);
 			if (index_path(one->sha1, one->path, &st, 0))
-- 
1.5.0.1.788.g8ca52

Re: [PATCH 4/8] diff: support reading a file from stdin via "-"

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> This allows you to say
>
> 	echo Hello World | git diff x -
>
> to compare the contents of file "x" with the line "Hello World".
> This automatically switches to --no-index mode.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>
> 	Since the revision machinery checks for the presence of files,
> 	diff_populate_filespec() will only change behaviour when there
> 	is a file "-"... I have yet to think of an elegant fix for that.

Another thing is that at some point diff_populate_filespec()
needs to have a way to discard what was cached if memory
pressure gets tight, and we would want to keep this data read
from the standard input.

One solution would be to add a "const char *stdin_data" to diffopts
and read the data from stdin when you parse the options, and
have populate_filespec point at that with s->data (setting
should_free and should_munmap both to 0).

Re: [PATCH 4/8] diff: support reading a file from stdin via "-"

[Johannes Schindelin]

Hi,

On Sun, 25 Feb 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > This allows you to say
> >
> > 	echo Hello World | git diff x -
> >
> > to compare the contents of file "x" with the line "Hello World".
> > This automatically switches to --no-index mode.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >
> > 	Since the revision machinery checks for the presence of files,
> > 	diff_populate_filespec() will only change behaviour when there
> > 	is a file "-"... I have yet to think of an elegant fix for that.
> 
> Another thing is that at some point diff_populate_filespec()
> needs to have a way to discard what was cached if memory
> pressure gets tight, and we would want to keep this data read
> from the standard input.

I see this purely for purposes of --no-index diff. And in that case, we 
only compare one file pair. Either one of them is from stdin, or it is 
not. Therefore, if memory gets tight, we cannot compare that file pair 
anyway, and have to error out.

Maybe I missed something obvious?

Ciao,
Dscho

Re: [PATCH 4/8] diff: support reading a file from stdin via "-"

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> I see this purely for purposes of --no-index diff. And in that case, we 
> only compare one file pair. Either one of them is from stdin, or it is 
> not. Therefore, if memory gets tight, we cannot compare that file pair 
> anyway, and have to error out.
>
> Maybe I missed something obvious?

I think the responsibility to mark if a path whose name is "-"
is a filesystem object or stdin lies within the option parser.
If the user says "diff - a" or diff a -" then the user clearly
means the stdin, while if the user says "diff ./- a" it is a
path (and the later stages like populate_filespec() would see
canonicalized "-" as the filename).  The point is that the
disambiguation must be done by somebody, and I think the option
parser, who calls get_pathspec(), should be the one that does
it.

So we at least need a bit in diff_options that lets the option
parser to tell the later stages "if you see - in pathspec, the
user means stdin".  And my suggestion was that if you are going
to do that in the option parser anyway, you could read stdin in
the option parser and hang that data in diff_options, and the
presence of that stdin data (pointer being non NULL) can be used
as that signal.

I think the callers of populate_filespec() may need to pass
around diff_options as a parameter for the above to work, but
hopefully that should not be a rocket surgery.

Re: [PATCH 4/8] diff: support reading a file from stdin via "-"

[Junio C Hamano]

Another hack just occurred to me... get_pathspec() could
translate '-' to /dev/stdin while leaving './-' to '-'.

It could be rather too ugly to the taste of some people.
Myself, I dunno...

Re: [PATCH 4/8] diff: support reading a file from stdin via "-"

[Johannes Schindelin]

Hi,

On Sun, 25 Feb 2007, Junio C Hamano wrote:

> I think the callers of populate_filespec() may need to pass around 
> diff_options as a parameter for the above to work, but hopefully that 
> should not be a rocket surgery.

That's a good idea! Actually, that pointer can be the pointer in the 
diff_filespec itself!

This is my current version of [4/8] (Gosh, I really _love_ rebase -i ;-):

-- snipsnap --
[PATCH] diff: support reading a file from stdin via "-"

This allows you to say

	echo Hello World | git diff x -

to compare the contents of file "x" with the line "Hello World".
This automatically switches to --no-index mode.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	This gives revs->max_count yet another meaning, oh well...

	The diff is a bit borked, because get_mode() and the first part 
	of the old queue_diff() are apparently a little similar...

 diff-lib.c |   92 ++++++++++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 65 insertions(+), 27 deletions(-)

diff --git a/diff-lib.c b/diff-lib.c
index 6678e22..9302d2b 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -30,22 +30,61 @@ static int read_directory(const char *path, struct path_list *list)
 	return 0;
 }
 
-static int queue_diff(struct diff_options *o,
-		const char *name1, const char *name2)
+static int get_mode(const char *path, int *mode, int seen_dashdash)
 {
 	struct stat st;
-	int mode1 = 0, mode2 = 0;
 
-	if (name1) {
-		if (stat(name1, &st))
-			return error("Could not access '%s'", name1);
-		mode1 = st.st_mode;
+	if (!path)
+		*mode = 0;
+	else if (!seen_dashdash && !strcmp(path, "-"))
+		*mode = 020600;
+	else if (stat(path, &st))
+		return error("Could not access '%s'", path);
+	else
+		*mode = st.st_mode;
+	return 0;
+}
+
+static void read_stdin(void **buffer, unsigned long *size)
+{
+	char *buf = NULL;
+#define INCREMENT 1024
+	int i = INCREMENT;
+
+	*size = 0;
+	buf = NULL;
+	while (i == INCREMENT) {
+		buf = xrealloc(buf, *size + INCREMENT);
+		i = xread(0, buf + *size, INCREMENT);
+		*size += i;
 	}
-	if (name2) {
-		if (stat(name2, &st))
-			return error("Could not access '%s'", name2);
-		mode2 = st.st_mode;
+	*buffer = buf;
+}
+
+static struct diff_filespec *make_diff_filespec(const char *path, int mode)
+{
+	struct diff_filespec *result;
+	if (!path)
+		path = "/dev/null";
+	result = alloc_filespec(path);
+	fill_filespec(result, null_sha1, mode);
+	if (mode == 020600) {
+		read_stdin(&result->data, &result->size);
+		result->mode = 0100644;
+		result->should_munmap = 0;
+		result->should_free = 1;
 	}
+	return result;
+}
+
+static int queue_diff(struct diff_options *o,
+		const char *name1, const char *name2, int seen_dashdash)
+{
+	int mode1 = 0, mode2 = 0;
+
+	if (get_mode(name1, &mode1, seen_dashdash) ||
+			get_mode(name2, &mode2, seen_dashdash))
+		return -1;
 
 	if (mode1 && mode2 && S_ISDIR(mode1) != S_ISDIR(mode2))
 		return error("file/directory conflict: %s, %s", name1, name2);
@@ -106,7 +145,7 @@ static int queue_diff(struct diff_options *o,
 						PATH_MAX - len2);
 			}
 
-			ret = queue_diff(o, n1, n2);
+			ret = queue_diff(o, n1, n2, 1);
 		}
 		path_list_clear(&p1, 0);
 		path_list_clear(&p2, 0);
@@ -122,14 +161,8 @@ static int queue_diff(struct diff_options *o,
 			tmp_c = name1; name1 = name2; name2 = tmp_c;
 		}
 
-		if (!name1)
-			name1 = "/dev/null";
-		if (!name2)
-			name2 = "/dev/null";
-		d1 = alloc_filespec(name1);
-		d2 = alloc_filespec(name2);
-		fill_filespec(d1, null_sha1, mode1);
-		fill_filespec(d2, null_sha1, mode2);
+		d1 = make_diff_filespec(name1, mode1);
+		d2 = make_diff_filespec(name2, mode2);
 
 		diff_queue(&diff_queued_diff, d1, d2);
 		return 0;
@@ -222,11 +255,12 @@ static int is_outside_repo(const char *path, int nongit, const char *prefix)
 int setup_diff_no_index(struct rev_info *revs,
 		int argc, const char ** argv, int nongit, const char *prefix)
 {
-	int i;
+	int i, seen_dashdash = 0;
 	for (i = 1; i < argc; i++)
-		if (argv[i][0] != '-')
+		if (argv[i][0] != '-' || argv[i][1] == '\0')
 			break;
 		else if (!strcmp(argv[i], "--")) {
+			seen_dashdash = 1;
 			i++;
 			break;
 		} else if (i < argc - 3 && !strcmp(argv[i], "--no-index")) {
@@ -238,7 +272,7 @@ int setup_diff_no_index(struct rev_info *revs,
 		return -1;
 
 	diff_setup(&revs->diffopt);
-	for (i = 1; i < argc - 2; )
+	for (i = 1; i < argc - 2 - seen_dashdash; )
 		if (!strcmp(argv[i], "--no-index"))
 			i++;
 		else {
@@ -250,22 +284,26 @@ int setup_diff_no_index(struct rev_info *revs,
 		}
 	revs->diffopt.paths = argv + argc - 2;
 	revs->diffopt.nr_paths = 2;
-	revs->max_count = -2;
+	revs->max_count = -2 - seen_dashdash;
 	return 0;
 }
 
 int run_diff_files_cmd(struct rev_info *revs, int argc, const char **argv)
 {
-	int silent_on_removed;
+	int silent_on_removed, seen_dashdash = 0;
 
-	if (handle_diff_files_args(revs, argc, argv, &silent_on_removed))
+	if (revs->max_count == -3) {
+		seen_dashdash = 1;
+		revs->max_count = -2;
+	} else if (handle_diff_files_args(revs, argc, argv,
+				&silent_on_removed))
 		return -1;
 
 	if (revs->max_count == -2) {
 		if (revs->diffopt.nr_paths != 2)
 			return error("need two files/directories with --no-index");
 		if (queue_diff(&revs->diffopt, revs->diffopt.paths[0],
-				revs->diffopt.paths[1]))
+				revs->diffopt.paths[1], seen_dashdash))
 			return -1;
 		diffcore_std(&revs->diffopt);
 		diff_flush(&revs->diffopt);
-- 
1.5.0.1.784.g105d9-dirty