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
