Git fork

Merge branch 'ds/midx-write-fixes' into maint-2.51

Fixes multiple crashes around midx write-out codepaths.

* ds/midx-write-fixes:
midx-write: simplify error cases
midx-write: reenable signed comparison errors
midx-write: use uint32_t for preferred_pack_idx
midx-write: use cleanup when incremental midx fails
midx-write: put failing response value back
midx-write: only load initialized packs

+86 -70
+65 -69
midx-write.c
··· 1 - #define DISABLE_SIGN_COMPARE_WARNINGS 2 - 3 1 #include "git-compat-util.h" 4 2 #include "abspath.h" 5 3 #include "config.h" ··· 24 22 #define BITMAP_POS_UNKNOWN (~((uint32_t)0)) 25 23 #define MIDX_CHUNK_FANOUT_SIZE (sizeof(uint32_t) * 256) 26 24 #define MIDX_CHUNK_LARGE_OFFSET_WIDTH (sizeof(uint64_t)) 25 + #define NO_PREFERRED_PACK (~((uint32_t)0)) 27 26 28 27 extern int midx_checksum_valid(struct multi_pack_index *m); 29 28 extern void clear_midx_files_ext(const char *object_dir, const char *ext, ··· 104 103 unsigned large_offsets_needed:1; 105 104 uint32_t num_large_offsets; 106 105 107 - int preferred_pack_idx; 106 + uint32_t preferred_pack_idx; 108 107 109 108 int incremental; 110 109 uint32_t num_multi_pack_indexes_before; ··· 260 259 static void midx_fanout_add_midx_fanout(struct midx_fanout *fanout, 261 260 struct multi_pack_index *m, 262 261 uint32_t cur_fanout, 263 - int preferred_pack) 262 + uint32_t preferred_pack) 264 263 { 265 264 uint32_t start = m->num_objects_in_base, end; 266 265 uint32_t cur_object; ··· 274 273 end = m->num_objects_in_base + ntohl(m->chunk_oid_fanout[cur_fanout]); 275 274 276 275 for (cur_object = start; cur_object < end; cur_object++) { 277 - if ((preferred_pack > -1) && 276 + if ((preferred_pack != NO_PREFERRED_PACK) && 278 277 (preferred_pack == nth_midxed_pack_int_id(m, cur_object))) { 279 278 /* 280 279 * Objects from preferred packs are added ··· 364 363 preferred, cur_fanout); 365 364 } 366 365 367 - if (-1 < ctx->preferred_pack_idx && ctx->preferred_pack_idx < start_pack) 366 + if (ctx->preferred_pack_idx != NO_PREFERRED_PACK && 367 + ctx->preferred_pack_idx < start_pack) 368 368 midx_fanout_add_pack_fanout(&fanout, ctx->info, 369 369 ctx->preferred_pack_idx, 1, 370 370 cur_fanout); ··· 843 843 uint32_t commits_nr, 844 844 unsigned flags) 845 845 { 846 - int ret, i; 846 + int ret; 847 847 uint16_t options = 0; 848 848 struct bitmap_writer writer; 849 849 struct pack_idx_entry **index; ··· 871 871 * this order). 872 872 */ 873 873 ALLOC_ARRAY(index, pdata->nr_objects); 874 - for (i = 0; i < pdata->nr_objects; i++) 874 + for (uint32_t i = 0; i < pdata->nr_objects; i++) 875 875 index[i] = &pdata->objects[i].idx; 876 876 877 877 bitmap_writer_init(&writer, ctx->repo, pdata, ··· 892 892 * happens between bitmap_writer_build_type_index() and 893 893 * bitmap_writer_finish(). 894 894 */ 895 - for (i = 0; i < pdata->nr_objects; i++) 895 + for (uint32_t i = 0; i < pdata->nr_objects; i++) 896 896 index[ctx->pack_order[i]] = &pdata->objects[i].idx; 897 897 898 898 bitmap_writer_select_commits(&writer, commits, commits_nr); ··· 920 920 return get_multi_pack_index(source); 921 921 } 922 922 923 - static int fill_packs_from_midx(struct write_midx_context *ctx, 924 - const char *preferred_pack_name, uint32_t flags) 923 + static int fill_packs_from_midx(struct write_midx_context *ctx) 925 924 { 926 925 struct multi_pack_index *m; 927 926 ··· 929 928 uint32_t i; 930 929 931 930 for (i = 0; i < m->num_packs; i++) { 932 - ALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc); 931 + if (prepare_midx_pack(ctx->repo, m, 932 + m->num_packs_in_base + i)) 933 + return error(_("could not load pack")); 933 934 934 - /* 935 - * If generating a reverse index, need to have 936 - * packed_git's loaded to compare their 937 - * mtimes and object count. 938 - * 939 - * If a preferred pack is specified, need to 940 - * have packed_git's loaded to ensure the chosen 941 - * preferred pack has a non-zero object count. 942 - */ 943 - if (flags & MIDX_WRITE_REV_INDEX || 944 - preferred_pack_name) { 945 - if (prepare_midx_pack(ctx->repo, m, 946 - m->num_packs_in_base + i)) { 947 - error(_("could not load pack")); 948 - return 1; 949 - } 950 - 951 - if (open_pack_index(m->packs[i])) 952 - die(_("could not open index for %s"), 953 - m->packs[i]->pack_name); 954 - } 955 - 935 + ALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc); 956 936 fill_pack_info(&ctx->info[ctx->nr++], m->packs[i], 957 937 m->pack_names[i], 958 938 m->num_packs_in_base + i); ··· 1056 1036 { 1057 1037 struct strbuf midx_name = STRBUF_INIT; 1058 1038 unsigned char midx_hash[GIT_MAX_RAWSZ]; 1059 - uint32_t i, start_pack; 1039 + uint32_t start_pack; 1060 1040 struct hashfile *f = NULL; 1061 1041 struct lock_file lk; 1062 1042 struct tempfile *incr; 1063 - struct write_midx_context ctx = { 0 }; 1043 + struct write_midx_context ctx = { 1044 + .preferred_pack_idx = NO_PREFERRED_PACK, 1045 + }; 1064 1046 int bitmapped_packs_concat_len = 0; 1065 1047 int pack_name_concat_len = 0; 1066 1048 int dropped_packs = 0; 1067 - int result = 0; 1049 + int result = -1; 1068 1050 const char **keep_hashes = NULL; 1069 1051 struct chunkfile *cf; 1070 1052 ··· 1117 1099 error(_("could not load reverse index for MIDX %s"), 1118 1100 hash_to_hex_algop(get_midx_checksum(m), 1119 1101 m->repo->hash_algo)); 1120 - result = 1; 1121 1102 goto cleanup; 1122 1103 } 1123 1104 ctx.num_multi_pack_indexes_before++; 1124 1105 m = m->base_midx; 1125 1106 } 1126 - } else if (ctx.m && fill_packs_from_midx(&ctx, preferred_pack_name, 1127 - flags) < 0) { 1107 + } else if (ctx.m && fill_packs_from_midx(&ctx)) { 1128 1108 goto cleanup; 1129 1109 } 1130 1110 ··· 1160 1140 */ 1161 1141 if (!want_bitmap) 1162 1142 clear_midx_files_ext(object_dir, "bitmap", NULL); 1143 + 1144 + result = 0; 1163 1145 goto cleanup; 1164 1146 } 1165 1147 } 1166 1148 1167 - if (ctx.incremental && !ctx.nr) 1149 + if (ctx.incremental && !ctx.nr) { 1150 + result = 0; 1168 1151 goto cleanup; /* nothing to do */ 1152 + } 1169 1153 1170 1154 if (preferred_pack_name) { 1171 - ctx.preferred_pack_idx = -1; 1155 + ctx.preferred_pack_idx = NO_PREFERRED_PACK; 1172 1156 1173 - for (i = 0; i < ctx.nr; i++) { 1157 + for (size_t i = 0; i < ctx.nr; i++) { 1174 1158 if (!cmp_idx_or_pack_name(preferred_pack_name, 1175 1159 ctx.info[i].pack_name)) { 1176 1160 ctx.preferred_pack_idx = i; ··· 1178 1162 } 1179 1163 } 1180 1164 1181 - if (ctx.preferred_pack_idx == -1) 1165 + if (ctx.preferred_pack_idx == NO_PREFERRED_PACK) 1182 1166 warning(_("unknown preferred pack: '%s'"), 1183 1167 preferred_pack_name); 1184 1168 } else if (ctx.nr && 1185 1169 (flags & (MIDX_WRITE_REV_INDEX | MIDX_WRITE_BITMAP))) { 1186 - struct packed_git *oldest = ctx.info[ctx.preferred_pack_idx].p; 1170 + struct packed_git *oldest = ctx.info[0].p; 1187 1171 ctx.preferred_pack_idx = 0; 1188 1172 1173 + /* 1174 + * Attempt opening the pack index to populate num_objects. 1175 + * Ignore failiures as they can be expected and are not 1176 + * fatal during this selection time. 1177 + */ 1178 + open_pack_index(oldest); 1179 + 1189 1180 if (packs_to_drop && packs_to_drop->nr) 1190 1181 BUG("cannot write a MIDX bitmap during expiration"); 1191 1182 ··· 1195 1186 * pack-order has all of its objects selected from that pack 1196 1187 * (and not another pack containing a duplicate) 1197 1188 */ 1198 - for (i = 1; i < ctx.nr; i++) { 1189 + for (size_t i = 1; i < ctx.nr; i++) { 1199 1190 struct packed_git *p = ctx.info[i].p; 1200 1191 1201 1192 if (!oldest->num_objects || p->mtime < oldest->mtime) { 1202 1193 oldest = p; 1194 + open_pack_index(oldest); 1203 1195 ctx.preferred_pack_idx = i; 1204 1196 } 1205 1197 } ··· 1211 1203 * objects to resolve, so the preferred value doesn't 1212 1204 * matter. 1213 1205 */ 1214 - ctx.preferred_pack_idx = -1; 1206 + ctx.preferred_pack_idx = NO_PREFERRED_PACK; 1215 1207 } 1216 1208 } else { 1217 1209 /* 1218 1210 * otherwise don't mark any pack as preferred to avoid 1219 1211 * interfering with expiration logic below 1220 1212 */ 1221 - ctx.preferred_pack_idx = -1; 1213 + ctx.preferred_pack_idx = NO_PREFERRED_PACK; 1222 1214 } 1223 1215 1224 - if (ctx.preferred_pack_idx > -1) { 1216 + if (ctx.preferred_pack_idx != NO_PREFERRED_PACK) { 1225 1217 struct packed_git *preferred = ctx.info[ctx.preferred_pack_idx].p; 1218 + 1219 + if (open_pack_index(preferred)) 1220 + die(_("failed to open preferred pack %s"), 1221 + ctx.info[ctx.preferred_pack_idx].pack_name); 1222 + 1226 1223 if (!preferred->num_objects) { 1227 1224 error(_("cannot select preferred pack %s with no objects"), 1228 1225 preferred->pack_name); 1229 - result = 1; 1230 1226 goto cleanup; 1231 1227 } 1232 1228 } ··· 1234 1230 compute_sorted_entries(&ctx, start_pack); 1235 1231 1236 1232 ctx.large_offsets_needed = 0; 1237 - for (i = 0; i < ctx.entries_nr; i++) { 1233 + for (size_t i = 0; i < ctx.entries_nr; i++) { 1238 1234 if (ctx.entries[i].offset > 0x7fffffff) 1239 1235 ctx.num_large_offsets++; 1240 1236 if (ctx.entries[i].offset > 0xffffffff) ··· 1244 1240 QSORT(ctx.info, ctx.nr, pack_info_compare); 1245 1241 1246 1242 if (packs_to_drop && packs_to_drop->nr) { 1247 - int drop_index = 0; 1243 + size_t drop_index = 0; 1248 1244 int missing_drops = 0; 1249 1245 1250 - for (i = 0; i < ctx.nr && drop_index < packs_to_drop->nr; i++) { 1246 + for (size_t i = 0; i < ctx.nr && drop_index < packs_to_drop->nr; i++) { 1251 1247 int cmp = strcmp(ctx.info[i].pack_name, 1252 1248 packs_to_drop->items[drop_index].string); 1253 1249 ··· 1265 1261 } 1266 1262 } 1267 1263 1268 - if (missing_drops) { 1269 - result = 1; 1264 + if (missing_drops) 1270 1265 goto cleanup; 1271 - } 1272 1266 } 1273 1267 1274 1268 /* ··· 1278 1272 * pack_perm[old_id] = new_id 1279 1273 */ 1280 1274 ALLOC_ARRAY(ctx.pack_perm, ctx.nr); 1281 - for (i = 0; i < ctx.nr; i++) { 1275 + for (size_t i = 0; i < ctx.nr; i++) { 1282 1276 if (ctx.info[i].expired) { 1283 1277 dropped_packs++; 1284 1278 ctx.pack_perm[ctx.info[i].orig_pack_int_id] = PACK_EXPIRED; ··· 1287 1281 } 1288 1282 } 1289 1283 1290 - for (i = 0; i < ctx.nr; i++) { 1284 + for (size_t i = 0; i < ctx.nr; i++) { 1291 1285 if (ctx.info[i].expired) 1292 1286 continue; 1293 1287 pack_name_concat_len += strlen(ctx.info[i].pack_name) + 1; ··· 1314 1308 1315 1309 if (ctx.nr - dropped_packs == 0) { 1316 1310 error(_("no pack files to index.")); 1317 - result = 1; 1318 1311 goto cleanup; 1319 1312 } 1320 1313 ··· 1334 1327 incr = mks_tempfile_m(midx_name.buf, 0444); 1335 1328 if (!incr) { 1336 1329 error(_("unable to create temporary MIDX layer")); 1337 - return -1; 1330 + goto cleanup; 1338 1331 } 1339 1332 1340 1333 if (adjust_shared_perm(r, get_tempfile_path(incr))) { 1341 1334 error(_("unable to adjust shared permissions for '%s'"), 1342 1335 get_tempfile_path(incr)); 1343 - return -1; 1336 + goto cleanup; 1344 1337 } 1345 1338 1346 1339 f = hashfd(r->hash_algo, get_tempfile_fd(incr), ··· 1417 1410 midx_hash, &pdata, commits, commits_nr, 1418 1411 flags) < 0) { 1419 1412 error(_("could not write multi-pack bitmap")); 1420 - result = 1; 1421 1413 clear_packing_data(&pdata); 1422 1414 free(commits); 1423 1415 goto cleanup; ··· 1431 1423 * have been freed in the previous if block. 1432 1424 */ 1433 1425 1426 + if (ctx.num_multi_pack_indexes_before == UINT32_MAX) 1427 + die(_("too many multi-pack-indexes")); 1428 + 1434 1429 CALLOC_ARRAY(keep_hashes, ctx.num_multi_pack_indexes_before + 1); 1435 1430 1436 1431 if (ctx.incremental) { ··· 1440 1435 1441 1436 if (!chainf) { 1442 1437 error_errno(_("unable to open multi-pack-index chain file")); 1443 - return -1; 1438 + goto cleanup; 1444 1439 } 1445 1440 1446 1441 if (link_midx_to_chain(ctx.base_midx) < 0) 1447 - return -1; 1442 + goto cleanup; 1448 1443 1449 1444 get_split_midx_filename_ext(r->hash_algo, &final_midx_name, 1450 1445 object_dir, midx_hash, MIDX_EXT_MIDX); 1451 1446 1452 1447 if (rename_tempfile(&incr, final_midx_name.buf) < 0) { 1453 1448 error_errno(_("unable to rename new multi-pack-index layer")); 1454 - return -1; 1449 + goto cleanup; 1455 1450 } 1456 1451 1457 1452 strbuf_release(&final_midx_name); ··· 1459 1454 keep_hashes[ctx.num_multi_pack_indexes_before] = 1460 1455 xstrdup(hash_to_hex_algop(midx_hash, r->hash_algo)); 1461 1456 1462 - for (i = 0; i < ctx.num_multi_pack_indexes_before; i++) { 1457 + for (uint32_t i = 0; i < ctx.num_multi_pack_indexes_before; i++) { 1463 1458 uint32_t j = ctx.num_multi_pack_indexes_before - i - 1; 1464 1459 1465 1460 keep_hashes[j] = xstrdup(hash_to_hex_algop(get_midx_checksum(m), ··· 1467 1462 m = m->base_midx; 1468 1463 } 1469 1464 1470 - for (i = 0; i < ctx.num_multi_pack_indexes_before + 1; i++) 1465 + for (uint32_t i = 0; i <= ctx.num_multi_pack_indexes_before; i++) 1471 1466 fprintf(get_lock_file_fp(&lk), "%s\n", keep_hashes[i]); 1472 1467 } else { 1473 1468 keep_hashes[ctx.num_multi_pack_indexes_before] = ··· 1483 1478 clear_midx_files(r, object_dir, keep_hashes, 1484 1479 ctx.num_multi_pack_indexes_before + 1, 1485 1480 ctx.incremental); 1481 + result = 0; 1486 1482 1487 1483 cleanup: 1488 - for (i = 0; i < ctx.nr; i++) { 1484 + for (size_t i = 0; i < ctx.nr; i++) { 1489 1485 if (ctx.info[i].p) { 1490 1486 close_pack(ctx.info[i].p); 1491 1487 free(ctx.info[i].p); ··· 1498 1494 free(ctx.pack_perm); 1499 1495 free(ctx.pack_order); 1500 1496 if (keep_hashes) { 1501 - for (i = 0; i < ctx.num_multi_pack_indexes_before + 1; i++) 1497 + for (uint32_t i = 0; i <= ctx.num_multi_pack_indexes_before; i++) 1502 1498 free((char *)keep_hashes[i]); 1503 1499 free(keep_hashes); 1504 1500 }
+21 -1
t/t5319-multi-pack-index.sh
··· 989 989 ) 990 990 ' 991 991 992 + test_expect_success EXPENSIVE 'repack/expire with many packs' ' 993 + cp -r dup many && 994 + ( 995 + cd many && 996 + 997 + for i in $(test_seq 1 100) 998 + do 999 + test_commit extra$i && 1000 + git maintenance run --task=loose-objects || return 1 1001 + done && 1002 + 1003 + git multi-pack-index write && 1004 + git multi-pack-index repack && 1005 + git multi-pack-index expire 1006 + ) 1007 + ' 1008 + 992 1009 test_expect_success 'repack --batch-size=<large> repacks everything' ' 993 1010 ( 994 1011 cd dup2 && ··· 1083 1100 mv $idx.bak $idx && 1084 1101 1085 1102 mv $pack $pack.bak && 1086 - git cat-file --batch-check="%(objectsize:disk)" <tip 1103 + git cat-file --batch-check="%(objectsize:disk)" <tip && 1104 + 1105 + test_must_fail git multi-pack-index write 2>err && 1106 + test_grep "could not load pack" err 1087 1107 ) 1088 1108 ' 1089 1109