{"thread":{"id":"39337","subject":"[PATCH] sha1_file: pass empty buffer to index empty file","startedAt":"2015-05-14T17:23:39Z","lastAt":"2015-05-20T17:38:36Z","messageCount":23,"participants":["Jim Hill","Junio C Hamano","Jeff King","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"261245","messageId":"1431624219-25045-1-git-send-email-gjthill@gmail.com","threadId":"39337","inReplyTo":null,"subject":"[PATCH] sha1_file: pass empty buffer to index empty file","fromName":"Jim Hill","fromEmail":"gjthill@gmail.com","sentAt":"2015-05-14T17:23:39Z","receivedAt":"2015-05-14T17:23:39Z","isPatch":true,"sender":{"key":"gjthill@gmail.com","avatar":"https://avatars.githubusercontent.com/u/80352?v=4"},"body":"`git add` of an empty file with a filter currently pops complaints from\n`copy_fd` about a bad file descriptor.\n\nThis traces back to these lines in sha1_file.c:index_core:\n\n\tif (!size) {\n\t\tret = index_mem(sha1, NULL, size, type, path, flags);\n\nThe problem here is that content to be added to the index can be\nsupplied from an fd, or from a memory buffer, or from a pathname. This\ncall is supplying a NULL buffer pointer and a zero size.\n\nDownstream logic takes the complete absence of a buffer to mean the\ndata is to be found elsewhere -- for instance, these, from convert.c:\n\n\tif (params->src) {\n\t\twrite_err = (write_in_full(child_process.in, params->src, params->size) < 0);\n\t} else {\n\t\twrite_err = copy_fd(params->fd, child_process.in);\n\t}\n\n~If there's a buffer, write from that, otherwise the data must be coming\nfrom an open fd.~\n\nPerfectly reasonable logic in a routine that's going to write from\neither a buffer or an fd.\n\nSo change `index_core` to supply an empty buffer when indexing an empty\nfile.\n\nThere's a patch out there that instead changes the logic quoted above to\ntake a `-1` fd to mean \"use the buffer\", but it seems to me that the\ndistinction between a missing buffer and an empty one carries intrinsic\nsemantics, where the logic change is adapting the code to handle\nincorrect arguments.\n\nSigned-off-by: Jim Hill <gjthill@gmail.com>\n---\n sha1_file.c                        |  2 +-\n t/t2205-add-empty-filtered-file.sh | 21 +++++++++++++++++++++\n 2 files changed, 22 insertions(+), 1 deletion(-)\n create mode 100755 t/t2205-add-empty-filtered-file.sh\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex f860d67..61e2735 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -3186,7 +3186,7 @@ static int index_core(unsigned char *sha1, int fd, size_t size,\n \tint ret;\n \n \tif (!size) {\n-\t\tret = index_mem(sha1, NULL, size, type, path, flags);\n+\t\tret = index_mem(sha1, \"\", size, type, path, flags);\n \t} else if (size <= SMALL_FILE_SIZE) {\n \t\tchar *buf = xmalloc(size);\n \t\tif (size == read_in_full(fd, buf, size))\ndiff --git a/t/t2205-add-empty-filtered-file.sh b/t/t2205-add-empty-filtered-file.sh\nnew file mode 100755\nindex 0000000..28903c4\n--- /dev/null\n+++ b/t/t2205-add-empty-filtered-file.sh\n@@ -0,0 +1,21 @@\n+#!/bin/sh\n+\n+test_description='adding empty filtered file'\n+\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\techo \"* filter=test\" >>.gitattributes &&\n+\tgit config filter.test.clean cat &&\n+\tgit config filter.test.smudge cat &&\n+\tgit add . &&\n+\tgit commit -m-\n+\n+'\n+\n+test_expect_success \"add of empty filtered file produces no complaints\" '\n+\ttouch emptyfile &&\n+\tgit add emptyfile 2>out &&\n+\ttest -e out -a ! -s out\n+'\n+test_done\n-- \n2.4.1.4.gd9c648d\n"},{"id":"261260","messageId":"xmqqbnhnknio.fsf@gitster.dls.corp.google.com","threadId":"39337","inReplyTo":"1431624219-25045-1-git-send-email-gjthill@gmail.com","subject":"Re: [PATCH] sha1_file: pass empty buffer to index empty file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-14T18:43:59Z","receivedAt":"2015-05-14T18:43:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jim Hill <gjthill@gmail.com> writes:\n\n> `git add` of an empty file with a filter currently pops complaints from\n> `copy_fd` about a bad file descriptor.\n\nOur log message typically begins with the description of a problem\nthat exists; \"currently\" is redundant in that context.  Please lose\nthat word.\n\n> This traces back to these lines in sha1_file.c:index_core:\n>\n> \tif (!size) {\n> \t\tret = index_mem(sha1, NULL, size, type, path, flags);\n>\n> The problem here is that content to be added to the index can be\n> supplied from an fd, or from a memory buffer, or from a pathname. This\n> call is supplying a NULL buffer pointer and a zero size.\n\nGood spotting.\n\nI think the original thinking was that \"we'd feed mem[0..size) to\nthe hash function, so mem being NULL should not matter\", but as you\nanalysed, mem being NULL is used as a signal with a different meaning,\nand your fix is the right one.\n\nI am not enthused to see a new test script wasted just for one piece\nof test.  Don't we have other \"run 'git add' with clean/smudge\nfilters\" test to which you can add this new one to?  If there is\nnone, then a new test script is good, but then it should be a place\nto _add_ missing \"run 'git add' with filters\" test and its name\nshould not be so specific to this \"empty\" special case.\n\n> diff --git a/sha1_file.c b/sha1_file.c\n> index f860d67..61e2735 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -3186,7 +3186,7 @@ static int index_core(unsigned char *sha1, int fd, size_t size,\n>  \tint ret;\n>  \n>  \tif (!size) {\n> -\t\tret = index_mem(sha1, NULL, size, type, path, flags);\n> +\t\tret = index_mem(sha1, \"\", size, type, path, flags);\n>  \t} else if (size <= SMALL_FILE_SIZE) {\n>  \t\tchar *buf = xmalloc(size);\n>  \t\tif (size == read_in_full(fd, buf, size))\n> diff --git a/t/t2205-add-empty-filtered-file.sh b/t/t2205-add-empty-filtered-file.sh\n> new file mode 100755\n> index 0000000..28903c4\n> --- /dev/null\n> +++ b/t/t2205-add-empty-filtered-file.sh\n> @@ -0,0 +1,21 @@\n> +#!/bin/sh\n> +\n> +test_description='adding empty filtered file'\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success setup '\n> +\techo \"* filter=test\" >>.gitattributes &&\n> +\tgit config filter.test.clean cat &&\n> +\tgit config filter.test.smudge cat &&\n> +\tgit add . &&\n> +\tgit commit -m-\n> +\n> +'\n\nPlease do not be cryptic and show a good example, e.g.\n\n\tgit commit -m test\n\nAlso lose the blank line from that test.\n\n> +\n> +test_expect_success \"add of empty filtered file produces no complaints\" '\n> +\ttouch emptyfile &&\n\n\"touch\" is to be used when you care about the timestamp.  When you\ncare more about the _presence_ of the file than what timestamp it\nhas, do not use \"touch\".  Say\n\n\t>emptyfile &&\n\ninstead.  This is especially true in this case, because you not only\ncare about the presence but you care about it being _empty_.\n\n> +\tgit add emptyfile 2>out &&\n> +\ttest -e out -a ! -s out\n\nFuture generation of Git users may want to see \"git add emptyfile\"\nthat succeeds to still say something and that something may be\ndifferent from \"the complaint about a bad file descriptor\".  Don't\nforce the person who makes such a change to update this test.\n\nYou may do\n\n\tgit add emptyfile 2>err &&\n\nand check that 'err' does not contain the copy-fd error (in other\nwords, this test is not in a good position to reject any other\noutput to the standard error stream), if you really wanted to, but I\ndo not think it is worth it.\n\nMy preference is to lose the test on 'out' and end this test like\nthis:\n\n\tgit add emptyfile\n\n> +'\n> +test_done\n\nThanks.\n"},{"id":"261315","messageId":"1431645434-11790-1-git-send-email-gjthill@gmail.com","threadId":"39337","inReplyTo":"xmqqbnhnknio.fsf@gitster.dls.corp.google.com","subject":"[PATCH v2] sha1_file: pass empty buffer to index empty file","fromName":"Jim Hill","fromEmail":"gjthill@gmail.com","sentAt":"2015-05-14T23:17:14Z","receivedAt":"2015-05-14T23:17:14Z","isPatch":true,"sender":{"key":"gjthill@gmail.com","avatar":"https://avatars.githubusercontent.com/u/80352?v=4"},"body":"`git add` of an empty file with a filter pops complaints from\n`copy_fd` about a bad file descriptor.\n\nThis traces back to these lines in sha1_file.c:index_core:\n\n\tif (!size) {\n\t\tret = index_mem(sha1, NULL, size, type, path, flags);\n\nThe problem here is that content to be added to the index can be\nsupplied from an fd, or from a memory buffer, or from a pathname. This\ncall is supplying a NULL buffer pointer and a zero size.\n\nDownstream logic takes the complete absence of a buffer to mean the\ndata is to be found elsewhere -- for instance, these, from convert.c:\n\n\tif (params->src) {\n\t\twrite_err = (write_in_full(child_process.in, params->src, params->size) < 0);\n\t} else {\n\t\twrite_err = copy_fd(params->fd, child_process.in);\n\t}\n\n~If there's a buffer, write from that, otherwise the data must be coming\nfrom an open fd.~\n\nPerfectly reasonable logic in a routine that's going to write from\neither a buffer or an fd.\n\nSo change `index_core` to supply an empty buffer when indexing an empty\nfile.\n\nThere's a patch out there that instead changes the logic quoted above to\ntake a `-1` fd to mean \"use the buffer\", but it seems to me that the\ndistinction between a missing buffer and an empty one carries intrinsic\nsemantics, where the logic change is adapting the code to handle\nincorrect arguments.\n\nSigned-off-by: Jim Hill <gjthill@gmail.com>\n---\n\nJunio C Hamano wrote:\n> Please lose that word.\nCheck.\n\n> Good spotting.\nThanks :-)\n\n> I am not enthused to see a new test script wasted just for one piece\n> of test.  Don't we have other \nYup. Didn't see a place I liked for it in the add and update-index\ntest suites, didn't think to look for a filter test suite...\nCheck.\n\n> Please do not be cryptic and show a good example, e.g.\n> Also lose the blank line from that test.\n>~Say `>emptyfile &&`\nCheckcheckcheck\n\n> check that 'err' does not contain the copy-fd error\n\nImplemented this out of necessity, because the add works and returns\nsuccess despite the complaints to stderr.\n\n\n sha1_file.c           |  2 +-\n t/t0021-conversion.sh | 11 +++++++++++\n 2 files changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex f860d67..61e2735 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -3186,7 +3186,7 @@ static int index_core(unsigned char *sha1, int fd, size_t size,\n \tint ret;\n \n \tif (!size) {\n-\t\tret = index_mem(sha1, NULL, size, type, path, flags);\n+\t\tret = index_mem(sha1, \"\", size, type, path, flags);\n \t} else if (size <= SMALL_FILE_SIZE) {\n \t\tchar *buf = xmalloc(size);\n \t\tif (size == read_in_full(fd, buf, size))\ndiff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\nindex ca7d2a6..5986bb0 100755\n--- a/t/t0021-conversion.sh\n+++ b/t/t0021-conversion.sh\n@@ -216,4 +216,15 @@ test_expect_success EXPENSIVE 'filter large file' '\n \t! test -s err\n '\n \n+test_expect_success \"filtering empty file should not produce complaints\" '\n+\techo \"emptyfile filter=cat\" >>.gitattributes &&\n+\tgit config filter.cat.clean cat &&\n+\tgit config filter.cat.smudge cat &&\n+\tgit add . &&\n+\tgit commit -m \"cat filter for emptyfile\" &&\n+\t> emptyfile &&\n+\tgit add emptyfile 2>err &&\n+\t! grep -Fiqs \"bad file descriptor\" err\n+'\n+\n test_done\n-- \n2.4.1.4.gd9c648d\n"},{"id":"261316","messageId":"20150514232640.GA11838@gadabout.domain.actdsltmp","threadId":"39337","inReplyTo":"xmqqbnhnknio.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] sha1_file: pass empty buffer to index empty file","fromName":"Jim Hill","fromEmail":"gjthill@gmail.com","sentAt":"2015-05-14T23:26:40Z","receivedAt":"2015-05-14T23:26:40Z","isPatch":true,"sender":{"key":"gjthill@gmail.com","avatar":"https://avatars.githubusercontent.com/u/80352?v=4"},"body":"Thanks for the corrections and improvements. The resulting test code\nlooks much cleaner to me.\n\nPlease do suggest any further improvements that occur to you,\n\nThanks,\nJim\n"},{"id":"261356","messageId":"xmqqlhgphg8x.fsf@gitster.dls.corp.google.com","threadId":"39337","inReplyTo":"1431645434-11790-1-git-send-email-gjthill@gmail.com","subject":"Re: [PATCH v2] sha1_file: pass empty buffer to index empty file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-15T18:01:34Z","receivedAt":"2015-05-15T18:01:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jim Hill <gjthill@gmail.com> writes:\n\n>> check that 'err' does not contain the copy-fd error\n>\n> Implemented this out of necessity, because the add works and returns\n> success despite the complaints to stderr.\n\nThat would mean that you found _another_ bug, wouldn't it?  If\ncopy-fd failed to read input to feed the external filter with, it\nmust have returned an error to its caller, and somebody in the\ncallchain is not paying attention to that error and pretending\nas if everything went well.  That's a separate issue, though.\n\nIn any case, I think the following patch may make the test better\n(apply on top of yours).\n\n * A failure to run the filter with the right contents can be caught\n   by examining the outcome.  I tweaked the filter to prepend an\n   extra header line to the contents; if copy-fd failed to drive the\n   filter, we wouldn't see the cleaned output to match that extra\n   header line (and nothing else---as the contents we are feeding is\n   an empty blob).\n\n * There is no need to create an extra commit; an uncommitted\n   .gitattributes from the working tree would work just fine.\n\n * The \"grep\" is gone, with use of -i (questionable why it is\n   needed), -q (generally, we do not squelch error output in\n   individual tests, which is unnecessary and running tests with -v\n   option less useful) and -s (same, withquestionable portability).\n\nThanks.\n\ndiff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\nindex 5986bb0..a72d265 100755\n--- a/t/t0021-conversion.sh\n+++ b/t/t0021-conversion.sh\n@@ -216,15 +216,17 @@ test_expect_success EXPENSIVE 'filter large file' '\n \t! test -s err\n '\n \n-test_expect_success \"filtering empty file should not produce complaints\" '\n-\techo \"emptyfile filter=cat\" >>.gitattributes &&\n-\tgit config filter.cat.clean cat &&\n-\tgit config filter.cat.smudge cat &&\n-\tgit add . &&\n-\tgit commit -m \"cat filter for emptyfile\" &&\n-\t> emptyfile &&\n-\tgit add emptyfile 2>err &&\n-\t! grep -Fiqs \"bad file descriptor\" err\n+test_expect_success \"filtering empty file should work correctly\" '\n+\twrite_script filter-clean.sh <<-EOF &&\n+\techo \"Extra Head\" && cat\n+\tEOF\n+\techo \"emptyfile filter=check\" >>.gitattributes &&\n+\tgit config filter.check.clean \"sh ./filter-clean.sh\" &&\n+\t>emptyfile &&\n+\tgit add emptyfile &&\n+\techo \"Extra Head\" >expect &&\n+\tgit cat-file blob :emptyfile >actual &&\n+\ttest_cmp expect actual\n '\n \n test_done\n"},{"id":"261385","messageId":"20150515233153.GA4157@gadabout.domain.actdsltmp","threadId":"39337","inReplyTo":"xmqqlhgphg8x.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2] sha1_file: pass empty buffer to index empty file","fromName":"Jim Hill","fromEmail":"gjthill@gmail.com","sentAt":"2015-05-15T23:31:53Z","receivedAt":"2015-05-15T23:31:53Z","isPatch":true,"sender":{"key":"gjthill@gmail.com","avatar":"https://avatars.githubusercontent.com/u/80352?v=4"},"body":"On Fri, May 15, 2015 at 11:01:34AM -0700, Junio C Hamano wrote:\n> That would mean that you found _another_ bug, wouldn't it?  If\n> copy-fd failed to read input to feed the external filter with, it\n> must have returned an error to its caller, and somebody in the\n> callchain is not paying attention to that error and pretending\n> as if everything went well.  That's a separate issue, though.\n\nas you say, separate ... I think I stumbled over more than one:\n\nsetup:\n\t~/sandbox/40$ git grl\n\tcore.autocrlf false\n\tcore.whitespace cr-at-eof\n\tcore.repositoryformatversion 0\n\tcore.filemode true\n\tcore.bare false\n\tcore.logallrefupdates true\n\tfilter.cat.smudge cat\n\tfilter.cat.clean echo Kilroy was here && cat\n\tfilter.cat.required true\n\t~/sandbox/40$ git rm --cached -f --ignore-unmatch emptyfile\n\trm 'emptyfile'\n\nwith required filter:\n\t~/sandbox/40$ cat emptyfile\n\t~/sandbox/40$ git add emptyfile\n\t~/sandbox/40$ git show :emptyfile\n\tKilroy was here\n\t~/sandbox/40$ git config --unset filter.cat.required\n\nthen with not-required filter:\n\t~/sandbox/40$ git rm --cached -f --ignore-unmatch emptyfile\n\terror: copy-fd: read returned Bad file descriptor\n\terror: cannot feed the input to external filter echo Kilroy was here && cat\n\terror: external filter echo Kilroy was here && cat failed\n\trm 'emptyfile'\n\t~/sandbox/40$ git show :emptyfile\n\tfatal: Path 'emptyfile' exists on disk, but not in the index.\n\t~/sandbox/40$ git add emptyfile\n\terror: copy-fd: read returned Bad file descriptor\n\terror: cannot feed the input to external filter echo Kilroy was here && cat\n\terror: external filter echo Kilroy was here && cat failed\n\t~/sandbox/40$ git show :emptyfile\n\t~/sandbox/40$ git rm --cached emptyfile\n\trm 'emptyfile'\n\t~/sandbox/40$ git add emptyfile\n\terror: copy-fd: read returned Bad file descriptor\n\terror: cannot feed the input to external filter echo Kilroy was here && cat\n\terror: external filter echo Kilroy was here && cat failed\n\t~/sandbox/40$ git rm --cached -f --ignore-unmatch emptyfile\n\trm 'emptyfile'\n\t~/sandbox/40$ \n\n===\n\nI don't understand rm's choices of when to run the filter, and the\napparently entirely separate code path for required filters is just\nbothersome.\n\n>  * A failure to run the filter with the right contents can be caught\n>    by examining the outcome.\n\nagreed. That's better anyway -- my few git greps didn't find any\nempty-file filter tests anyway.\n\n>  * There is no need to create an extra commit; an uncommitted\n>    .gitattributes from the working tree would work just fine.\n\nDone.\n\n>  * The \"grep\" is gone, with use of -i (questionable why it is\n>    needed), \n\nYah, I was bad-thinking strerror results might be a bit unpredictable, I\nshould have checked for a string under git's control instead.  I'd just\nassumed the 0 return was because non-required filters are allowed to\nfail, got the above transcript while checking the assumption.\n\n=== \n\nSo, so long as we're testing empty-file filters, I figured I'd add real\nempty-file filter tests, I think that covers it.\n\nSo is this better instead?\n\ndiff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\nindex 5986bb0..fc2c644 100755\n--- a/t/t0021-conversion.sh\n+++ b/t/t0021-conversion.sh\n@@ -216,15 +216,33 @@ test_expect_success EXPENSIVE 'filter large file' '\n \t! test -s err\n '\n \n-test_expect_success \"filtering empty file should not produce complaints\" '\n-\techo \"emptyfile filter=cat\" >>.gitattributes &&\n-\tgit config filter.cat.clean cat &&\n-\tgit config filter.cat.smudge cat &&\n-\tgit add . &&\n-\tgit commit -m \"cat filter for emptyfile\" &&\n-\t> emptyfile &&\n-\tgit add emptyfile 2>err &&\n-\t! grep -Fiqs \"bad file descriptor\" err\n+test_expect_success \"filter: clean empty file\" '\n+\theader=---in-repo-header--- &&\n+\tgit config filter.in-repo-header.clean  \"echo $header && cat\" &&\n+\tgit config filter.in-repo-header.smudge \"sed 1d\" &&\n+\n+\techo \"empty-in-worktree    filter=in-repo-header\" >>.gitattributes &&\n+\t> empty-in-worktree &&\n+\n+\techo $header              > expected &&\n+\tgit add empty-in-worktree            &&\n+\tgit show :empty-in-worktree > actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success \"filter: smudge empty file\" '\n+\tgit config filter.empty-in-repo.smudge \"echo smudge added line && cat\" &&\n+\tgit config filter.empty-in-repo.clean   true &&\n+\n+\techo \"empty-in-repo      filter=empty-in-repo\"  >>.gitattributes &&\n+\n+\techo dead data walking > empty-in-repo &&\n+\tgit add empty-in-repo &&\n+\n+\t:\t\t\t> expected &&\n+\tgit show :empty-in-repo\t> actual &&\n+\ttest_cmp expected actual\n '\n \n test_done\n+\n"},{"id":"261392","messageId":"xmqqa8x4fjf5.fsf@gitster.dls.corp.google.com","threadId":"39337","inReplyTo":"20150515233153.GA4157@gadabout.domain.actdsltmp","subject":"Re: [PATCH v2] sha1_file: pass empty buffer to index empty file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-16T18:48:14Z","receivedAt":"2015-05-16T18:48:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jim Hill <gjthill@gmail.com> writes:\n\n> So, so long as we're testing empty-file filters, I figured I'd add real\n> empty-file filter tests, I think that covers it.\n>\n> So is this better instead?\n\nI wouldn't use \"---in-repo-header--\" as that extra string.  Feeding\nanything that begins with '-' to 'echo' gives me portability worries\nfor one thing.  A single word \"Extra\" would suffice.\n\nBe careful and consistent wrt redirection operator and its file; we\ndo not write SP there but some of yours do and some others don't.\n\nDo not attempt to align && with excess SPs; other tests don't.\n\nOther than that, yeah, I think that is an improvement.\n\nThanks.\n\n>\n> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\n> index 5986bb0..fc2c644 100755\n> --- a/t/t0021-conversion.sh\n> +++ b/t/t0021-conversion.sh\n> @@ -216,15 +216,33 @@ test_expect_success EXPENSIVE 'filter large file' '\n>  \t! test -s err\n>  '\n>  \n> -test_expect_success \"filtering empty file should not produce complaints\" '\n> -\techo \"emptyfile filter=cat\" >>.gitattributes &&\n> -\tgit config filter.cat.clean cat &&\n> -\tgit config filter.cat.smudge cat &&\n> -\tgit add . &&\n> -\tgit commit -m \"cat filter for emptyfile\" &&\n> -\t> emptyfile &&\n> -\tgit add emptyfile 2>err &&\n> -\t! grep -Fiqs \"bad file descriptor\" err\n> +test_expect_success \"filter: clean empty file\" '\n> +\theader=---in-repo-header--- &&\n> +\tgit config filter.in-repo-header.clean  \"echo $header && cat\" &&\n> +\tgit config filter.in-repo-header.smudge \"sed 1d\" &&\n> +\n> +\techo \"empty-in-worktree    filter=in-repo-header\" >>.gitattributes &&\n> +\t> empty-in-worktree &&\n> +\n> +\techo $header              > expected &&\n> +\tgit add empty-in-worktree            &&\n> +\tgit show :empty-in-worktree > actual &&\n> +\ttest_cmp expected actual\n> +'\n> +\n> +test_expect_success \"filter: smudge empty file\" '\n> +\tgit config filter.empty-in-repo.smudge \"echo smudge added line && cat\" &&\n> +\tgit config filter.empty-in-repo.clean   true &&\n> +\n> +\techo \"empty-in-repo      filter=empty-in-repo\"  >>.gitattributes &&\n> +\n> +\techo dead data walking > empty-in-repo &&\n> +\tgit add empty-in-repo &&\n> +\n> +\t:\t\t\t> expected &&\n> +\tgit show :empty-in-repo\t> actual &&\n> +\ttest_cmp expected actual\n>  '\n>  \n>  test_done\n> +\n"},{"id":"261396","messageId":"1431806796-28902-1-git-send-email-gjthill@gmail.com","threadId":"39337","inReplyTo":"xmqqa8x4fjf5.fsf@gitster.dls.corp.google.com","subject":"[PATCH v3] sha1_file: pass empty buffer to index empty file","fromName":"Jim Hill","fromEmail":"gjthill@gmail.com","sentAt":"2015-05-16T20:06:36Z","receivedAt":"2015-05-16T20:06:36Z","isPatch":true,"sender":{"key":"gjthill@gmail.com","avatar":"https://avatars.githubusercontent.com/u/80352?v=4"},"body":"`git add` of an empty file with a filter pops complaints from\n`copy_fd` about a bad file descriptor.\n\nThis traces back to these lines in sha1_file.c:index_core:\n\n\tif (!size) {\n\t\tret = index_mem(sha1, NULL, size, type, path, flags);\n\nThe problem here is that content to be added to the index can be\nsupplied from an fd, or from a memory buffer, or from a pathname. This\ncall is supplying a NULL buffer pointer and a zero size.\n\nDownstream logic takes the complete absence of a buffer to mean the\ndata is to be found elsewhere -- for instance, these, from convert.c:\n\n\tif (params->src) {\n\t\twrite_err = (write_in_full(child_process.in, params->src, params->size) < 0);\n\t} else {\n\t\twrite_err = copy_fd(params->fd, child_process.in);\n\t}\n\n~If there's a buffer, write from that, otherwise the data must be coming\nfrom an open fd.~\n\nPerfectly reasonable logic in a routine that's going to write from\neither a buffer or an fd.\n\nSo change `index_core` to supply an empty buffer when indexing an empty\nfile.\n\nThere's a patch out there that instead changes the logic quoted above to\ntake a `-1` fd to mean \"use the buffer\", but it seems to me that the\ndistinction between a missing buffer and an empty one carries intrinsic\nsemantics, where the logic change is adapting the code to handle\nincorrect arguments.\n\nSigned-off-by: Jim Hill <gjthill@gmail.com>\n---\nI promise to pay more attention to test quality in the future, thanks for the\npatience.\n\n sha1_file.c           |  2 +-\n t/t0021-conversion.sh | 26 ++++++++++++++++++++++++++\n 2 files changed, 27 insertions(+), 1 deletion(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex f860d67..61e2735 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -3186,7 +3186,7 @@ static int index_core(unsigned char *sha1, int fd, size_t size,\n \tint ret;\n \n \tif (!size) {\n-\t\tret = index_mem(sha1, NULL, size, type, path, flags);\n+\t\tret = index_mem(sha1, \"\", size, type, path, flags);\n \t} else if (size <= SMALL_FILE_SIZE) {\n \t\tchar *buf = xmalloc(size);\n \t\tif (size == read_in_full(fd, buf, size))\ndiff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\nindex ca7d2a6..bf87e9b 100755\n--- a/t/t0021-conversion.sh\n+++ b/t/t0021-conversion.sh\n@@ -216,4 +216,30 @@ test_expect_success EXPENSIVE 'filter large file' '\n \t! test -s err\n '\n \n+test_expect_success \"filter: clean empty file\" '\n+\tgit config filter.in-repo-header.clean  \"echo cleaned && cat\" &&\n+\tgit config filter.in-repo-header.smudge \"sed 1d\" &&\n+\n+\techo \"empty-in-worktree    filter=in-repo-header\" >>.gitattributes &&\n+\t>empty-in-worktree &&\n+\n+\techo cleaned >expected &&\n+\tgit add empty-in-worktree &&\n+\tgit show :empty-in-worktree >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success \"filter: smudge empty file\" '\n+\tgit config filter.empty-in-repo.clean true &&\n+\tgit config filter.empty-in-repo.smudge \"echo smudged && cat\" &&\n+\n+\techo \"empty-in-repo filter=empty-in-repo\"  >>.gitattributes &&\n+\techo dead data walking >empty-in-repo &&\n+\tgit add empty-in-repo &&\n+\n+\techo smudged >expected &&\n+\tgit checkout-index --prefix=filtered- empty-in-repo &&\n+\ttest_cmp expected filtered-empty-in-repo\n+'\n+\n test_done\n-- \n2.4.1.4.g08cda19\n"},{"id":"261397","messageId":"xmqqsiawds5a.fsf@gitster.dls.corp.google.com","threadId":"39337","inReplyTo":"1431806796-28902-1-git-send-email-gjthill@gmail.com","subject":"Re: [PATCH v3] sha1_file: pass empty buffer to index empty file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-16T23:22:41Z","receivedAt":"2015-05-16T23:22:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jim Hill <gjthill@gmail.com> writes:\n\n> `git add` of an empty file with a filter pops complaints from\n> `copy_fd` about a bad file descriptor.\n>\n> This traces back to these lines in sha1_file.c:index_core:\n>\n> \tif (!size) {\n> \t\tret = index_mem(sha1, NULL, size, type, path, flags);\n>\n> The problem here is that content to be added to the index can be\n> supplied from an fd, or from a memory buffer, or from a pathname. This\n> call is supplying a NULL buffer pointer and a zero size.\n>\n> Downstream logic takes the complete absence of a buffer to mean the\n> data is to be found elsewhere -- for instance, these, from convert.c:\n>\n> \tif (params->src) {\n> \t\twrite_err = (write_in_full(child_process.in, params->src, params->size) < 0);\n> \t} else {\n> \t\twrite_err = copy_fd(params->fd, child_process.in);\n> \t}\n>\n> ~If there's a buffer, write from that, otherwise the data must be coming\n> from an open fd.~\n>\n> Perfectly reasonable logic in a routine that's going to write from\n> either a buffer or an fd.\n>\n> So change `index_core` to supply an empty buffer when indexing an empty\n> file.\n>\n> There's a patch out there that instead changes the logic quoted above to\n> take a `-1` fd to mean \"use the buffer\", but it seems to me that the\n> distinction between a missing buffer and an empty one carries intrinsic\n> semantics, where the logic change is adapting the code to handle\n> incorrect arguments.\n>\n> Signed-off-by: Jim Hill <gjthill@gmail.com>\n> ---\n> I promise to pay more attention to test quality in the future, thanks for the\n> patience.\n\nIt's us who should thank you ;-).  Thanks for spending time to\npolish essentially a one-liner this long.\n\n\n>\n>  sha1_file.c           |  2 +-\n>  t/t0021-conversion.sh | 26 ++++++++++++++++++++++++++\n>  2 files changed, 27 insertions(+), 1 deletion(-)\n>\n> diff --git a/sha1_file.c b/sha1_file.c\n> index f860d67..61e2735 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -3186,7 +3186,7 @@ static int index_core(unsigned char *sha1, int fd, size_t size,\n>  \tint ret;\n>  \n>  \tif (!size) {\n> -\t\tret = index_mem(sha1, NULL, size, type, path, flags);\n> +\t\tret = index_mem(sha1, \"\", size, type, path, flags);\n>  \t} else if (size <= SMALL_FILE_SIZE) {\n>  \t\tchar *buf = xmalloc(size);\n>  \t\tif (size == read_in_full(fd, buf, size))\n> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\n> index ca7d2a6..bf87e9b 100755\n> --- a/t/t0021-conversion.sh\n> +++ b/t/t0021-conversion.sh\n> @@ -216,4 +216,30 @@ test_expect_success EXPENSIVE 'filter large file' '\n>  \t! test -s err\n>  '\n>  \n> +test_expect_success \"filter: clean empty file\" '\n> +\tgit config filter.in-repo-header.clean  \"echo cleaned && cat\" &&\n> +\tgit config filter.in-repo-header.smudge \"sed 1d\" &&\n> +\n> +\techo \"empty-in-worktree    filter=in-repo-header\" >>.gitattributes &&\n> +\t>empty-in-worktree &&\n> +\n> +\techo cleaned >expected &&\n> +\tgit add empty-in-worktree &&\n> +\tgit show :empty-in-worktree >actual &&\n> +\ttest_cmp expected actual\n> +'\n> +\n> +test_expect_success \"filter: smudge empty file\" '\n> +\tgit config filter.empty-in-repo.clean true &&\n> +\tgit config filter.empty-in-repo.smudge \"echo smudged && cat\" &&\n> +\n> +\techo \"empty-in-repo filter=empty-in-repo\"  >>.gitattributes &&\n> +\techo dead data walking >empty-in-repo &&\n> +\tgit add empty-in-repo &&\n> +\n> +\techo smudged >expected &&\n> +\tgit checkout-index --prefix=filtered- empty-in-repo &&\n> +\ttest_cmp expected filtered-empty-in-repo\n> +'\n> +\n>  test_done\n"},{"id":"261412","messageId":"xmqqegmfds1n.fsf@gitster.dls.corp.google.com","threadId":"39337","inReplyTo":"1431806796-28902-1-git-send-email-gjthill@gmail.com","subject":"Re: [PATCH v3] sha1_file: pass empty buffer to index empty file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-17T17:37:08Z","receivedAt":"2015-05-17T17:37:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jim Hill <gjthill@gmail.com> writes:\n\n> +test_expect_success \"filter: smudge empty file\" '\n> +\tgit config filter.empty-in-repo.clean true &&\n\nBut this one is correct but tricky ;-)\n\nIf the contents to be cleaned is small enough (i.e. the one-liner\nfile used in this test) to fit in the pipe buffer and we feed the\npipe before 'true' exits, we won't see any problem.  Otherwise we\nmay get SIGPIPE when we attempt to write to the 'true' (non-)filter,\nbut because we explicitly ignore SIGPIPE, 'true' still is a \"black\nhole\" filter.\n\n\"cat >/dev/null\" may have been a more naive and straight-forward way\nto write this \"black hole\" filter, but what you did is fine.\n\n> +\tgit config filter.empty-in-repo.smudge \"echo smudged && cat\" &&\n> +\n> +\techo \"empty-in-repo filter=empty-in-repo\"  >>.gitattributes &&\n> +\techo dead data walking >empty-in-repo &&\n> +\tgit add empty-in-repo &&\n> +\n> +\techo smudged >expected &&\n> +\tgit checkout-index --prefix=filtered- empty-in-repo &&\n> +\ttest_cmp expected filtered-empty-in-repo\n\nThis is also correct but tricky.\n\n    rm -f empty-in-repo && git checkout empty-in-repo\n\nmay have been more straight-forward, but this exercises the same\ncodepath and perfectly fine.\n\nWill queue and let's merge this to 'next' soonish.\n\nThanks.\n\n> +'\n> +\n>  test_done\n"},{"id":"261415","messageId":"xmqqvbfrc952.fsf@gitster.dls.corp.google.com","threadId":"39337","inReplyTo":"xmqqegmfds1n.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3] sha1_file: pass empty buffer to index empty file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-17T19:10:49Z","receivedAt":"2015-05-17T19:10:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> If the contents to be cleaned is small enough (i.e. the one-liner\n> file used in this test) to fit in the pipe buffer and we feed the\n> pipe before 'true' exits, we won't see any problem.  Otherwise we\n> may get SIGPIPE when we attempt to write to the 'true' (non-)filter,\n> but because we explicitly ignore SIGPIPE, 'true' still is a \"black\n> hole\" filter.\n>\n> \"cat >/dev/null\" may have been a more naive and straight-forward way\n> to write this \"black hole\" filter, but what you did is fine.\n\nI spoke too fast X-<.  \"while sh t0021-*.sh; do :; done\" dies after\na few iterations and with this squashed in it doesn't.\n\n t/t0021-conversion.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\nindex 42e6423..b778faf 100755\n--- a/t/t0021-conversion.sh\n+++ b/t/t0021-conversion.sh\n@@ -218,7 +218,7 @@ test_expect_success \"filter: clean empty file\" '\n '\n \n test_expect_success \"filter: smudge empty file\" '\n-\tgit config filter.empty-in-repo.clean true &&\n+\tgit config filter.empty-in-repo.clean \"cat >/dev/null\" &&\n \tgit config filter.empty-in-repo.smudge \"echo smudged && cat\" &&\n \n \techo \"empty-in-repo filter=empty-in-repo\" >>.gitattributes &&\n-- \n2.4.1-374-g090bfc9\n"},{"id":"261417","messageId":"1431909705-29090-1-git-send-email-gjthill@gmail.com","threadId":"39337","inReplyTo":"xmqqvbfrc952.fsf@gitster.dls.corp.google.com","subject":"[PATCH v4] sha1_file: pass empty buffer to index empty file","fromName":"Jim Hill","fromEmail":"gjthill@gmail.com","sentAt":"2015-05-18T00:41:45Z","receivedAt":"2015-05-18T00:41:45Z","isPatch":true,"sender":{"key":"gjthill@gmail.com","avatar":"https://avatars.githubusercontent.com/u/80352?v=4"},"body":"`git add` of an empty file with a filter pops complaints from\n`copy_fd` about a bad file descriptor.\n\nThis traces back to these lines in sha1_file.c:index_core:\n\n\tif (!size) {\n\t\tret = index_mem(sha1, NULL, size, type, path, flags);\n\nThe problem here is that content to be added to the index can be\nsupplied from an fd, or from a memory buffer, or from a pathname. This\ncall is supplying a NULL buffer pointer and a zero size.\n\nDownstream logic takes the complete absence of a buffer to mean the\ndata is to be found elsewhere -- for instance, these, from convert.c:\n\n\tif (params->src) {\n\t\twrite_err = (write_in_full(child_process.in, params->src, params->size) < 0);\n\t} else {\n\t\twrite_err = copy_fd(params->fd, child_process.in);\n\t}\n\n~If there's a buffer, write from that, otherwise the data must be coming\nfrom an open fd.~\n\nPerfectly reasonable logic in a routine that's going to write from\neither a buffer or an fd.\n\nSo change `index_core` to supply an empty buffer when indexing an empty\nfile.\n\nThere's a patch out there that instead changes the logic quoted above to\ntake a `-1` fd to mean \"use the buffer\", but it seems to me that the\ndistinction between a missing buffer and an empty one carries intrinsic\nsemantics, where the logic change is adapting the code to handle\nincorrect arguments.\n\nSigned-off-by: Jim Hill <gjthill@gmail.com>\n---\nOkay, that's it: I'm officially laughably rusty, 'cause I'm laughing. This\napplies your fix, _thank you_ for the caution. I get it: `true` can have\nalready exited by the time the write hits.\n\n sha1_file.c           |  2 +-\n t/t0021-conversion.sh | 26 ++++++++++++++++++++++++++\n 2 files changed, 27 insertions(+), 1 deletion(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex f860d67..61e2735 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -3186,7 +3186,7 @@ static int index_core(unsigned char *sha1, int fd, size_t size,\n \tint ret;\n \n \tif (!size) {\n-\t\tret = index_mem(sha1, NULL, size, type, path, flags);\n+\t\tret = index_mem(sha1, \"\", size, type, path, flags);\n \t} else if (size <= SMALL_FILE_SIZE) {\n \t\tchar *buf = xmalloc(size);\n \t\tif (size == read_in_full(fd, buf, size))\ndiff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\nindex ca7d2a6..eee4761 100755\n--- a/t/t0021-conversion.sh\n+++ b/t/t0021-conversion.sh\n@@ -216,4 +216,30 @@ test_expect_success EXPENSIVE 'filter large file' '\n \t! test -s err\n '\n \n+test_expect_success \"filter: clean empty file\" '\n+\tgit config filter.in-repo-header.clean  \"echo cleaned && cat\" &&\n+\tgit config filter.in-repo-header.smudge \"sed 1d\" &&\n+\n+\techo \"empty-in-worktree    filter=in-repo-header\" >>.gitattributes &&\n+\t>empty-in-worktree &&\n+\n+\techo cleaned >expected &&\n+\tgit add empty-in-worktree &&\n+\tgit show :empty-in-worktree >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success \"filter: smudge empty file\" '\n+\tgit config filter.empty-in-repo.clean \"cat >/dev/null\" &&\n+\tgit config filter.empty-in-repo.smudge \"echo smudged && cat\" &&\n+\n+\techo \"empty-in-repo filter=empty-in-repo\"  >>.gitattributes &&\n+\techo dead data walking >empty-in-repo &&\n+\tgit add empty-in-repo &&\n+\n+\techo smudged >expected &&\n+\tgit checkout-index --prefix=filtered- empty-in-repo &&\n+\ttest_cmp expected filtered-empty-in-repo\n+'\n+\n test_done\n-- \n2.4.1.4.g08cda19\n"},{"id":"261536","messageId":"20150519063716.GA22771@peff.net","threadId":"39337","inReplyTo":"xmqqvbfrc952.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3] sha1_file: pass empty buffer to index empty file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-19T06:37:16Z","receivedAt":"2015-05-19T06:37:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, May 17, 2015 at 12:10:49PM -0700, Junio C Hamano wrote:\n\n> I spoke too fast X-<.  \"while sh t0021-*.sh; do :; done\" dies after\n> a few iterations and with this squashed in it doesn't.\n> \n>  t/t0021-conversion.sh | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\n> index 42e6423..b778faf 100755\n> --- a/t/t0021-conversion.sh\n> +++ b/t/t0021-conversion.sh\n> @@ -218,7 +218,7 @@ test_expect_success \"filter: clean empty file\" '\n>  '\n>  \n>  test_expect_success \"filter: smudge empty file\" '\n> -\tgit config filter.empty-in-repo.clean true &&\n> +\tgit config filter.empty-in-repo.clean \"cat >/dev/null\" &&\n\nHmm, I thought we turned off SIGPIPE when writing to filters these days.\nLooks like we still complain if we get EPIPE, though. I feel like it\nshould be the filter's business whether it wants to consume all of the\ninput or not[1], and we should only be checking its exit status.\n\n-Peff\n\n[1] As a practical example, consider a file format that has a lot of\n    cruft at the end. The clean filter would want to read only to the\n    start of the cruft, and then stop for reasons of efficiency.\n"},{"id":"261576","messageId":"xmqqk2w48mjp.fsf@gitster.dls.corp.google.com","threadId":"39337","inReplyTo":"20150519063716.GA22771@peff.net","subject":"Re: [PATCH v3] sha1_file: pass empty buffer to index empty file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-19T18:11:38Z","receivedAt":"2015-05-19T18:11:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Sun, May 17, 2015 at 12:10:49PM -0700, Junio C Hamano wrote:\n>\n>> I spoke too fast X-<.  \"while sh t0021-*.sh; do :; done\" dies after\n>> a few iterations and with this squashed in it doesn't.\n>> \n>>  t/t0021-conversion.sh | 2 +-\n>>  1 file changed, 1 insertion(+), 1 deletion(-)\n>> \n>> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\n>> index 42e6423..b778faf 100755\n>> --- a/t/t0021-conversion.sh\n>> +++ b/t/t0021-conversion.sh\n>> @@ -218,7 +218,7 @@ test_expect_success \"filter: clean empty file\" '\n>>  '\n>>  \n>>  test_expect_success \"filter: smudge empty file\" '\n>> -\tgit config filter.empty-in-repo.clean true &&\n>> +\tgit config filter.empty-in-repo.clean \"cat >/dev/null\" &&\n>\n> Hmm, I thought we turned off SIGPIPE when writing to filters these days.\n> Looks like we still complain if we get EPIPE, though. I feel like it\n> should be the filter's business whether it wants to consume all of the\n> input or not[1], and we should only be checking its exit status.\n>\n> -Peff\n>\n> [1] As a practical example, consider a file format that has a lot of\n>     cruft at the end. The clean filter would want to read only to the\n>     start of the cruft, and then stop for reasons of efficiency.\n\nYes.  Let's do these two.  The preparatory patch is larger than the\nreal change.\n\n-- >8 --\nFrom: Junio C Hamano <gitster@pobox.com>\nDate: Tue, 19 May 2015 10:55:16 -0700\nSubject: [PATCH] copy.c: make copy_fd() report its status silently\n\nWhen copy_fd() function encounters errors, it emits error messages\nitself, which makes it impossible for callers to take responsibility\nfor reporting errors, especially when they want to ignore certaion\nerrors.\n\nMove the error reporting to its callers in preparation.\n\n - copy_file() and copy_file_with_time() by indirection get their\n   own calls to error().\n\n - hold_lock_file_for_append(), when told to die on error, used to\n   exit(128) relying on the error message from copy_fd(), but now it\n   does its own die() instead.  Note that the callers that do not\n   pass LOCK_DIE_ON_ERROR need to be adjusted for this change, but\n   fortunately there is none ;-)\n\n - filter_buffer_or_fd() has its own error() already, in addition to\n   the message from copy_fd(), so this will change the output but\n   arguably in a better way.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n cache.h    |  4 ++++\n copy.c     | 17 +++++++++++------\n lockfile.c |  2 +-\n 3 files changed, 16 insertions(+), 7 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 22b7b81..2981eec 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1482,9 +1482,13 @@ extern const char *git_mailmap_blob;\n extern void maybe_flush_or_die(FILE *, const char *);\n __attribute__((format (printf, 2, 3)))\n extern void fprintf_or_die(FILE *, const char *fmt, ...);\n+\n+#define COPY_READ_ERROR (-2)\n+#define COPY_WRITE_ERROR (-3)\n extern int copy_fd(int ifd, int ofd);\n extern int copy_file(const char *dst, const char *src, int mode);\n extern int copy_file_with_time(const char *dst, const char *src, int mode);\n+\n extern void write_or_die(int fd, const void *buf, size_t count);\n extern int write_or_whine(int fd, const void *buf, size_t count, const char *msg);\n extern int write_or_whine_pipe(int fd, const void *buf, size_t count, const char *msg);\ndiff --git a/copy.c b/copy.c\nindex f2970ec..574fa1f 100644\n--- a/copy.c\n+++ b/copy.c\n@@ -7,13 +7,10 @@ int copy_fd(int ifd, int ofd)\n \t\tssize_t len = xread(ifd, buffer, sizeof(buffer));\n \t\tif (!len)\n \t\t\tbreak;\n-\t\tif (len < 0) {\n-\t\t\treturn error(\"copy-fd: read returned %s\",\n-\t\t\t\t     strerror(errno));\n-\t\t}\n+\t\tif (len < 0)\n+\t\t\treturn COPY_READ_ERROR;\n \t\tif (write_in_full(ofd, buffer, len) < 0)\n-\t\t\treturn error(\"copy-fd: write returned %s\",\n-\t\t\t\t     strerror(errno));\n+\t\t\treturn COPY_WRITE_ERROR;\n \t}\n \treturn 0;\n }\n@@ -43,6 +40,14 @@ int copy_file(const char *dst, const char *src, int mode)\n \t\treturn fdo;\n \t}\n \tstatus = copy_fd(fdi, fdo);\n+\tswitch (status) {\n+\tcase COPY_READ_ERROR:\n+\t\terror(\"copy-fd: read returned %s\", strerror(errno));\n+\t\tbreak;\n+\tcase COPY_WRITE_ERROR:\n+\t\terror(\"copy-fd: write returned %s\", strerror(errno));\n+\t\tbreak;\n+\t}\n \tclose(fdi);\n \tif (close(fdo) != 0)\n \t\treturn error(\"%s: close error: %s\", dst, strerror(errno));\ndiff --git a/lockfile.c b/lockfile.c\nindex 4f16ee7..beba0ed 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -206,7 +206,7 @@ int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags)\n \t\tint save_errno = errno;\n \n \t\tif (flags & LOCK_DIE_ON_ERROR)\n-\t\t\texit(128);\n+\t\t\tdie(\"failed to prepare '%s' for appending\", path);\n \t\tclose(orig_fd);\n \t\trollback_lock_file(lk);\n \t\terrno = save_errno;\n-- \n2.4.1-413-ga38dc94\n"},{"id":"261577","messageId":"xmqqd21w8mal.fsf@gitster.dls.corp.google.com","threadId":"39337","inReplyTo":"xmqqk2w48mjp.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3] sha1_file: pass empty buffer to index empty file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-19T18:17:06Z","receivedAt":"2015-05-19T18:17:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>> Hmm, I thought we turned off SIGPIPE when writing to filters these days.\n>> Looks like we still complain if we get EPIPE, though. I feel like it\n>> should be the filter's business whether it wants to consume all of the\n>> input or not[1], and we should only be checking its exit status.\n>>\n>> -Peff\n>>\n>> [1] As a practical example, consider a file format that has a lot of\n>>     cruft at the end. The clean filter would want to read only to the\n>>     start of the cruft, and then stop for reasons of efficiency.\n>\n> Yes.  Let's do these two.  The preparatory patch is larger than the\n> real change.\n\nAnd this is the second one.\n\nWhile preparing these, I noticed a handful of system calls whose\nreturn values are not checked in the codepaths involved.  We should\nclean them up, but I left them out of these two patches, as they are\nseparate issues.\n\n-- >8 --\nSubject: [PATCH 2/2] filter_buffer_or_fd(): ignore EPIPE\n\nWe are explicitly ignoring SIGPIPE, as we fully expect that the\nfilter program may not read our output fully.  Ignore EPIPE that\nmay come from writing to it as well.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n convert.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/convert.c b/convert.c\nindex 9a5612e..0f20979 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -359,6 +359,8 @@ static int filter_buffer_or_fd(int in, int out, void *data)\n \t\twrite_err = (write_in_full(child_process.in, params->src, params->size) < 0);\n \t} else {\n \t\twrite_err = copy_fd(params->fd, child_process.in);\n+\t\tif (write_error == COPY_WRITE_ERROR && errno == EPIPE)\n+\t\t\twrite_error = 0; /* we are ignoring it, right? */\n \t}\n \n \tif (close(child_process.in))\n-- \n2.4.1-413-ga38dc94\n"},{"id":"261579","messageId":"xmqq1tic8lgj.fsf@gitster.dls.corp.google.com","threadId":"39337","inReplyTo":"xmqqd21w8mal.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3] sha1_file: pass empty buffer to index empty file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-19T18:35:08Z","receivedAt":"2015-05-19T18:35:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Subject: [PATCH 2/2] filter_buffer_or_fd(): ignore EPIPE\n>\n> We are explicitly ignoring SIGPIPE, as we fully expect that the\n> filter program may not read our output fully.  Ignore EPIPE that\n> may come from writing to it as well.\n\nYuck; please discard the previous one.  write_in_full() side is also\nwriting into that process, so we should do the same.\n\n-- >8 --\nSubject: [PATCH v2 2/2] filter_buffer_or_fd(): ignore EPIPE\n\nWe are explicitly ignoring SIGPIPE, as we fully expect that the\nfilter program may not read our output fully.  Ignore EPIPE that\nmay come from writing to it as well.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n convert.c | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/convert.c b/convert.c\nindex 9a5612e..f3bd3e9 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -356,9 +356,14 @@ static int filter_buffer_or_fd(int in, int out, void *data)\n \tsigchain_push(SIGPIPE, SIG_IGN);\n \n \tif (params->src) {\n-\t\twrite_err = (write_in_full(child_process.in, params->src, params->size) < 0);\n+\t\twrite_err = (write_in_full(child_process.in,\n+\t\t\t\t\t   params->src, params->size) < 0);\n+\t\tif (errno == EPIPE)\n+\t\t\twrite_err = 0;\n \t} else {\n \t\twrite_err = copy_fd(params->fd, child_process.in);\n+\t\tif (write_err == COPY_WRITE_ERROR && errno == EPIPE)\n+\t\t\twrite_err = 0;\n \t}\n \n \tif (close(child_process.in))\n-- \n2.4.1-413-ga38dc94\n"},{"id":"261587","messageId":"CAPig+cQUJPWiz7OzQTwaDp3aeq5Gu6rWezYNfq8nuK_h+n48DA@mail.gmail.com","threadId":"39337","inReplyTo":"xmqqk2w48mjp.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3] sha1_file: pass empty buffer to index empty file","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-05-19T19:40:38Z","receivedAt":"2015-05-19T19:40:38Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, May 19, 2015 at 2:11 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Subject: [PATCH] copy.c: make copy_fd() report its status silently\n>\n> When copy_fd() function encounters errors, it emits error messages\n> itself, which makes it impossible for callers to take responsibility\n> for reporting errors, especially when they want to ignore certaion\n\ns/certaion/certain/\n\n> errors.\n>\n> Move the error reporting to its callers in preparation.\n>\n>  - copy_file() and copy_file_with_time() by indirection get their\n>    own calls to error().\n>\n>  - hold_lock_file_for_append(), when told to die on error, used to\n>    exit(128) relying on the error message from copy_fd(), but now it\n>    does its own die() instead.  Note that the callers that do not\n>    pass LOCK_DIE_ON_ERROR need to be adjusted for this change, but\n>    fortunately there is none ;-)\n>\n>  - filter_buffer_or_fd() has its own error() already, in addition to\n>    the message from copy_fd(), so this will change the output but\n>    arguably in a better way.\n>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n"},{"id":"261589","messageId":"xmqqk2w473i2.fsf@gitster.dls.corp.google.com","threadId":"39337","inReplyTo":"xmqq1tic8lgj.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3] sha1_file: pass empty buffer to index empty file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-19T19:48:21Z","receivedAt":"2015-05-19T19:48:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Yuck; please discard the previous one.  write_in_full() side is also\n> writing into that process, so we should do the same.\n\nOK, without these two, and with the \"true\" filter that does not read\nanything reinstated in the test script, t0021 used to die\n\n    i=0; while sh t0021-conversion.sh; do i=$(( $i + 1 )); done\n\nafter 150 iteration or so for me.  With these two, it seems to go on\nwithout breaking (I bored after 1000 iterations), so I'd declare it\ngood enough ;-)\n"},{"id":"261609","messageId":"20150519220918.GA779@peff.net","threadId":"39337","inReplyTo":"xmqqk2w48mjp.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3] sha1_file: pass empty buffer to index empty file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-19T22:09:18Z","receivedAt":"2015-05-19T22:09:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 19, 2015 at 11:11:38AM -0700, Junio C Hamano wrote:\n\n> Subject: [PATCH] copy.c: make copy_fd() report its status silently\n> \n> When copy_fd() function encounters errors, it emits error messages\n> itself, which makes it impossible for callers to take responsibility\n> for reporting errors, especially when they want to ignore certaion\n> errors.\n> \n> Move the error reporting to its callers in preparation.\n> [...]\n\nLooks good to me. And thank you for being thorough in analyzing the\nimpact on all the callers.\n\n>  - hold_lock_file_for_append(), when told to die on error, used to\n>    exit(128) relying on the error message from copy_fd(), but now it\n>    does its own die() instead.  Note that the callers that do not\n>    pass LOCK_DIE_ON_ERROR need to be adjusted for this change, but\n>    fortunately there is none ;-)\n\nNot related to your patch, but I've often wondered if we can just get\nrid of hold_lock_file_for_append. There's exactly one caller, and I\nthink it is doing the wrong thing. It is add_to_alternates_file(), but\nshouldn't it probably read the existing lines to make sure it is not\nadding a duplicate? IOW, I think hold_lock_file_for_append is a\nfundamentally bad interface, because almost nobody truly wants to _just_\nappend.\n\nAnd I have not investigated it carefully, but I suspect that we do not\neven have to be that careful. The only time we write the file is during\nclone, and I suspect we could just use a string_list, and then write it\nout. We probably don't even need to lock (it's not like we take a lock\nbefore creating the \"objects\" directory in the first place).\n\nAnyway, end mini-rant. It is probably not hurting anyone and does not\nneed to be dealt with anytime soon.\n\n-Peff\n"},{"id":"261614","messageId":"20150519221450.GB779@peff.net","threadId":"39337","inReplyTo":"xmqqk2w473i2.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3] sha1_file: pass empty buffer to index empty file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-19T22:14:50Z","receivedAt":"2015-05-19T22:14:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 19, 2015 at 12:48:21PM -0700, Junio C Hamano wrote:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > Yuck; please discard the previous one.  write_in_full() side is also\n> > writing into that process, so we should do the same.\n> \n> OK, without these two, and with the \"true\" filter that does not read\n> anything reinstated in the test script, t0021 used to die\n> \n>     i=0; while sh t0021-conversion.sh; do i=$(( $i + 1 )); done\n> \n> after 150 iteration or so for me.  With these two, it seems to go on\n> without breaking (I bored after 1000 iterations), so I'd declare it\n> good enough ;-)\n\nYour revised patch 2 looks good to me. I think you could test it more\nreliably by simply adding a larger file, like:\n\n  test-genrandom foo $((128 * 1024 + 1)) >big &&\n  echo 'big filter=epipe' >.gitattributes &&\n  git config filter.epipe.clean true &&\n  git add big\n\nThe worst case if you get the size of the pipe buffer too small is that\nthe test will erroneously pass, but that is OK. As long as one person\nhas a reasonable-sized buffer, they will complain to the list\neventually. :)\n\n-Peff\n"},{"id":"261666","messageId":"xmqqvbfnnpu6.fsf@gitster.dls.corp.google.com","threadId":"39337","inReplyTo":"20150519221450.GB779@peff.net","subject":"Re: [PATCH v3] sha1_file: pass empty buffer to index empty file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-20T17:03:45Z","receivedAt":"2015-05-20T17:03:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Your revised patch 2 looks good to me. I think you could test it more\n> reliably by simply adding a larger file, like:\n>\n>   test-genrandom foo $((128 * 1024 + 1)) >big &&\n>   echo 'big filter=epipe' >.gitattributes &&\n>   git config filter.epipe.clean true &&\n>   git add big\n>\n> The worst case if you get the size of the pipe buffer too small is that\n> the test will erroneously pass, but that is OK. As long as one person\n> has a reasonable-sized buffer, they will complain to the list\n> eventually. :)\n\nYeah, I like it.  It was lazy of me not to add a new test.\n\nThanks.\n"},{"id":"261674","messageId":"xmqqr3qbnotm.fsf@gitster.dls.corp.google.com","threadId":"39337","inReplyTo":"20150519220918.GA779@peff.net","subject":"Re: [PATCH v3] sha1_file: pass empty buffer to index empty file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-20T17:25:41Z","receivedAt":"2015-05-20T17:25:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Not related to your patch, but I've often wondered if we can just get\n> rid of hold_lock_file_for_append. There's exactly one caller, and I\n> think it is doing the wrong thing. It is add_to_alternates_file(), but\n> shouldn't it probably read the existing lines to make sure it is not\n> adding a duplicate? IOW, I think hold_lock_file_for_append is a\n> fundamentally bad interface, because almost nobody truly wants to _just_\n> append.\n\nYeah, I tend to agree.  Perhaps I should throw it into the list of\nlow hanging fruits (aka lmgtfy:\"git blame leftover bits\") and see if\nanybody bites ;-)\n"},{"id":"261675","messageId":"20150520173836.GA14561@peff.net","threadId":"39337","inReplyTo":"xmqqr3qbnotm.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3] sha1_file: pass empty buffer to index empty file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-20T17:38:36Z","receivedAt":"2015-05-20T17:38:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 20, 2015 at 10:25:41AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > Not related to your patch, but I've often wondered if we can just get\n> > rid of hold_lock_file_for_append. There's exactly one caller, and I\n> > think it is doing the wrong thing. It is add_to_alternates_file(), but\n> > shouldn't it probably read the existing lines to make sure it is not\n> > adding a duplicate? IOW, I think hold_lock_file_for_append is a\n> > fundamentally bad interface, because almost nobody truly wants to _just_\n> > append.\n> \n> Yeah, I tend to agree.  Perhaps I should throw it into the list of\n> low hanging fruits (aka lmgtfy:\"git blame leftover bits\") and see if\n> anybody bites ;-)\n\nGood thinking. I think it is the right urgency and difficulty for that\nlist.\n\n-Peff\n"}]}