Re: [PATCH v3] 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 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>
> ---
> I promise to pay more attention to test quality in the future, thanks for the
> patience.

It's us who should thank you ;-).  Thanks for spending time to
polish essentially a one-liner this long.


>
>  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..bf87e9b 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 true &&
> +	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