Re: [PATCH v2 07/11] streaming_write_entry(): use streaming API in write_entry()
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:51:17
Junio C Hamano [off-list ref] writes:
When the output to a path does not have to be converted, we can read from the object database from the streaming API and write to the file in the working tree, without having to hold everything in the memory.
There was an embarrassing bug hiding here. The way I found it was doubly embarrassing.
quoted hunk
diff --git a/entry.c b/entry.c index cc6502a..7733a6b 100644 --- a/entry.c +++ b/entry.c@@ -114,6 +115,50 @@ static int fstat_output(int fd, const struct checkout *state, struct stat *st) return 0; } +static int streaming_write_entry(struct cache_entry *ce, char *path, + const struct checkout *state, int to_tempfile, + int *fstat_done, struct stat *statbuf) +{ + struct git_istream *st; + enum object_type type; + unsigned long sz; + int result = -1;
We see result is initialized to -1 here...
+ int fd = -1; + + st = open_istream(ce->sha1, &type, &sz); + ... +close_and_exit: + close_istream(st);
...but nobody touches it before coming here via various codepaths. If I haven't opened the output, fd is -1, but if I have, then the condition of the next if statement is met, and I close the output.
+ if (0 <= fd) + result |= close(fd);
Oops. This is the embarrassment. This has to be an assignment, not OR-ing it in. I am not clearing the result in a successful case.
+ if (result && 0 <= fd) + unlink(path); + return result;
Hence, even though the codepath fully wrote out, we run unlink(path), and return a failure.
quoted hunk
@@ -124,6 +169,12 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout size_t wrote, newsize = 0; struct stat st; + if ((ce_mode_s_ifmt == S_IFREG) && + can_bypass_conversion(path) && + !streaming_write_entry(ce, path, state, to_tempfile, + &fstat_done, &st)) + goto finish;
Then the caller just thinks that the contents could not be streamed (e.g. the data is in pack deltified) and takes the fall-back codepath to fix everything up, hiding the above bug.
switch (ce_mode_s_ifmt) {
case S_IFREG:
case S_IFLNK:
... fall-back code follows ...The other embarrassment is the way how I found the bug. I was trying to rewrite this "if regular file and can bypass conversion, try streaming" logic, and botched the result to jump to "finish" when streaming write returned failure. Will re-queue with the obvious fix squashed in.