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

From: Jeff King <peff@peff.net>
Date: Mar 15, 2017, 22:22 UTC
Message-ID: <20170315222223.qyrxb6ey3rz3omt4@sigill.intra.peff.net>
In-Reply-To: <f42ba0f5-a879-f63f-1cbd-de268da05966@ramsayjones.plus.com>

On Wed, Mar 15, 2017 at 10:03:56PM +0000, Ramsay Jones wrote:

Show 24 quoted lines

> > -	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 (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 (finalize_object_file(curr_index_name, final_index_name))
> > -			die(_("cannot store index file"));
> > -	} else
> > -		chmod(final_index_name, 0444);
> 
> Is from_stdin always true if final_index_name == curr_index_name?
> Was the original asymmetry deliberate?

Hrm, good question.

I think the logic on the pack side is that:

  git index-pack --stdin

will write a new pack, and we should chmod it as appropriate. But it would be wrong to do so when generating an index for an existing pack:

  git index-pack foo.pack

Whatever wrote foo.pack is responsible for the chmod then.

So the flip side (and what your question is asking) is whether it is safe to skip the .idx chmod in the non-stdin case.

Usually when we don't have an existing .idx file, we write to a new tempfile. We obviously want to chmod there, but it would fall under the "final_index_name != curr_index_name" case.

But it looks like you can give "-o", or we can derive the name from the .pack name. So if we do:

  git pack-objects --all --stdout >foo.pack
  git index-pack foo.pack

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.

-Peff