Re: [PATCH v3] sha1_file: pass empty buffer to index empty file

Junio C Hamano [off-list ref] writes:

If the contents to be cleaned is small enough (i.e. the one-liner
file used in this test) to fit in the pipe buffer and we feed the
pipe before 'true' exits, we won't see any problem.  Otherwise we
may get SIGPIPE when we attempt to write to the 'true' (non-)filter,
but because we explicitly ignore SIGPIPE, 'true' still is a "black
hole" filter.

"cat >/dev/null" may have been a more naive and straight-forward way
to write this "black hole" filter, but what you did is fine.

I spoke too fast X-<.  "while sh t0021-*.sh; do :; done" dies after
a few iterations and with this squashed in it doesn't.

 t/t0021-conversion.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index 42e6423..b778faf 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh

@@ -218,7 +218,7 @@ test_expect_success "filter: clean empty file" '
 '
 
 test_expect_success "filter: smudge empty file" '
-	git config filter.empty-in-repo.clean true &&
+	git config filter.empty-in-repo.clean "cat >/dev/null" &&
 	git config filter.empty-in-repo.smudge "echo smudged && cat" &&
 
 	echo "empty-in-repo filter=empty-in-repo" >>.gitattributes &&

-- 
2.4.1-374-g090bfc9

`git add` of an empty file with a filter pops complaints from
`copy_fd` about a bad file descriptor.

This traces back to these lines in sha1_file.c:index_core:

	if (!size) {
		ret = index_mem(sha1, NULL, size, type, path, flags);

The problem here is that content to be added to the index can be
supplied from an fd, or from a memory buffer, or from a pathname. This
call is supplying a NULL buffer pointer and a zero size.

Downstream logic takes the complete absence of a buffer to mean the
data is to be found elsewhere -- for instance, these, from convert.c:

	if (params->src) {
		write_err = (write_in_full(child_process.in, params->src, params->size) < 0);
	} else {
		write_err = copy_fd(params->fd, child_process.in);
	}

~If there's a buffer, write from that, otherwise the data must be coming
from an open fd.~

Perfectly reasonable logic in a routine that's going to write from
either a buffer or an fd.

So change `index_core` to supply an empty buffer when indexing an empty
file.

There's a patch out there that instead changes the logic quoted above to
take a `-1` fd to mean "use the buffer", but it seems to me that the
distinction between a missing buffer and an empty one carries intrinsic
semantics, where the logic change is adapting the code to handle
incorrect arguments.

Signed-off-by: Jim Hill <redacted>
---
Okay, that's it: I'm officially laughably rusty, 'cause I'm laughing. This
applies your fix, _thank you_ for the caution. I get it: `true` can have
already exited by the time the write hits.

 sha1_file.c           |  2 +-
 t/t0021-conversion.sh | 26 ++++++++++++++++++++++++++
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/sha1_file.c b/sha1_file.c
index f860d67..61e2735 100644
--- a/sha1_file.c
+++ b/sha1_file.c

@@ -3186,7 +3186,7 @@ static int index_core(unsigned char *sha1, int fd, size_t size,
 	int ret;
 
 	if (!size) {
-		ret = index_mem(sha1, NULL, size, type, path, flags);
+		ret = index_mem(sha1, "", size, type, path, flags);
 	} else if (size <= SMALL_FILE_SIZE) {
 		char *buf = xmalloc(size);
 		if (size == read_in_full(fd, buf, size))

diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index ca7d2a6..eee4761 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh

@@ -216,4 +216,30 @@ test_expect_success EXPENSIVE 'filter large file' '
 	! test -s err
 '
 
+test_expect_success "filter: clean empty file" '
+	git config filter.in-repo-header.clean  "echo cleaned && cat" &&
+	git config filter.in-repo-header.smudge "sed 1d" &&
+
+	echo "empty-in-worktree    filter=in-repo-header" >>.gitattributes &&
+	>empty-in-worktree &&
+
+	echo cleaned >expected &&
+	git add empty-in-worktree &&
+	git show :empty-in-worktree >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success "filter: smudge empty file" '
+	git config filter.empty-in-repo.clean "cat >/dev/null" &&
+	git config filter.empty-in-repo.smudge "echo smudged && cat" &&
+
+	echo "empty-in-repo filter=empty-in-repo"  >>.gitattributes &&
+	echo dead data walking >empty-in-repo &&
+	git add empty-in-repo &&
+
+	echo smudged >expected &&
+	git checkout-index --prefix=filtered- empty-in-repo &&
+	test_cmp expected filtered-empty-in-repo
+'
+
 test_done

-- 
2.4.1.4.g08cda19

On Sun, May 17, 2015 at 12:10:49PM -0700, Junio C Hamano wrote:

Hmm, I thought we turned off SIGPIPE when writing to filters these days.
Looks like we still complain if we get EPIPE, though. I feel like it
should be the filter's business whether it wants to consume all of the
input or not[1], and we should only be checking its exit status.

-Peff

[1] As a practical example, consider a file format that has a lot of
    cruft at the end. The clean filter would want to read only to the
    start of the cruft, and then stop for reasons of efficiency.