On Sun, Oct 1, 2023 at 10:11 PM Eric W. Biederman <ebiederm@xxxxxxxxx> wrote:
> Eric Sunshine <sunshine@xxxxxxxxxxxxxx> writes:
> > On Wed, Sep 27, 2023 at 3:56 PM Eric W. Biederman <ebiederm@xxxxxxxxx> wrote:
> >> + for (; iter != kh_end(map); iter++) {
> >> + if (kh_exist(map, iter)) {
> >> + if (oideq(&kh_key(map, iter), the_hash_algo->empty_tree) ||
> >> + oideq(&kh_key(map, iter), the_hash_algo->empty_blob))
> >> + continue;
> >> + strbuf_addf(&buf, "%s %s\n", oid_to_hex(&kh_key(map, iter)), oid_to_hex(kh_value(map, iter)));
> >> + if (write_in_full(fd, buf.buf, buf.len) < 0)
> >> + goto errout;
> >> + strbuf_reset(&buf);
> >> + }
> >> + }
> >
> > Nit: If you call strbuf_reset() immediately before strbuf_addf(), then
> > you save the reader of the code the effort of having to scan backward
> > through the function to verify that `buf` is empty the first time
> > through the loop.
>
> I am actually perplexed why the code works this way.
>
> My gut says we should create the entire buffer in memory and then
> write it to disk all in a single system call, and not perform
> this line buffering.
I think I had a similar question/thought while reading the patch but...
> Doing that would remove the need for the strbuf_reset entirely,
> and would just require the buffer to be primed with the
> loose_object_header.
>
> But what is there works and I will leave it for now.
... came to this same conclusion.
> It isn't a bug, and it can be improved with a follow up patch.
Exactly.
I haven't thought through the consequences, but perhaps brian was
worried about the memory footprint of buffering the whole thing before
writing to disk.
