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

Taylor Blau [off-list ref] writes:

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().

OK.

I do not like '*.%s' here.  Without '*' it is clear enough, and
because nb->buf has already the same ext information, 

	unable to rename temporary file to '%s'.

would be even simpler without losing any information than this
rewrite does.

The original explicitly used the more human-facing terms like "pack
file", so we are losing information by this refactoring, but the
loss is not too bad.  In one case, namely ".idx" files, it is even
better, as the original says "temporary index file" to refer to the
new .idx file, which makes it ambiguous with _the_ index file.

+	strbuf_setlen(nb, nb_len);
+}

In the longer term if we were to add more auxiliary files next to
each of the .pack file, it makes perfect sense that the common
prefix is fed to rename_tmp_packfile() and the function reverts
contents of the prefix buffer back when it is done with it.  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.

It is unfortunate that write_idx_file() and write_rev_file() take
hash and come up with the temporary filename on their own (which
means their resulting filenames may not share the same prefix as
.pack and .idx files), but it is just the naming of temporaries and
inconsistencies among them is, eh, temporary, so it is OK.

+	rename_tmp_packfile(name_buffer, pack_tmp_name, "pack");
+	rename_tmp_packfile(name_buffer, idx_tmp_name, "idx");
+	if (rev_tmp_name)
+		rename_tmp_packfile(name_buffer, rev_tmp_name, "rev");
 
 	free((void *)idx_tmp_name);
 }

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

Taylor Blau [off-list ref] writes:

quoted

@@ -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