Re: [PATCH 05/17] pack-mtimes: support writing pack .mtimes files
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Dec 3, 2021, 23:04 UTC
- Message-ID
- <YaqiYGM48p5F9lS1@nand.local>
- In-Reply-To
- <2a26da15-8d4f-92b9-d727-debf8b969899@gmail.com>
On Thu, Dec 02, 2021 at 10:36:16AM -0500, Derrick Stolee wrote:
Show 11 quoted lines
> 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.
Show 12 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.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;
Show 22 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.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 ;).
Show 38 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.You're right, it's impossible for it to be NULL here. I'll remove the redundant side of the &&-expression here.
Show 18 quoted lines
> > +
> > + 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