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

[Jeff King <peff@peff.net>]

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

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