patchsha1_file: pass empty buffer to index empty file

`git add` of an empty file with a filter currently 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>
---
 sha1_file.c                        |  2 +-
 t/t2205-add-empty-filtered-file.sh | 21 +++++++++++++++++++++
 2 files changed, 22 insertions(+), 1 deletion(-)
 create mode 100755 t/t2205-add-empty-filtered-file.sh

Jim Hill <gjthill@gmail.com> writes:

Our log message typically begins with the description of a problem that exists; "currently" is redundant in that context. Please lose that word.

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.

Please do not be cryptic and show a good example, e.g.

	git commit -m test

Also lose the blank line from that test.

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

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

Thanks.

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

Junio C Hamano wrote:

Check.

Thanks :-)

Yup. Didn't see a place I liked for it in the add and update-index test suites, didn't think to look for a filter test suite... Check.

Checkcheckcheck

Implemented this out of necessity, because the add works and returns success despite the complaints to stderr.

 sha1_file.c           |  2 +-
 t/t0021-conversion.sh | 11 +++++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

Jim Hill <gjthill@gmail.com> writes:

That would mean that you found _another_ bug, wouldn't it? If copy-fd failed to read input to feed the external filter with, it must have returned an error to its caller, and somebody in the callchain is not paying attention to that error and pretending as if everything went well. That's a separate issue, though.

In any case, I think the following patch may make the test better (apply on top of yours).

 * A failure to run the filter with the right contents can be caught
   by examining the outcome.  I tweaked the filter to prepend an
   extra header line to the contents; if copy-fd failed to drive the
   filter, we wouldn't see the cleaned output to match that extra
   header line (and nothing else---as the contents we are feeding is
   an empty blob).

 * There is no need to create an extra commit; an uncommitted
   .gitattributes from the working tree would work just fine.

 * The "grep" is gone, with use of -i (questionable why it is
   needed), -q (generally, we do not squelch error output in
   individual tests, which is unnecessary and running tests with -v
   option less useful) and -s (same, withquestionable portability).

Thanks.

On Fri, May 15, 2015 at 11:01:34AM -0700, Junio C Hamano wrote:

as you say, separate ... I think I stumbled over more than one:

setup:
	~/sandbox/40$ git grl
	core.autocrlf false
	core.whitespace cr-at-eof
	core.repositoryformatversion 0
	core.filemode true
	core.bare false
	core.logallrefupdates true
	filter.cat.smudge cat
	filter.cat.clean echo Kilroy was here && cat
	filter.cat.required true
	~/sandbox/40$ git rm --cached -f --ignore-unmatch emptyfile
	rm 'emptyfile'

with required filter:
	~/sandbox/40$ cat emptyfile
	~/sandbox/40$ git add emptyfile
	~/sandbox/40$ git show :emptyfile
	Kilroy was here
	~/sandbox/40$ git config --unset filter.cat.required

then with not-required filter:
	~/sandbox/40$ git rm --cached -f --ignore-unmatch emptyfile
	error: copy-fd: read returned Bad file descriptor
	error: cannot feed the input to external filter echo Kilroy was here && cat
	error: external filter echo Kilroy was here && cat failed
	rm 'emptyfile'
	~/sandbox/40$ git show :emptyfile
	fatal: Path 'emptyfile' exists on disk, but not in the index.
	~/sandbox/40$ git add emptyfile
	error: copy-fd: read returned Bad file descriptor
	error: cannot feed the input to external filter echo Kilroy was here && cat
	error: external filter echo Kilroy was here && cat failed
	~/sandbox/40$ git show :emptyfile
	~/sandbox/40$ git rm --cached emptyfile
	rm 'emptyfile'
	~/sandbox/40$ git add emptyfile
	error: copy-fd: read returned Bad file descriptor
	error: cannot feed the input to external filter echo Kilroy was here && cat
	error: external filter echo Kilroy was here && cat failed
	~/sandbox/40$ git rm --cached -f --ignore-unmatch emptyfile
	rm 'emptyfile'
	~/sandbox/40$ 

===

I don't understand rm's choices of when to run the filter, and the apparently entirely separate code path for required filters is just bothersome.

agreed. That's better anyway -- my few git greps didn't find any empty-file filter tests anyway.

Done.

Yah, I was bad-thinking strerror results might be a bit unpredictable, I should have checked for a string under git's control instead. I'd just assumed the 0 return was because non-required filters are allowed to fail, got the above transcript while checking the assumption.

=== 

So, so long as we're testing empty-file filters, I figured I'd add real empty-file filter tests, I think that covers it.

So is this better instead?

Jim Hill <gjthill@gmail.com> writes:

I wouldn't use "---in-repo-header--" as that extra string. Feeding anything that begins with '-' to 'echo' gives me portability worries for one thing. A single word "Extra" would suffice.

Be careful and consistent wrt redirection operator and its file; we do not write SP there but some of yours do and some others don't.

Do not attempt to align && with excess SPs; other tests don't.

Other than that, yeah, I think that is an improvement.

Thanks.

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

 sha1_file.c           |  2 +-
 t/t0021-conversion.sh | 26 ++++++++++++++++++++++++++
 2 files changed, 27 insertions(+), 1 deletion(-)

Jim Hill <gjthill@gmail.com> writes:

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

Jim Hill <gjthill@gmail.com> writes:

But this one is correct but tricky ;-)

If the contents to be cleaned is small enough (i.e. the one-liner file used in this test) to fit in the pipe buffer and we feed the pipe before 'true' exits, we won't see any problem. Otherwise we may get SIGPIPE when we attempt to write to the 'true' (non-)filter, but because we explicitly ignore SIGPIPE, 'true' still is a "black hole" filter.

"cat >/dev/null" may have been a more naive and straight-forward way to write this "black hole" filter, but what you did is fine.

This is also correct but tricky.

    rm -f empty-in-repo && git checkout empty-in-repo

may have been more straight-forward, but this exercises the same codepath and perfectly fine.

Will queue and let's merge this to 'next' soonish.

Thanks.

Junio C Hamano <gitster@pobox.com> writes:

I spoke too fast X-<. "while sh t0021-*.sh; do :; done" dies after a few iterations and with this squashed in it doesn't.

 t/t0021-conversion.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

`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>
---
Okay, that's it: I'm officially laughably rusty, 'cause I'm laughing. This
applies your fix, _thank you_ for the caution. I get it: `true` can have
already exited by the time the write hits.

 sha1_file.c           |  2 +-
 t/t0021-conversion.sh | 26 ++++++++++++++++++++++++++
 2 files changed, 27 insertions(+), 1 deletion(-)

On Sun, May 17, 2015 at 12:10:49PM -0700, Junio C Hamano wrote:

Hmm, I thought we turned off SIGPIPE when writing to filters these days. Looks like we still complain if we get EPIPE, though. I feel like it should be the filter's business whether it wants to consume all of the input or not[1], and we should only be checking its exit status.

-Peff

[1] As a practical example, consider a file format that has a lot of
    cruft at the end. The clean filter would want to read only to the
    start of the cruft, and then stop for reasons of efficiency.

Jeff King <peff@peff.net> writes:

Yes. Let's do these two. The preparatory patch is larger than the real change.

-- >8 --
From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 19 May 2015 10:55:16 -0700
Subject: [PATCH] copy.c: make copy_fd() report its status silently

When copy_fd() function encounters errors, it emits error messages itself, which makes it impossible for callers to take responsibility for reporting errors, especially when they want to ignore certaion errors.

Move the error reporting to its callers in preparation.

 - copy_file() and copy_file_with_time() by indirection get their
   own calls to error().

 - hold_lock_file_for_append(), when told to die on error, used to
   exit(128) relying on the error message from copy_fd(), but now it
   does its own die() instead.  Note that the callers that do not
   pass LOCK_DIE_ON_ERROR need to be adjusted for this change, but
   fortunately there is none ;-)

 - filter_buffer_or_fd() has its own error() already, in addition to
   the message from copy_fd(), so this will change the output but
   arguably in a better way.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 cache.h    |  4 ++++
 copy.c     | 17 +++++++++++------
 lockfile.c |  2 +-
 3 files changed, 16 insertions(+), 7 deletions(-)

Junio C Hamano <gitster@pobox.com> writes:

And this is the second one.

While preparing these, I noticed a handful of system calls whose return values are not checked in the codepaths involved. We should clean them up, but I left them out of these two patches, as they are separate issues.

-- >8 --
Subject: [PATCH 2/2] filter_buffer_or_fd(): ignore EPIPE

We are explicitly ignoring SIGPIPE, as we fully expect that the filter program may not read our output fully. Ignore EPIPE that may come from writing to it as well.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 convert.c | 2 ++
 1 file changed, 2 insertions(+)

Junio C Hamano <gitster@pobox.com> writes:

Yuck; please discard the previous one. write_in_full() side is also writing into that process, so we should do the same.

-- >8 --
Subject: [PATCH v2 2/2] filter_buffer_or_fd(): ignore EPIPE

We are explicitly ignoring SIGPIPE, as we fully expect that the filter program may not read our output fully. Ignore EPIPE that may come from writing to it as well.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 convert.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Junio C Hamano <gitster@pobox.com> writes:

OK, without these two, and with the "true" filter that does not read anything reinstated in the test script, t0021 used to die

    i=0; while sh t0021-conversion.sh; do i=$(( $i + 1 )); done

after 150 iteration or so for me. With these two, it seems to go on without breaking (I bored after 1000 iterations), so I'd declare it good enough ;-)

On Tue, May 19, 2015 at 12:48:21PM -0700, Junio C Hamano wrote:

Your revised patch 2 looks good to me. I think you could test it more reliably by simply adding a larger file, like:

  test-genrandom foo $((128 * 1024 + 1)) >big &&
  echo 'big filter=epipe' >.gitattributes &&
  git config filter.epipe.clean true &&
  git add big

The worst case if you get the size of the pipe buffer too small is that the test will erroneously pass, but that is OK. As long as one person has a reasonable-sized buffer, they will complain to the list eventually. :)

-Peff

Jeff King <peff@peff.net> writes:

Yeah, I like it.  It was lazy of me not to add a new test.

Thanks.

On Tue, May 19, 2015 at 2:11 PM, Junio C Hamano <gitster@pobox.com> wrote:

s/certaion/certain/

On Tue, May 19, 2015 at 11:11:38AM -0700, Junio C Hamano wrote:

Looks good to me. And thank you for being thorough in analyzing the impact on all the callers.

Not related to your patch, but I've often wondered if we can just get rid of hold_lock_file_for_append. There's exactly one caller, and I think it is doing the wrong thing. It is add_to_alternates_file(), but shouldn't it probably read the existing lines to make sure it is not adding a duplicate? IOW, I think hold_lock_file_for_append is a fundamentally bad interface, because almost nobody truly wants to _just_ append.

And I have not investigated it carefully, but I suspect that we do not even have to be that careful. The only time we write the file is during clone, and I suspect we could just use a string_list, and then write it out. We probably don't even need to lock (it's not like we take a lock before creating the "objects" directory in the first place).

Anyway, end mini-rant. It is probably not hurting anyone and does not need to be dealt with anytime soon.

-Peff

Jeff King <peff@peff.net> writes:

Yeah, I tend to agree. Perhaps I should throw it into the list of low hanging fruits (aka lmgtfy:"git blame leftover bits") and see if anybody bites ;-)

On Wed, May 20, 2015 at 10:25:41AM -0700, Junio C Hamano wrote:

Good thinking. I think it is the right urgency and difficulty for that list.

-Peff

Thanks for the corrections and improvements. The resulting test code looks much cleaner to me.

Please do suggest any further improvements that occur to you,

Thanks, Jim