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

[Junio C Hamano <gitster@pobox.com>]

Jim Hill <gjthill@gmail.com> writes:

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

Our log message typically begins with the description of a problem
that exists; "currently" is redundant in that context.  Please lose
that word.

> 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.

Good spotting.

I think the original thinking was that "we'd feed mem[0..size) to
the hash function, so mem being NULL should not matter", but as you
analysed, mem being NULL is used as a signal with a different meaning,
and your fix is the right one.

I am not enthused to see a new test script wasted just for one piece
of test.  Don't we have other "run 'git add' with clean/smudge
filters" test to which you can add this new one to?  If there is
none, then a new test script is good, but then it should be a place
to _add_ missing "run 'git add' with filters" test and its name
should not be so specific to this "empty" special case.

> 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/t2205-add-empty-filtered-file.sh b/t/t2205-add-empty-filtered-file.sh
> new file mode 100755
> index 0000000..28903c4
> --- /dev/null
> +++ b/t/t2205-add-empty-filtered-file.sh
> @@ -0,0 +1,21 @@
> +#!/bin/sh
> +
> +test_description='adding empty filtered file'
> +
> +. ./test-lib.sh
> +
> +test_expect_success setup '
> +	echo "* filter=test" >>.gitattributes &&
> +	git config filter.test.clean cat &&
> +	git config filter.test.smudge cat &&
> +	git add . &&
> +	git commit -m-
> +
> +'

Please do not be cryptic and show a good example, e.g.

	git commit -m test

Also lose the blank line from that test.

> +
> +test_expect_success "add of empty filtered file produces no complaints" '
> +	touch emptyfile &&

"touch" is to be used when you care about the timestamp.  When you
care more about the _presence_ of the file than what timestamp it
has, do not use "touch".  Say

	>emptyfile &&

instead.  This is especially true in this case, because you not only
care about the presence but you care about it being _empty_.

> +	git add emptyfile 2>out &&
> +	test -e out -a ! -s out

Future generation of Git users may want to see "git add emptyfile"
that succeeds to still say something and that something may be
different from "the complaint about a bad file descriptor".  Don't
force the person who makes such a change to update this test.

You may do

	git add emptyfile 2>err &&

and check that 'err' does not contain the copy-fd error (in other
words, this test is not in a good position to reject any other
output to the standard error stream), if you really wanted to, but I
do not think it is worth it.

My preference is to lose the test on 'out' and end this test like
this:

	git add emptyfile

> +'
> +test_done

Thanks.