Jim Hill <gjthill@xxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html