Re: Bug: problem with file named with dash character

Subsystems: the rest

3 messages, 2 authors, 2016-06-15 · open the first message on its own page

Re: Bug: problem with file named with dash character

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:54:11

Daniel Lyubomirov -|- Digitalus Bulgaria [off-list ref]
writes:

Аccidentally my colleague created a file in the root dir of the git repo called - (just dash).
As result for every commit having this file, diff , merge, cherry-pick maybe others just hang.

Thanks for a report.  I think the "diff" callchain should be
refactored so that the caller can mark the special "stdin" token in
a saner way, but until it happens, the following one-liner should
do.


 diff.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/diff.c b/diff.c
index 1a594df..caa2309 100644
--- a/diff.c
+++ b/diff.c

@@ -2589,6 +2589,14 @@ static int reuse_worktree_file(const char *name, const unsigned char *sha1, int
 	if (!FAST_WORKING_DIRECTORY && !want_file && has_sha1_pack(sha1))
 		return 0;
 
+	/*
+	 * And asking to read "-" from the working tree triggers stdin
+	 * input (which needs to be fixed separately by refactoring the
+	 * callchain), forbid "reuse" for now.
+	 */
+	if (!strcmp(name, "-"))
+		return 0;
+
 	len = strlen(name);
 	pos = cache_name_pos(name, len);
 	if (pos < 0)

Re: Bug: problem with file named with dash character

From: Jeff King <hidden>
Date: 2016-06-15 22:54:11

On Wed, Jun 27, 2012 at 11:28:21AM -0700, Junio C Hamano wrote:

Thanks for a report.  I think the "diff" callchain should be
refactored so that the caller can mark the special "stdin" token in
a saner way, but until it happens, the following one-liner should
do.

Yeah, I assume this is there at all to support "diff --no-index -", so the
special marking should happen at that layer.

quoted hunk

diff --git a/diff.c b/diff.c
index 1a594df..caa2309 100644
--- a/diff.c
+++ b/diff.c

@@ -2589,6 +2589,14 @@ static int reuse_worktree_file(const char *name, const unsigned char *sha1, int
 	if (!FAST_WORKING_DIRECTORY && !want_file && has_sha1_pack(sha1))
 		return 0;
 
+	/*
+	 * And asking to read "-" from the working tree triggers stdin
+	 * input (which needs to be fixed separately by refactoring the
+	 * callchain), forbid "reuse" for now.
+	 */
+	if (!strcmp(name, "-"))
+		return 0;
+

Unfortunately this is not enough. The problematic code path is the call
to populate_from_stdin in diff_populate_filespec. And we follow that
conditional if reuse_worktree_file is true, _or_ if the sha1_valid flag
on the filespec is not set. We hit the latter due to the --no-index
case, but we can also hit it if we are comparing a working tree file in
the repo that is stat-dirty.

So without your patch, this reads from stdin:

  git init repo &&
  cd repo &&
  echo foo >- &&
  git add - &&
  git commit -m foo

when the commit command tries to generate the diff summary. Your patch
fixes it, but remains broken if you then do:

  echo changes >>- &&
  git diff

I think you'd want to do just do something like:

diff --git a/diff.c b/diff.c
index 1a594df..aac72b7 100644
--- a/diff.c
+++ b/diff.c

@@ -2684,9 +2684,6 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only)
 		struct stat st;
 		int fd;
 
-		if (!strcmp(s->path, "-"))
-			return populate_from_stdin(s);
-
 		if (lstat(s->path, &st) < 0) {
 			if (errno == ENOENT) {
 			err_empty:

to temporarily fix it. That breaks

  echo content | git diff --no-index - some-file

but that code path should be fixed properly (with a use_stdin flag in
the filespec).

-Peff

Re: Bug: problem with file named with dash character

From: Jeff King <hidden>
Date: 2016-06-15 22:54:11

On Wed, Jun 27, 2012 at 03:52:05PM -0400, Jeff King wrote:

quoted hunk

I think you'd want to do just do something like:

diff --git a/diff.c b/diff.c
index 1a594df..aac72b7 100644
--- a/diff.c
+++ b/diff.c

@@ -2684,9 +2684,6 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only)
 		struct stat st;
 		int fd;
 
-		if (!strcmp(s->path, "-"))
-			return populate_from_stdin(s);
-
 		if (lstat(s->path, &st) < 0) {
 			if (errno == ENOENT) {
 			err_empty:

to temporarily fix it. That breaks

  echo content | git diff --no-index - some-file

but that code path should be fixed properly (with a use_stdin flag in
the filespec).

Something like the patch below, which keeps the stdin test in t4002
working for me.

I suspect there are other problems lurking with the stdin case. For
example, we try to drop filespec contents whenever we can to reduce
memory pressure, under the assumption that we can always re-read the
blob later. But with stdin, we would need to be careful to mark the
contents as precious somehow.

diff --git a/diff-no-index.c b/diff-no-index.c
index f0b0010..c64bd5c 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c

@@ -51,6 +51,21 @@ static int get_mode(const char *path, int *mode)
 	return 0;
 }
 
+static struct diff_filespec *noindex_filespec(const char *name, int mode)
+{
+	struct diff_filespec *r;
+
+	if (!name)
+		name = "/dev/null";
+	r = alloc_filespec(name);
+
+	fill_filespec(r, null_sha1, mode);
+	if (!strcmp(name, "-"))
+		r->is_stdin = 1;
+
+	return r;
+}
+
 static int queue_diff(struct diff_options *o,
 		      const char *name1, const char *name2)
 {

@@ -137,15 +152,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 = noindex_filespec(name1, mode1);
+		d2 = noindex_filespec(name2, mode2);
 		diff_queue(&diff_queued_diff, d1, d2);
 		return 0;
 	}

diff --git a/diff.c b/diff.c
index 1a594df..449bfba 100644
--- a/diff.c
+++ b/diff.c

@@ -2675,6 +2675,9 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only)
 	if (size_only && 0 < s->size)
 		return 0;
 
+	if (s->is_stdin)
+		return populate_from_stdin(s);
+
 	if (S_ISGITLINK(s->mode))
 		return diff_populate_gitlink(s, size_only);

@@ -2684,9 +2687,6 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only)
 		struct stat st;
 		int fd;
 
-		if (!strcmp(s->path, "-"))
-			return populate_from_stdin(s);
-
 		if (lstat(s->path, &st) < 0) {
 			if (errno == ENOENT) {
 			err_empty:

diff --git a/diffcore.h b/diffcore.h
index 8f32b82..be0739c 100644
--- a/diffcore.h
+++ b/diffcore.h

@@ -43,6 +43,7 @@ struct diff_filespec {
 	unsigned should_free : 1; /* data should be free()'ed */
 	unsigned should_munmap : 1; /* data should be munmap()'ed */
 	unsigned dirty_submodule : 2;  /* For submodules: its work tree is dirty */
+	unsigned is_stdin : 1;
 #define DIRTY_SUBMODULE_UNTRACKED 1
 #define DIRTY_SUBMODULE_MODIFIED  2
 	unsigned has_more_entries : 1; /* only appear in combined diff */

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help