On Thu, Sep 09, 2021 at 12:29:01PM -0700, Junio C Hamano wrote:
> Taylor Blau <me@xxxxxxxxxxxx> writes:
>
> > @@ -1250,8 +1251,9 @@ static void write_pack_file(void)
> > &pack_idx_opts, hash);
> >
> > if (write_bitmap_index) {
> > - strbuf_addf(&tmpname, "%s.bitmap", hash_to_hex(hash));
> > + size_t tmpname_len = tmpname.len;
> >
> > + strbuf_addstr(&tmpname, "bitmap");
> > stop_progress(&progress_state);
> >
> > bitmap_writer_show_progress(progress);
> > @@ -1260,6 +1262,7 @@ static void write_pack_file(void)
> > bitmap_writer_finish(written_list, nr_written,
> > tmpname.buf, write_bitmap_options);
> > write_bitmap_index = 0;
> > + strbuf_setlen(&tmpname, tmpname_len);
> > }
> >
> > strbuf_release(&tmpname);
>
> This runs setlen on tmpname strbuf and immediately releases (the
> close brace before release closes the "if (write_bitmap_index)", not
> any loop. If we plan to insert more code to use tmpname where the
> blank line we see above is in the future, it may make sense, but
> even in such a case, adding setlen() only when it starts to matter
> may make it easier to follow.
>
> In any case, the above correctly adjusts tmpname to have a <hash>
> plus '.' at the end of tmpname to call finish_tmp_packfile().
Ævar makes a slight mention of this in their commit message:
This approach of reusing the buffer does make the last user in
pack-object.c's write_pack_file() slightly awkward, since we
needlessly do a strbuf_setlen() before calling strbuf_release() for
consistency. In subsequent changes we'll move that bitmap writing
code around, so let's not skip the strbuf_setlen() now.
And this is to set us up for the final patch which moves the call to
rename_tmp_packfile_idx(&tmpname, &idx_tmp_name) after the closing brace
in the quoted portion of your message and the strbuf_release(). There,
we'll want the strbuf reset to the appropriate contents and length, and
not released.
But in the meantime it is awkward.
> I do not like '*.%s' here. Without '*' it is clear enough, and
> because nb->buf has already the same ext information,
>
> [...] But it would be more friendly to those adding more calls to this
> function in the future to document the convention in a comment before
> the function.
>
> Also, the name "nb" would need rethinking. As far as the callers
> are concerned, that is not a full name, but they are supplying the
> common prefix to the function. Perhaps "struct strbuf *name_prefix"
> or soemthing might be better? I dunno.
In each of these three, I wasn't able to decide if you wanted these
addressed in a newer version of this series, or if you were happy enough
with the result to pick it up. I'm happy to send you a new version, but
don't want to clog your inbox if you were already planning on picking
this up.
Thanks,
Taylor
