Re: [PATCH 1/6] index-pack: factor out pack/idx finalization

[Jeff King <peff@peff.net>]

On Wed, Mar 15, 2017 at 06:22:23PM -0400, Jeff King wrote:

> Then we'd expect the newly-created .idx to have mode 0444, but it
> doesn't. So yeah, I think the distinction does matter.
> 
> I'm not sure if the best path is to include that flag in the
> finalize_file() helper, or just ditch the helper.  Its main purpose was
> cleanup so that the odb_pack_name() refactor didn't have to happen
> twice. But after that refactor, the amount of shared code is relatively
> small.

I think I'm leaning towards just ditching the helper. The resulting
change looks pretty good:

@@ -1423,22 +1424,16 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
 	}
 
 	if (final_pack_name != curr_pack_name) {
-		if (!final_pack_name) {
-			snprintf(name, sizeof(name), "%s/pack/pack-%s.pack",
-				 get_object_directory(), sha1_to_hex(sha1));
-			final_pack_name = name;
-		}
+		if (!final_pack_name)
+			final_pack_name = odb_pack_name(&name, sha1, "pack");
 		if (finalize_object_file(curr_pack_name, final_pack_name))
 			die(_("cannot store pack file"));
 	} else if (from_stdin)
 		chmod(final_pack_name, 0444);
 
 	if (final_index_name != curr_index_name) {
-		if (!final_index_name) {
-			snprintf(name, sizeof(name), "%s/pack/pack-%s.idx",
-				 get_object_directory(), sha1_to_hex(sha1));
-			final_index_name = name;
-		}
+		if (!final_index_name)
+			final_index_name = odb_pack_name(&name, sha1, "idx");
 		if (finalize_object_file(curr_index_name, final_index_name))
 			die(_("cannot store index file"));
 	} else

And we just end up sharing the same name buffer (and the "keep" code
path can use the same one, too).

I'll wait until tomorrow to collect any more comments and then send out
a re-roll.

Thanks for reading so carefully.

-Peff