Re: [PATCH 3/9] pack-write: refactor renaming in finish_tmp_packfile()

From: Taylor Blau <me@ttaylorr.com>
Date: Sep 9, 2021, 21:07 UTC
Message-ID: <YTp3rYqxLE5/X521@nand.local>
In-Reply-To: <xmqqk0jpzgxu.fsf@gitster.g>

On Thu, Sep 09, 2021 at 12:29:01PM -0700, Junio C Hamano wrote:

Show 31 quoted lines

> Taylor Blau <me@ttaylorr.com> 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.

Show 11 quoted lines

> 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