freshlybakedca.ke / git
Git fork
fork atom

Merge branch 'ds/midx-write-fixes'

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

Junio C Hamano e18e761b 13d1e868

+84 -69
+63 -68
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(struct odb_source *source, 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; ··· 261 260 static void midx_fanout_add_midx_fanout(struct midx_fanout *fanout, 262 261 struct multi_pack_index *m, 263 262 uint32_t cur_fanout, 264 - int preferred_pack) 263 + uint32_t preferred_pack) 265 264 { 266 265 uint32_t start = m->num_objects_in_base, end; 267 266 uint32_t cur_object; ··· 275 274 end = m->num_objects_in_base + ntohl(m->chunk_oid_fanout[cur_fanout]); 276 275 277 276 for (cur_object = start; cur_object < end; cur_object++) { 278 - if ((preferred_pack > -1) && 277 + if ((preferred_pack != NO_PREFERRED_PACK) && 279 278 (preferred_pack == nth_midxed_pack_int_id(m, cur_object))) { 280 279 /* 281 280 * Objects from preferred packs are added ··· 365 364 preferred, cur_fanout); 366 365 } 367 366 368 - if (-1 < ctx->preferred_pack_idx && ctx->preferred_pack_idx < start_pack) 367 + if (ctx->preferred_pack_idx != NO_PREFERRED_PACK && 368 + ctx->preferred_pack_idx < start_pack) 369 369 midx_fanout_add_pack_fanout(&fanout, ctx->info, 370 370 ctx->preferred_pack_idx, 1, 371 371 cur_fanout); ··· 841 841 uint32_t commits_nr, 842 842 unsigned flags) 843 843 { 844 - int ret, i; 844 + int ret; 845 845 uint16_t options = 0; 846 846 struct bitmap_writer writer; 847 847 struct pack_idx_entry **index; ··· 868 868 * this order). 869 869 */ 870 870 ALLOC_ARRAY(index, pdata->nr_objects); 871 - for (i = 0; i < pdata->nr_objects; i++) 871 + for (uint32_t i = 0; i < pdata->nr_objects; i++) 872 872 index[i] = &pdata->objects[i].idx; 873 873 874 874 bitmap_writer_init(&writer, ctx->repo, pdata, ··· 889 889 * happens between bitmap_writer_build_type_index() and 890 890 * bitmap_writer_finish(). 891 891 */ 892 - for (i = 0; i < pdata->nr_objects; i++) 892 + for (uint32_t i = 0; i < pdata->nr_objects; i++) 893 893 index[ctx->pack_order[i]] = &pdata->objects[i].idx; 894 894 895 895 bitmap_writer_select_commits(&writer, commits, commits_nr); ··· 910 910 return ret; 911 911 } 912 912 913 - static int fill_packs_from_midx(struct write_midx_context *ctx, 914 - const char *preferred_pack_name, uint32_t flags) 913 + static int fill_packs_from_midx(struct write_midx_context *ctx) 915 914 { 916 915 struct multi_pack_index *m; 917 916 ··· 919 918 uint32_t i; 920 919 921 920 for (i = 0; i < m->num_packs; i++) { 922 - ALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc); 923 - 924 - /* 925 - * If generating a reverse index, need to have 926 - * packed_git's loaded to compare their 927 - * mtimes and object count. 928 - * 929 - * If a preferred pack is specified, need to 930 - * have packed_git's loaded to ensure the chosen 931 - * preferred pack has a non-zero object count. 932 - */ 933 - if (flags & MIDX_WRITE_REV_INDEX || 934 - preferred_pack_name) { 935 - if (prepare_midx_pack(m, m->num_packs_in_base + i)) { 936 - error(_("could not load pack")); 937 - return 1; 938 - } 921 + if (prepare_midx_pack(m, m->num_packs_in_base + i)) 922 + return error(_("could not load pack")); 939 923 940 - if (open_pack_index(m->packs[i])) 941 - die(_("could not open index for %s"), 942 - m->packs[i]->pack_name); 943 - } 944 - 924 + ALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc); 945 925 fill_pack_info(&ctx->info[ctx->nr++], m->packs[i], 946 926 m->pack_names[i], 947 927 m->num_packs_in_base + i); ··· 1045 1025 struct repository *r = source->odb->repo; 1046 1026 struct strbuf midx_name = STRBUF_INIT; 1047 1027 unsigned char midx_hash[GIT_MAX_RAWSZ]; 1048 - uint32_t i, start_pack; 1028 + uint32_t start_pack; 1049 1029 struct hashfile *f = NULL; 1050 1030 struct lock_file lk; 1051 1031 struct tempfile *incr; 1052 - struct write_midx_context ctx = { 0 }; 1032 + struct write_midx_context ctx = { 1033 + .preferred_pack_idx = NO_PREFERRED_PACK, 1034 + }; 1053 1035 int bitmapped_packs_concat_len = 0; 1054 1036 int pack_name_concat_len = 0; 1055 1037 int dropped_packs = 0; 1056 - int result = 0; 1038 + int result = -1; 1057 1039 const char **keep_hashes = NULL; 1058 1040 struct chunkfile *cf; 1059 1041 ··· 1107 1089 error(_("could not load reverse index for MIDX %s"), 1108 1090 hash_to_hex_algop(get_midx_checksum(m), 1109 1091 m->source->odb->repo->hash_algo)); 1110 - result = 1; 1111 1092 goto cleanup; 1112 1093 } 1113 1094 ctx.num_multi_pack_indexes_before++; 1114 1095 m = m->base_midx; 1115 1096 } 1116 - } else if (ctx.m && fill_packs_from_midx(&ctx, preferred_pack_name, 1117 - flags) < 0) { 1097 + } else if (ctx.m && fill_packs_from_midx(&ctx)) { 1118 1098 goto cleanup; 1119 1099 } 1120 1100 ··· 1150 1130 */ 1151 1131 if (!want_bitmap) 1152 1132 clear_midx_files_ext(source, "bitmap", NULL); 1133 + result = 0; 1153 1134 goto cleanup; 1154 1135 } 1155 1136 } 1156 1137 1157 - if (ctx.incremental && !ctx.nr) 1138 + if (ctx.incremental && !ctx.nr) { 1139 + result = 0; 1158 1140 goto cleanup; /* nothing to do */ 1141 + } 1159 1142 1160 1143 if (preferred_pack_name) { 1161 - ctx.preferred_pack_idx = -1; 1144 + ctx.preferred_pack_idx = NO_PREFERRED_PACK; 1162 1145 1163 - for (i = 0; i < ctx.nr; i++) { 1146 + for (size_t i = 0; i < ctx.nr; i++) { 1164 1147 if (!cmp_idx_or_pack_name(preferred_pack_name, 1165 1148 ctx.info[i].pack_name)) { 1166 1149 ctx.preferred_pack_idx = i; ··· 1168 1151 } 1169 1152 } 1170 1153 1171 - if (ctx.preferred_pack_idx == -1) 1154 + if (ctx.preferred_pack_idx == NO_PREFERRED_PACK) 1172 1155 warning(_("unknown preferred pack: '%s'"), 1173 1156 preferred_pack_name); 1174 1157 } else if (ctx.nr && 1175 1158 (flags & (MIDX_WRITE_REV_INDEX | MIDX_WRITE_BITMAP))) { 1176 - struct packed_git *oldest = ctx.info[ctx.preferred_pack_idx].p; 1159 + struct packed_git *oldest = ctx.info[0].p; 1177 1160 ctx.preferred_pack_idx = 0; 1178 1161 1162 + /* 1163 + * Attempt opening the pack index to populate num_objects. 1164 + * Ignore failiures as they can be expected and are not 1165 + * fatal during this selection time. 1166 + */ 1167 + open_pack_index(oldest); 1168 + 1179 1169 if (packs_to_drop && packs_to_drop->nr) 1180 1170 BUG("cannot write a MIDX bitmap during expiration"); 1181 1171 ··· 1185 1175 * pack-order has all of its objects selected from that pack 1186 1176 * (and not another pack containing a duplicate) 1187 1177 */ 1188 - for (i = 1; i < ctx.nr; i++) { 1178 + for (size_t i = 1; i < ctx.nr; i++) { 1189 1179 struct packed_git *p = ctx.info[i].p; 1190 1180 1191 1181 if (!oldest->num_objects || p->mtime < oldest->mtime) { 1192 1182 oldest = p; 1183 + open_pack_index(oldest); 1193 1184 ctx.preferred_pack_idx = i; 1194 1185 } 1195 1186 } ··· 1201 1192 * objects to resolve, so the preferred value doesn't 1202 1193 * matter. 1203 1194 */ 1204 - ctx.preferred_pack_idx = -1; 1195 + ctx.preferred_pack_idx = NO_PREFERRED_PACK; 1205 1196 } 1206 1197 } else { 1207 1198 /* 1208 1199 * otherwise don't mark any pack as preferred to avoid 1209 1200 * interfering with expiration logic below 1210 1201 */ 1211 - ctx.preferred_pack_idx = -1; 1202 + ctx.preferred_pack_idx = NO_PREFERRED_PACK; 1212 1203 } 1213 1204 1214 - if (ctx.preferred_pack_idx > -1) { 1205 + if (ctx.preferred_pack_idx != NO_PREFERRED_PACK) { 1215 1206 struct packed_git *preferred = ctx.info[ctx.preferred_pack_idx].p; 1207 + 1208 + if (open_pack_index(preferred)) 1209 + die(_("failed to open preferred pack %s"), 1210 + ctx.info[ctx.preferred_pack_idx].pack_name); 1211 + 1216 1212 if (!preferred->num_objects) { 1217 1213 error(_("cannot select preferred pack %s with no objects"), 1218 1214 preferred->pack_name); 1219 - result = 1; 1220 1215 goto cleanup; 1221 1216 } 1222 1217 } ··· 1224 1219 compute_sorted_entries(&ctx, start_pack); 1225 1220 1226 1221 ctx.large_offsets_needed = 0; 1227 - for (i = 0; i < ctx.entries_nr; i++) { 1222 + for (size_t i = 0; i < ctx.entries_nr; i++) { 1228 1223 if (ctx.entries[i].offset > 0x7fffffff) 1229 1224 ctx.num_large_offsets++; 1230 1225 if (ctx.entries[i].offset > 0xffffffff) ··· 1234 1229 QSORT(ctx.info, ctx.nr, pack_info_compare); 1235 1230 1236 1231 if (packs_to_drop && packs_to_drop->nr) { 1237 - int drop_index = 0; 1232 + size_t drop_index = 0; 1238 1233 int missing_drops = 0; 1239 1234 1240 - for (i = 0; i < ctx.nr && drop_index < packs_to_drop->nr; i++) { 1235 + for (size_t i = 0; i < ctx.nr && drop_index < packs_to_drop->nr; i++) { 1241 1236 int cmp = strcmp(ctx.info[i].pack_name, 1242 1237 packs_to_drop->items[drop_index].string); 1243 1238 ··· 1255 1250 } 1256 1251 } 1257 1252 1258 - if (missing_drops) { 1259 - result = 1; 1253 + if (missing_drops) 1260 1254 goto cleanup; 1261 - } 1262 1255 } 1263 1256 1264 1257 /* ··· 1268 1261 * pack_perm[old_id] = new_id 1269 1262 */ 1270 1263 ALLOC_ARRAY(ctx.pack_perm, ctx.nr); 1271 - for (i = 0; i < ctx.nr; i++) { 1264 + for (size_t i = 0; i < ctx.nr; i++) { 1272 1265 if (ctx.info[i].expired) { 1273 1266 dropped_packs++; 1274 1267 ctx.pack_perm[ctx.info[i].orig_pack_int_id] = PACK_EXPIRED; ··· 1277 1270 } 1278 1271 } 1279 1272 1280 - for (i = 0; i < ctx.nr; i++) { 1273 + for (size_t i = 0; i < ctx.nr; i++) { 1281 1274 if (ctx.info[i].expired) 1282 1275 continue; 1283 1276 pack_name_concat_len += strlen(ctx.info[i].pack_name) + 1; ··· 1304 1297 1305 1298 if (ctx.nr - dropped_packs == 0) { 1306 1299 error(_("no pack files to index.")); 1307 - result = 1; 1308 1300 goto cleanup; 1309 1301 } 1310 1302 ··· 1324 1316 incr = mks_tempfile_m(midx_name.buf, 0444); 1325 1317 if (!incr) { 1326 1318 error(_("unable to create temporary MIDX layer")); 1327 - return -1; 1319 + goto cleanup; 1328 1320 } 1329 1321 1330 1322 if (adjust_shared_perm(r, get_tempfile_path(incr))) { 1331 1323 error(_("unable to adjust shared permissions for '%s'"), 1332 1324 get_tempfile_path(incr)); 1333 - return -1; 1325 + goto cleanup; 1334 1326 } 1335 1327 1336 1328 f = hashfd(r->hash_algo, get_tempfile_fd(incr), ··· 1407 1399 midx_hash, &pdata, commits, commits_nr, 1408 1400 flags) < 0) { 1409 1401 error(_("could not write multi-pack bitmap")); 1410 - result = 1; 1411 1402 clear_packing_data(&pdata); 1412 1403 free(commits); 1413 1404 goto cleanup; ··· 1421 1412 * have been freed in the previous if block. 1422 1413 */ 1423 1414 1415 + if (ctx.num_multi_pack_indexes_before == UINT32_MAX) 1416 + die(_("too many multi-pack-indexes")); 1417 + 1424 1418 CALLOC_ARRAY(keep_hashes, ctx.num_multi_pack_indexes_before + 1); 1425 1419 1426 1420 if (ctx.incremental) { ··· 1430 1424 1431 1425 if (!chainf) { 1432 1426 error_errno(_("unable to open multi-pack-index chain file")); 1433 - return -1; 1427 + goto cleanup; 1434 1428 } 1435 1429 1436 1430 if (link_midx_to_chain(ctx.base_midx) < 0) 1437 - return -1; 1431 + goto cleanup; 1438 1432 1439 1433 get_split_midx_filename_ext(source, &final_midx_name, 1440 1434 midx_hash, MIDX_EXT_MIDX); 1441 1435 1442 1436 if (rename_tempfile(&incr, final_midx_name.buf) < 0) { 1443 1437 error_errno(_("unable to rename new multi-pack-index layer")); 1444 - return -1; 1438 + goto cleanup; 1445 1439 } 1446 1440 1447 1441 strbuf_release(&final_midx_name); ··· 1449 1443 keep_hashes[ctx.num_multi_pack_indexes_before] = 1450 1444 xstrdup(hash_to_hex_algop(midx_hash, r->hash_algo)); 1451 1445 1452 - for (i = 0; i < ctx.num_multi_pack_indexes_before; i++) { 1446 + for (uint32_t i = 0; i < ctx.num_multi_pack_indexes_before; i++) { 1453 1447 uint32_t j = ctx.num_multi_pack_indexes_before - i - 1; 1454 1448 1455 1449 keep_hashes[j] = xstrdup(hash_to_hex_algop(get_midx_checksum(m), ··· 1457 1451 m = m->base_midx; 1458 1452 } 1459 1453 1460 - for (i = 0; i < ctx.num_multi_pack_indexes_before + 1; i++) 1454 + for (uint32_t i = 0; i <= ctx.num_multi_pack_indexes_before; i++) 1461 1455 fprintf(get_lock_file_fp(&lk), "%s\n", keep_hashes[i]); 1462 1456 } else { 1463 1457 keep_hashes[ctx.num_multi_pack_indexes_before] = ··· 1473 1467 clear_midx_files(source, keep_hashes, 1474 1468 ctx.num_multi_pack_indexes_before + 1, 1475 1469 ctx.incremental); 1470 + result = 0; 1476 1471 1477 1472 cleanup: 1478 - for (i = 0; i < ctx.nr; i++) { 1473 + for (size_t i = 0; i < ctx.nr; i++) { 1479 1474 if (ctx.info[i].p) { 1480 1475 close_pack(ctx.info[i].p); 1481 1476 free(ctx.info[i].p); ··· 1488 1483 free(ctx.pack_perm); 1489 1484 free(ctx.pack_order); 1490 1485 if (keep_hashes) { 1491 - for (i = 0; i < ctx.num_multi_pack_indexes_before + 1; i++) 1486 + for (uint32_t i = 0; i <= ctx.num_multi_pack_indexes_before; i++) 1492 1487 free((char *)keep_hashes[i]); 1493 1488 free(keep_hashes); 1494 1489 }
+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