Re: [PATCH 05/17] pack-mtimes: support writing pack .mtimes files

[Taylor Blau <me@ttaylorr.com>]

On Thu, Dec 02, 2021 at 10:36:16AM -0500, Derrick Stolee wrote:
> On 11/29/2021 5:25 PM, Taylor Blau wrote:> @@ -168,6 +168,9 @@ struct packing_data {
> >  	/* delta islands */
> >  	unsigned int *tree_depth;
> >  	unsigned char *layer;
> > +
> > +	/* cruft packs */
> > +	uint32_t *cruft_mtime;
>
> This comment is a bit terse. Perhaps...
>
> 	/* Used when writing cruft packs. */

Sure; here I was imitating the terseness of the "delta islands" comment
a few lines above. But I don't mind changing it here.

> > +static inline uint32_t oe_cruft_mtime(struct packing_data *pack,
> > +				      struct object_entry *e)
> > +{
> > +	if (!pack->cruft_mtime)
> > +		return 0;
> > +	return pack->cruft_mtime[e - pack->objects];
> > +}
>
> When writing a pack, it appears that the cruft_mtime array
> maps to objects in pack-order, not idx-order, correct? That
> might be worth mentioning in the struct definition because
> it differs from the .mtimes file.

Great observation and suggestion, thank you! The comment that I
ultimately settled on is:

  /*
   * Used when writing cruft packs.
   *
   * Object mtimes  are stored in pack order when writing, but
   * written out in lexicographic (index) order.
   */
   uint32_t *cruft_mtime;

> > +static void write_mtimes_objects(struct hashfile *f,
> > +				 struct packing_data *to_pack,
> > +				 struct pack_idx_entry **objects,
> > +				 uint32_t nr_objects)
> > +{
> > +	uint32_t i;
> > +	for (i = 0; i < nr_objects; i++) {
> > +		struct object_entry *e = (struct object_entry*)objects[i];
> > +		hashwrite_be32(f, oe_cruft_mtime(to_pack, e));
> > +	}
>
> The name "objects" here confused me at first, thinking it
> corresponded to the objects member of 'struct packing_data', but
> that is being handled by the fact that 'objects' is actually a
> lex-sorted list of pack_idx_entry pointers (and they happen to
> also point to 'struct object_entry' values because the 'struct
> pack_idx_entry' is the first member.
>
> So this is (very densely) handling the translation from pack-order
> to lex-order through the double pointer 'objects'. I'm not sure if
> there is a way to make it more clear or if every reader will need
> to do the same mental gymnastics I had to do.

Exactly, and sorry that I didn't point this out more clearly. It's been
long enough since I wrote this code that I can sympathize with the
mental gymnastics required ;).

> > +}
> > +
> > +static void write_mtimes_trailer(struct hashfile *f, const unsigned char *hash)
> > +{
> > +	hashwrite(f, hash, the_hash_algo->rawsz);
> > +}
> > +
> > +static const char *write_mtimes_file(const char *mtimes_name,
> > +				     struct packing_data *to_pack,
> > +				     struct pack_idx_entry **objects,
> > +				     uint32_t nr_objects,
> > +				     const unsigned char *hash)
> > +{
> > +	struct hashfile *f;
> > +	int fd;
> > +
> > +	if (!to_pack)
> > +		BUG("cannot call write_mtimes_file with NULL packing_data");
> > +
> > +	if (!mtimes_name) {
> > +		struct strbuf tmp_file = STRBUF_INIT;
> > +		fd = odb_mkstemp(&tmp_file, "pack/tmp_mtimes_XXXXXX");
> > +		mtimes_name = strbuf_detach(&tmp_file, NULL);
> > +	} else {
> > +		unlink(mtimes_name);
> > +		fd = xopen(mtimes_name, O_CREAT|O_EXCL|O_WRONLY, 0600);
> > +	}
> > +	f = hashfd(fd, mtimes_name);
> > +
> > +	write_mtimes_header(f);
> > +	write_mtimes_objects(f, to_pack, objects, nr_objects);
> > +	write_mtimes_trailer(f, hash);
> > +
> > +	if (mtimes_name && adjust_shared_perm(mtimes_name) < 0)
> > +		die(_("failed to make %s readable"), mtimes_name);
>
> What could cause 'mtimes_name' to be NULL here? It seems that it would
> be initialized in the "if (!mtimes_name)" block above.

You're right, it's impossible for it to be NULL here. I'll remove the
redundant side of the &&-expression here.

> > +
> > +	finalize_hashfile(f, NULL,
> > +			  CSUM_HASH_IN_STREAM | CSUM_CLOSE | CSUM_FSYNC);
> > +
> > +	return mtimes_name;
>
> Note that you return the name here...
>
> > +	if (pack_idx_opts->flags & WRITE_MTIMES) {
> > +		mtimes_tmp_name = write_mtimes_file(NULL, to_pack, written_list,
> > +						    nr_written,
> > +						    hash);
> > +		if (adjust_shared_perm(mtimes_tmp_name))
> > +			die_errno("unable to make temporary mtimes file readable");
>
> ...and then adjust the perms again. I think that this adjustment is
> redundant, because it already happened within the write_mtimes_file()
> method.

Yep, thanks. I'll clean it up here to just call adjust_shared_perm()
witin write_mtimes_file().

Thanks,
Taylor