Re: [PATCH 05/17] pack-mtimes: support writing pack .mtimes files
- From
Derrick Stolee <stolee@gmail.com>
- Date
- Dec 2, 2021, 15:36 UTC
- Message-ID
- <2a26da15-8d4f-92b9-d727-debf8b969899@gmail.com>
- In-Reply-To
- <deece9eb70e9750bb8350946679b521e59139fe2.1638224692.git.me@ttaylorr.com>
On 11/29/2021 5:25 PM, Taylor Blau wrote:> @@ -168,6 +168,9 @@ struct packing_data {Show 6 quoted lines
> /* 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. */
Show 7 quoted lines
> +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.
Show 10 quoted lines
> +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.
Show 35 quoted lines
> +}
> +
> +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.
Show 5 quoted lines
> + > + finalize_hashfile(f, NULL, > + CSUM_HASH_IN_STREAM | CSUM_CLOSE | CSUM_FSYNC); > + > + return mtimes_name;
Note that you return the name here...
Show 6 quoted lines
> + 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.
Show 7 quoted lines
> + } > + > 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