Junio C Hamano <gitster@xxxxxxxxx> 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.
> 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.
> @@ -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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html