[PATCH v2] sha1_file: pass empty buffer to index empty file

From: Jim Hill <gjthill@gmail.com>
Date: May 14, 2015, 23:17 UTC
Message-ID: <1431645434-11790-1-git-send-email-gjthill@gmail.com>
In-Reply-To: <xmqqbnhnknio.fsf@gitster.dls.corp.google.com>

`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 <gjthill@gmail.com>
---

Junio C Hamano wrote:

> Please lose that word.

Check.

> Good spotting.

Thanks :-)

> I am not enthused to see a new test script wasted just for one piece
> of test.  Don't we have other

Yup. Didn't see a place I liked for it in the add and update-index test suites, didn't think to look for a filter test suite... Check.

> Please do not be cryptic and show a good example, e.g.
> Also lose the blank line from that test.
>~Say `>emptyfile &&`

Checkcheckcheck

> check that 'err' does not contain the copy-fd error

Implemented this out of necessity, because the add works and returns success despite the complaints to stderr.

 sha1_file.c           |  2 +-
 t/t0021-conversion.sh | 11 +++++++++++
 2 files changed, 12 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..5986bb0 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh
@@ -216,4 +216,15 @@ test_expect_success EXPENSIVE 'filter large file' '
 	! test -s err
 '
 
+test_expect_success "filtering empty file should not produce complaints" '
+	echo "emptyfile filter=cat" >>.gitattributes &&
+	git config filter.cat.clean cat &&
+	git config filter.cat.smudge cat &&
+	git add . &&
+	git commit -m "cat filter for emptyfile" &&
+	> emptyfile &&
+	git add emptyfile 2>err &&
+	! grep -Fiqs "bad file descriptor" err
+'
+
 test_done

-- 
2.4.1.4.gd9c648d