From: Junio C Hamano <gitster@pobox•com>
To: Jim Hill <gjthill@gmail•com>
Cc: git@vger•kernel.org
Subject: Re: [PATCH] sha1_file: pass empty buffer to index empty file
Date: Thu, 14 May 2015 11:43:59 -0700 [thread overview]
Message-ID: <xmqqbnhnknio.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <1431624219-25045-1-git-send-email-gjthill@gmail.com> (Jim Hill's message of "Thu, 14 May 2015 10:23:39 -0700")
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.
next prev parent reply other threads:[~2015-05-14 18:44 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-05-14 17:23 [PATCH] sha1_file: pass empty buffer to index empty file Jim Hill
2015-05-14 18:43 ` Junio C Hamano [this message]
2015-05-14 23:17 ` [PATCH v2] " Jim Hill
2015-05-15 18:01 ` Junio C Hamano
2015-05-15 23:31 ` Jim Hill
2015-05-16 18:48 ` Junio C Hamano
2015-05-16 20:06 ` [PATCH v3] " Jim Hill
2015-05-16 23:22 ` Junio C Hamano
2015-05-17 17:37 ` Junio C Hamano
2015-05-17 19:10 ` Junio C Hamano
2015-05-18 0:41 ` [PATCH v4] " Jim Hill
2015-05-19 6:37 ` [PATCH v3] " Jeff King
2015-05-19 18:11 ` Junio C Hamano
2015-05-19 18:17 ` Junio C Hamano
2015-05-19 18:35 ` Junio C Hamano
2015-05-19 19:48 ` Junio C Hamano
2015-05-19 22:14 ` Jeff King
2015-05-20 17:03 ` Junio C Hamano
2015-05-19 19:40 ` Eric Sunshine
2015-05-19 22:09 ` Jeff King
2015-05-20 17:25 ` Junio C Hamano
2015-05-20 17:38 ` Jeff King
2015-05-14 23:26 ` [PATCH] " Jim Hill
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqbnhnknio.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=gjthill@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox