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

From: Junio C Hamano <gitster@pobox.com>
Date: May 14, 2015, 18:43 UTC
Message-ID: <xmqqbnhnknio.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1431624219-25045-1-git-send-email-gjthill@gmail.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.

Show 8 quoted lines

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

Show 33 quoted lines

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