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. */
> +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.
> +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.
> +}
> +
> +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.
> +
> + 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.
> + }
> +
> rename_tmp_packfile(name_buffer, pack_tmp_name, "pack");
> if (rev_tmp_name)
> rename_tmp_packfile(name_buffer, rev_tmp_name, "rev");
> + if (mtimes_tmp_name)
> + rename_tmp_packfile(name_buffer, mtimes_tmp_name, "mtimes");
And then it is finally renamed here, if it had a temporary name to
start.
Thanks,
-Stolee
