Re: [PATCH v2 07/11] streaming_write_entry(): use streaming API in write_entry()
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 20, 2011, 22:52 UTC
- Message-ID
- <7vvcx50yt8.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <1305840826-7783-8-git-send-email-gitster@pobox.com>
Junio C Hamano <gitster@pobox.com> 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.
Show 16 quoted lines
> 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...
Show 6 quoted lines
> + 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.
Show 9 quoted lines
> @@ -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.