Re: [PATCH v2 6/8] builtin/repack.c: support writing a MIDX while repacking

[Taylor Blau <me@ttaylorr.com>]

On Wed, Sep 22, 2021 at 03:39:35PM -0700, Jonathan Tan wrote:
> > @@ -683,22 +755,41 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
> >  	}
> >  	/* End of pack replacement. */
> >
> > +	if (delete_redundant && pack_everything & ALL_INTO_ONE) {
> > +		const int hexsz = the_hash_algo->hexsz;
> > +		string_list_sort(&names);
> > +		for_each_string_list_item(item, &existing_packs) {
> > +			char *sha1;
> > +			size_t len = strlen(item->string);
> > +			if (len < hexsz)
> > +				continue;
> > +			sha1 = item->string + len - hexsz;
> > +			item->util = (void*)(intptr_t)!string_list_has_string(&names, sha1);
>
> OK, here is the tricky part. They are marked for deletion here...

Yep, instead of deleting the packs immediately, which would cause an
existing MIDX referencing them to become corrupt, we split that into two
phases: one to figure out which packs need to be deleted (after the new
MIDX), and then another to actually delete those packs.

> > +		}
> > +	}
> > +
> > +	if (write_midx) {
> > +		struct string_list include = STRING_LIST_INIT_NODUP;
> > +		midx_included_packs(&include, &existing_packs,
> > +				    &existing_kept_packs, &names, geometry);
>
> ...the mark for deletion is taken into account during the execution of
> midx_included_packs() (as can be seen by looking at that function)...
>
> > +
> > +		ret = write_midx_included_packs(&include,
> > +						show_progress, write_bitmaps > 0);
> > +
> > +		string_list_clear(&include, 0);
> > +
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> >  	reprepare_packed_git(the_repository);
> >
> >  	if (delete_redundant) {
> >  		int opts = 0;
> > -		if (pack_everything & ALL_INTO_ONE) {
> > -			const int hexsz = the_hash_algo->hexsz;
> > -			string_list_sort(&names);
> > -			for_each_string_list_item(item, &existing_packs) {
> > -				char *sha1;
> > -				size_t len = strlen(item->string);
> > -				if (len < hexsz)
> > -					continue;
> > -				sha1 = item->string + len - hexsz;
> > -				if (!string_list_has_string(&names, sha1))
> > -					remove_redundant_pack(packdir, item->string);
> > -			}
> > +		for_each_string_list_item(item, &existing_packs) {
> > +			if (!item->util)
> > +				continue;
>
> ...and the marks are also used here. I was at first confused about why
> the functionality of midx_included_packs() depended on whether redundant
> packs were marked for deletion - if they are redundant, shouldn't they
> never be taken into account (regardless of whether we actually delete
> them)? But I guess it makes sense as an overall design point that we
> pass all packs that are to remain (so if they will be deleted, exclude
> them, and if they will not be, include them).

Right; midx_included_packs() is used to determine which packs are going
to be written into the new MIDX, and we need to make sure that we
exclude any packs which are about to be deleted.

> I think a comment "mark this pack for deletion" at the point we write
> the mark (so, where the cast to intptr_t is) is worthwhile. Other than
> that, this patch looks good to me.

Sure, that seems like it would be helpful to other readers.

> I (and the others in our review club) only managed to reach this patch.
> I hope to get to the other 2 by the end of the week.

Thanks for reviewing, I look forward to more of your feedback.

Thanks,
Taylor