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

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 {

quoted

 	/* 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.

quoted

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

quoted

+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 ;).

quoted

+}
+
+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.

quoted

+
+	finalize_hashfile(f, NULL,
+			  CSUM_HASH_IN_STREAM | CSUM_CLOSE | CSUM_FSYNC);
+
+	return mtimes_name;

Note that you return the name here...

quoted

+	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