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

[Junio C Hamano <gitster@pobox.com>]

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

> diff --git a/bulk-checkin.c b/bulk-checkin.c
> index 6283bc8bd9..c19d471f0b 100644
> --- a/bulk-checkin.c
> +++ b/bulk-checkin.c
> @@ -46,7 +46,8 @@ static void finish_bulk_checkin(struct bulk_checkin_state *state)
>  		close(fd);
>  	}
>  
> -	strbuf_addf(&packname, "%s/pack/pack-", get_object_directory());
> +	strbuf_addf(&packname, "%s/pack/pack-%s.", get_object_directory(),
> +		    hash_to_hex(hash));
>  	finish_tmp_packfile(&packname, state->pack_tmp_name,
>  			    state->written, state->nr_written,
>  			    &state->pack_idx_opts, hash);

OK.

> diff --git a/pack-write.c b/pack-write.c
> index 2767b78619..95b063be94 100644
> --- a/pack-write.c
> +++ b/pack-write.c
> @@ -458,6 +458,18 @@ struct hashfile *create_tmp_packfile(char **pack_tmp_name)
>  	return hashfd(fd, *pack_tmp_name);
>  }
>  
> +static void rename_tmp_packfile(struct strbuf *nb, const char *source,
> +				const char *ext)
> +{
> +	size_t nb_len = nb->len;
> +
> +	strbuf_addstr(nb, ext);
> +	if (rename(source, nb->buf))
> +		die_errno("unable to rename temporary '*.%s' file to '%s'",
> +			  ext, nb->buf);

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.

>  void finish_tmp_packfile(struct strbuf *name_buffer,
>  			 const char *pack_tmp_name,
>  			 struct pack_idx_entry **written_list,
> @@ -466,7 +478,6 @@ void finish_tmp_packfile(struct strbuf *name_buffer,
>  			 unsigned char hash[])
>  {
>  	const char *idx_tmp_name, *rev_tmp_name = NULL;
> -	int basename_len = name_buffer->len;
>  
>  	if (adjust_shared_perm(pack_tmp_name))
>  		die_errno("unable to make temporary pack file readable");
> @@ -479,26 +490,10 @@ void finish_tmp_packfile(struct strbuf *name_buffer,
>  	rev_tmp_name = write_rev_file(NULL, written_list, nr_written, hash,
>  				      pack_idx_opts->flags);

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);
>  }