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

From
Jonathan Tan <hidden>
Date
2021-09-22 22:39:42
Reply to
Taylor Blau, 2021-09-15
quoted hunk ↗ jump to hunk
@@ -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...
quoted hunk ↗ jump to hunk
+		}
+	}
+
+	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)...
quoted hunk ↗ jump to hunk
+
+		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).

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.

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.
Thread (67 messages) 67 messages, 6 authors, last reply 2021-10-01
Keyboard shortcuts
hback out one level
jnext message in thread / next series-diff position
kprevious message in thread / previous series-diff position
ldrill in
Escclose help / fold thread tree
?toggle this help