commit d22b3dbc4e92ed8f515c9fd4cd2e22bbb40cb441 from: Stefan Sperling via: Thomas Adam date: Thu Jan 30 18:04:18 2025 UTC rework got-read-pack's commit coloring loop Port the parent commit coloring fix to got-read-pack, and ensure that it starts off with the same state as the main process. got-read-pack did not have access to the main process's ids array, and was thus working with a different initial state. With these changes the same commit traversal happens regardless of whether coloring is "offloaded" to got-read-pack or not (verified manually by placing debug printfs). ok jamsek commit - 88b16410211fb0741a604736b34531fe3d56b8fd commit + d22b3dbc4e92ed8f515c9fd4cd2e22bbb40cb441 blob - 7dcc1e80f53fa249b30e5aa7ce4a8e337967d80b blob + ba7c6aae995f0e1e618534a513d3a695412df773 --- lib/got_lib_privsep.h +++ lib/got_lib_privsep.h @@ -345,13 +345,6 @@ struct got_imsg_reused_deltas { sizeof(struct got_imsg_reused_deltas)) \ / sizeof(struct got_imsg_reused_delta)) }; - -/* Structure for GOT_IMSG_COMMIT_PAINTING_REQUEST. */ -struct got_imsg_commit_painting_request { - struct got_object_id id; - int idx; - int color; -} __attribute__((__packed__)); /* Structure for GOT_IMSG_PAINTED_COMMITS. */ struct got_imsg_painted_commit { @@ -833,8 +826,7 @@ const struct got_error *got_privsep_recv_reused_deltas struct got_imsg_reused_delta *, size_t *, struct imsgbuf *); const struct got_error *got_privsep_init_commit_painting(struct imsgbuf *); -const struct got_error *got_privsep_send_painting_request(struct imsgbuf *, - int, struct got_object_id *, intptr_t); +const struct got_error *got_privsep_send_painting_request(struct imsgbuf *); typedef const struct got_error *(*got_privsep_recv_painted_commit_cb)(void *, struct got_object_id *, intptr_t); const struct got_error *got_privsep_send_painted_commits(struct imsgbuf *, blob - 47150180b95579e50c3b719155ffad96a8703195 blob + 209b393f44f5a2f7813238dde4fcbd2285f9811f --- lib/pack_create_privsep.c +++ lib/pack_create_privsep.c @@ -94,6 +94,39 @@ send_idset(struct imsgbuf *ibuf, struct got_object_ids } static const struct got_error * +send_filtered_id_queue(struct imsgbuf *ibuf, struct got_object_id_queue *ids, + uintptr_t color) +{ + const struct got_error *err = NULL; + struct got_object_qid *qid; + struct got_object_id *filtered_ids[GOT_IMSG_OBJ_ID_LIST_MAX_NIDS]; + int nids = 0; + + STAILQ_FOREACH(qid, ids, entry) { + if (color != (intptr_t)qid->data) + continue; + + filtered_ids[nids++] = &qid->id; + if (nids >= GOT_IMSG_OBJ_ID_LIST_MAX_NIDS) { + err = got_privsep_send_object_idlist(ibuf, + filtered_ids, nids); + if (err) + return err; + nids = 0; + } + + } + + if (nids > 0) { + err = got_privsep_send_object_idlist(ibuf, filtered_ids, nids); + if (err) + return err; + } + + return got_privsep_send_object_idlist_done(ibuf); +} + +static const struct got_error * recv_reused_delta(struct got_imsg_reused_delta *delta, struct got_object_idset *idset, struct got_pack_metavec *v) { @@ -209,6 +242,7 @@ struct recv_painted_commit_arg { struct got_ratelimit *rl; got_cancel_cb cancel_cb; void *cancel_arg; + struct got_object_qid **qid0; }; static const struct got_error * @@ -253,6 +287,8 @@ recv_painted_commit(void *arg, struct got_object_id *i continue; STAILQ_REMOVE(a->ids, qid, got_object_qid, entry); color = (intptr_t)qid->data; + if (*(a->qid0) == qid) + *(a->qid0) = NULL; got_object_qid_free(qid); (*a->nqueued)--; if (color == COLOR_SKIP) @@ -265,9 +301,9 @@ recv_painted_commit(void *arg, struct got_object_id *i } static const struct got_error * -paint_packed_commits(struct got_pack *pack, struct got_object_id *id, - int idx, intptr_t color, int *ncolored, int *nqueued, int *nskip, - struct got_object_id_queue *ids, +paint_packed_commits(struct got_object_qid **qid0, + struct got_pack *pack, intptr_t color, + int *ncolored, int *nqueued, int *nskip, struct got_object_id_queue *ids, struct got_object_idset *keep, struct got_object_idset *drop, struct got_object_idset *skip, struct got_repository *repo, got_pack_progress_cb progress_cb, void *progress_arg, @@ -280,11 +316,25 @@ paint_packed_commits(struct got_pack *pack, struct got STAILQ_INIT(&next_ids); - err = got_privsep_send_painting_request(pack->privsep_child->ibuf, - idx, id, color); + err = got_privsep_send_painting_request(pack->privsep_child->ibuf); + if (err) + return err; + + err = send_filtered_id_queue(pack->privsep_child->ibuf, ids, + COLOR_KEEP); + if (err) + return err; + + err = send_filtered_id_queue(pack->privsep_child->ibuf, ids, + COLOR_DROP); if (err) return err; + err = send_filtered_id_queue(pack->privsep_child->ibuf, ids, + COLOR_SKIP); + if (err) + return err; + arg.ncolored = ncolored; arg.nqueued = nqueued; arg.nskip = nskip; @@ -297,6 +347,7 @@ paint_packed_commits(struct got_pack *pack, struct got arg.rl = rl; arg.cancel_cb = cancel_cb; arg.cancel_arg = cancel_arg; + arg.qid0 = qid0; err = got_privsep_recv_painted_commits(&next_ids, recv_painted_commit, &arg, pack->privsep_child->ibuf); if (err) @@ -336,6 +387,33 @@ paint_packed_commits(struct got_pack *pack, struct got return err; } +static const struct got_error * +pin_pack_for_commit_painting(struct got_pack **pack, + struct got_packidx **packidx, struct got_object_id_queue *ids, int nqueued, + struct got_repository *repo) +{ + const struct got_error *err; + + err = got_pack_find_pack_for_commit_painting(packidx, ids, nqueued, + repo); + if (err || *packidx == NULL) { + *pack = NULL; + return err; + } + + err = got_pack_cache_pack_for_packidx(pack, *packidx, repo); + if (err) + return err; + + if ((*pack)->privsep_child == NULL) { + err = got_pack_start_privsep_child(*pack, *packidx); + if (err) + return err; + } + + return got_repo_pin_pack(repo, *packidx, *pack); +} + const struct got_error * got_pack_paint_commits(int *ncolored, struct got_object_id_queue *ids, int nids, struct got_object_idset *keep, struct got_object_idset *drop, @@ -352,6 +430,10 @@ got_pack_paint_commits(int *ncolored, struct got_objec int nqueued = nids, nskip = 0; int idx; + err = pin_pack_for_commit_painting(&pack, &packidx, ids, nqueued, repo); + if (err) + return err; + while (!STAILQ_EMPTY(ids) && nskip != nqueued) { intptr_t color; @@ -361,10 +443,63 @@ got_pack_paint_commits(int *ncolored, struct got_objec break; } + /* Pinned pack may have moved to different cache slot. */ + pack = got_repo_get_pinned_pack(repo); + if (pack == NULL) { + err = pin_pack_for_commit_painting(&pack, &packidx, + ids, nqueued, repo); + if (err) + break; + } + qid = STAILQ_FIRST(ids); + color = (intptr_t)qid->data; + + /* Use the fast path if we have a suitable pack file. */ + if (packidx && pack) { + idx = got_packidx_get_object_idx(packidx, &qid->id); + if (idx != -1) { + err = got_privsep_init_commit_painting( + pack->privsep_child->ibuf); + if (err) + goto done; + err = send_idset(pack->privsep_child->ibuf, + keep); + if (err) + goto done; + err = send_idset(pack->privsep_child->ibuf, + drop); + if (err) + goto done; + err = send_idset(pack->privsep_child->ibuf, + skip); + if (err) + goto done; + err = paint_packed_commits(&qid, pack, + color, ncolored, &nqueued, &nskip, + ids, keep, drop, skip, repo, + progress_cb, progress_arg, rl, + cancel_cb, cancel_arg); + if (err) + goto done; + if (qid) { + STAILQ_REMOVE(ids, qid, + got_object_qid, entry); + nqueued--; + got_object_qid_free(qid); + qid = NULL; + } + continue; + } + } + STAILQ_REMOVE_HEAD(ids, entry); nqueued--; - color = (intptr_t)qid->data; + + got_repo_unpin_pack(repo); + pack = NULL; + packidx = NULL; + if (color == COLOR_SKIP) nskip--; @@ -386,25 +521,6 @@ got_pack_paint_commits(int *ncolored, struct got_objec continue; } - /* Pinned pack may have moved to different cache slot. */ - pack = got_repo_get_pinned_pack(repo); - - if (packidx && pack) { - idx = got_packidx_get_object_idx(packidx, &qid->id); - if (idx != -1) { - err = paint_packed_commits(pack, &qid->id, - idx, color, ncolored, &nqueued, &nskip, - ids, keep, drop, skip, repo, - progress_cb, progress_arg, rl, - cancel_cb, cancel_arg); - if (err) - break; - got_object_qid_free(qid); - qid = NULL; - continue; - } - } - switch (color) { case COLOR_KEEP: if (got_object_idset_contains(drop, &qid->id)) { @@ -481,44 +597,6 @@ got_pack_paint_commits(int *ncolored, struct got_objec nqueued++; if (color == COLOR_SKIP) nskip++; - } - } - - if (pack == NULL && (commit->flags & GOT_COMMIT_FLAG_PACKED)) { - if (packidx == NULL) { - err = got_pack_find_pack_for_commit_painting( - &packidx, ids, nqueued, repo); - if (err) - goto done; - } - if (packidx != NULL) { - err = got_pack_cache_pack_for_packidx(&pack, - packidx, repo); - if (err) - goto done; - if (pack->privsep_child == NULL) { - err = got_pack_start_privsep_child( - pack, packidx); - if (err) - goto done; - } - err = got_privsep_init_commit_painting( - pack->privsep_child->ibuf); - if (err) - goto done; - err = send_idset(pack->privsep_child->ibuf, - keep); - if (err) - goto done; - err = send_idset(pack->privsep_child->ibuf, drop); - if (err) - goto done; - err = send_idset(pack->privsep_child->ibuf, skip); - if (err) - goto done; - err = got_repo_pin_pack(repo, packidx, pack); - if (err) - goto done; } } blob - c5ff41385b1af379c173e6dd1e9bb09d87956cc0 blob + 737f2fd3eae3d2bd2a1672cca89b7c82faa58ce8 --- lib/privsep.c +++ lib/privsep.c @@ -3386,18 +3386,10 @@ got_privsep_init_commit_painting(struct imsgbuf *ibuf) } const struct got_error * -got_privsep_send_painting_request(struct imsgbuf *ibuf, int idx, - struct got_object_id *id, intptr_t color) +got_privsep_send_painting_request(struct imsgbuf *ibuf) { - struct got_imsg_commit_painting_request ireq; - - memset(&ireq, 0, sizeof(ireq)); - memcpy(&ireq.id, id, sizeof(ireq.id)); - ireq.idx = idx; - ireq.color = color; - - if (imsg_compose(ibuf, GOT_IMSG_COMMIT_PAINTING_REQUEST, 0, 0, -1, - &ireq, sizeof(ireq)) == -1) + if (imsg_compose(ibuf, GOT_IMSG_COMMIT_PAINTING_REQUEST, 0, 0, -1, + NULL, 0) == -1) return got_error_from_errno("imsg_compose " "COMMIT_PAINTING_REQUEST"); blob - 4547268fa12901fca5606607f1d5db8d8a534ef8 blob + 30fa8f3d2d76c9df5883564176b64116c94fe2dc --- libexec/got-read-pack/got-read-pack.c +++ libexec/got-read-pack/got-read-pack.c @@ -1051,8 +1051,8 @@ recv_object_ids(struct got_object_idset *idset, struct } static const struct got_error * -recv_object_id_queue(struct got_object_id_queue *queue, - struct got_object_idset *queued_ids, struct imsgbuf *ibuf) +recv_object_id_queue(size_t *nids_total, struct got_object_id_queue *queue, + void *data, struct got_object_idset *queued_ids, struct imsgbuf *ibuf) { const struct got_error *err = NULL; int done = 0; @@ -1060,19 +1060,26 @@ recv_object_id_queue(struct got_object_id_queue *queue struct got_object_id *ids; size_t nids, i; + *nids_total = 0; for (;;) { err = got_privsep_recv_object_idlist(&done, &ids, &nids, ibuf); if (err || done) break; + *nids_total += nids; for (i = 0; i < nids; i++) { err = got_object_qid_alloc_partial(&qid); if (err) goto done; memcpy(&qid->id, &ids[i], sizeof(qid->id)); + if (data) + qid->data = data; STAILQ_INSERT_TAIL(queue, qid, entry); - err = got_object_idset_add(queued_ids, &qid->id, NULL); - if (err) - goto done; + if (queued_ids) { + err = got_object_idset_add(queued_ids, + &qid->id, NULL); + if (err) + goto done; + } } free(ids); ids = NULL; @@ -1453,7 +1460,7 @@ enumeration_request(struct imsg *imsg, struct imsgbuf struct got_object_idset *idset, *queued_ids = NULL; int i, idx, have_all_entries = 1; struct enumerated_tree *trees = NULL; - size_t ntrees = 0, nalloc = 16; + size_t ntrees = 0, nalloc = 16, nids = 0; STAILQ_INIT(&commit_ids); @@ -1473,7 +1480,7 @@ enumeration_request(struct imsg *imsg, struct imsgbuf goto done; } - err = recv_object_id_queue(&commit_ids, queued_ids, ibuf); + err = recv_object_id_queue(&nids, &commit_ids, NULL, queued_ids, ibuf); if (err) goto done; @@ -1671,6 +1678,87 @@ queue_commit_id(struct got_object_id_queue *ids, struc } static const struct got_error * +repaint_parent_commits(struct got_object_id *commit_id, int color, + struct got_object_idset *set, struct got_object_idset *skip, + struct got_pack *pack, struct got_packidx *packidx, + struct got_object_cache *objcache) +{ + const struct got_error *err; + struct got_object_id_queue ids; + struct got_object_qid *qid = NULL; + struct got_commit_object *commit; + const struct got_object_id_queue *parents; + int idx; + + STAILQ_INIT(&ids); + + idx = got_packidx_get_object_idx(packidx, commit_id); + if (idx == -1) + return got_error(GOT_ERR_NO_OBJ); + + err = open_commit(&commit, pack, packidx, idx, commit_id, objcache); + if (err) + return err; + + while (commit) { + parents = got_object_commit_get_parent_ids(commit); + if (parents) { + struct got_object_qid *pid; + STAILQ_FOREACH(pid, parents, entry) { + /* + * No need to traverse parents which are + * already in the desired set or are + * marked for skipping already. + */ + if (got_object_idset_contains(set, &pid->id)) + continue; + if (skip != set && + got_object_idset_contains(skip, &pid->id)) + continue; + + err = queue_commit_id(&ids, &pid->id, color); + if (err) + break; + } + } + got_object_commit_close(commit); + commit = NULL; + + qid = STAILQ_FIRST(&ids); + if (qid) { + STAILQ_REMOVE_HEAD(&ids, entry); + if (!got_object_idset_contains(set, &qid->id)) { + err = got_object_idset_add(set, &qid->id, + NULL); + if (err) + break; + } + + idx = got_packidx_get_object_idx(packidx, &qid->id); + if (idx == -1) { + err = got_error(GOT_ERR_NO_OBJ); + break; + } + + err = open_commit(&commit, pack, packidx, idx, + &qid->id, objcache); + if (err) + break; + + got_object_qid_free(qid); + qid = NULL; + } + } + + if (commit) + got_object_commit_close(commit); + if (qid) + got_object_qid_free(qid); + got_object_id_queue_free(&ids); + + return err; +} +static const struct got_error * paint_commits(struct got_object_id_queue *ids, int *nids, struct got_object_idset *keep, struct got_object_idset *drop, struct got_object_idset *skip, struct got_pack *pack, @@ -1725,6 +1813,15 @@ paint_commits(struct got_object_id_queue *ids, int *ni err = paint_commit(qid, COLOR_SKIP); if (err) goto done; + err = got_object_idset_add(skip, &qid->id, + NULL); + if (err) + goto done; + err = repaint_parent_commits(&qid->id, + COLOR_SKIP, skip, skip, pack, packidx, + objcache); + if (err) + goto done; } err = got_object_idset_add(keep, &qid->id, NULL); if (err) @@ -1740,6 +1837,15 @@ paint_commits(struct got_object_id_queue *ids, int *ni err = paint_commit(qid, COLOR_SKIP); if (err) goto done; + err = got_object_idset_add(skip, &qid->id, + NULL); + if (err) + goto done; + err = repaint_parent_commits(&qid->id, + COLOR_SKIP, skip, skip, pack, packidx, + objcache); + if (err) + goto done; } err = got_object_idset_add(drop, &qid->id, NULL); if (err) @@ -1870,25 +1976,48 @@ commit_painting_request(struct imsg *imsg, struct imsg struct got_object_idset *drop, struct got_object_idset *skip) { const struct got_error *err = NULL; - struct got_imsg_commit_painting_request ireq; - struct got_object_id id; size_t datalen; struct got_object_id_queue ids; + size_t nkeep = 0, ndrop = 0, nskip = 0; int nids = 0; + uintptr_t color; STAILQ_INIT(&ids); datalen = imsg->hdr.len - IMSG_HEADER_SIZE; - if (datalen != sizeof(ireq)) + if (datalen != 0) return got_error(GOT_ERR_PRIVSEP_LEN); - memcpy(&ireq, imsg->data, sizeof(ireq)); - memcpy(&id, &ireq.id, sizeof(id)); - err = queue_commit_id(&ids, &id, ireq.color); + color = COLOR_KEEP; + err = recv_object_id_queue(&nkeep, &ids, (void *)color, NULL, ibuf); if (err) - return err; - nids = 1; + goto done; + if (nids + nkeep > INT_MAX) { + err = got_error(GOT_ERR_PRIVSEP_LEN); + goto done; + } + nids += nkeep; + color = COLOR_DROP; + err = recv_object_id_queue(&ndrop, &ids, (void *)color, NULL, ibuf); + if (err) + goto done; + if (nids + ndrop > INT_MAX) { + err = got_error(GOT_ERR_PRIVSEP_LEN); + goto done; + } + nids += ndrop; + + color = COLOR_SKIP; + err = recv_object_id_queue(&nskip, &ids, (void *)color, NULL, ibuf); + if (err) + goto done; + if (nids + nskip > INT_MAX) { + err = got_error(GOT_ERR_PRIVSEP_LEN); + goto done; + } + nids += nskip; + err = paint_commits(&ids, &nids, keep, drop, skip, pack, packidx, ibuf, objcache); if (err) blob - 5038d17342275e2c833422660801b878be6e5f4e blob + c9209d96f9e169e5ea494d92a6af8092d68cdca0 --- regress/cmdline/pack.sh +++ regress/cmdline/pack.sh @@ -771,7 +771,76 @@ test_pack_exclude_via_ancestor_commit() { test_done "$testroot" "$ret" } + +test_pack_exclude_via_ancestor_commit_packed() { + local testroot=`test_init pack_exclude` + local commit0=`git_show_head $testroot/repo` + + # no pack files should exist yet + ls $testroot/repo/.git/objects/pack/ > $testroot/stdout + ret=$? + if [ $ret -ne 0 ]; then + test_done "$testroot" "$ret" + return 1 + fi + echo -n > $testroot/stdout.expected + cmp -s $testroot/stdout.expected $testroot/stdout + ret=$? + if [ $ret -ne 0 ]; then + diff -u $testroot/stdout.expected $testroot/stdout + test_done "$testroot" "$ret" + return 1 + fi + + got checkout $testroot/repo $testroot/wt > /dev/null + ret=$? + if [ $ret -ne 0 ]; then + test_done "$testroot" "$ret" + return 1 + fi + + for i in 1 2 3 4 5 6 7; do + echo a new line >> $testroot/wt/alpha + (cd $testroot/wt && got commit -m "edit alpha" >/dev/null) + done + local parent_commit=`git_show_head $testroot/repo` + + echo a new line >> $testroot/wt/alpha + (cd $testroot/wt && got commit -m "edit alpha" >/dev/null) + got ref -r $testroot/repo -c $parent_commit refs/heads/pleasepackthis + + # pack the repository and remove loose objects + gotadmin cleanup -aq -r $testroot/repo > /dev/null 2> $testroot/stderr + ret=$? + if [ $ret -ne 0 ]; then + echo "gotadmin cleanup failed unexpectedly" >&2 + test_done "$testroot" "$ret" + return 1 + fi + + # Packing the 'pleasepackthis' branch while exluding commits + # reachable via 'master' should result in an empty pack file. + gotadmin pack -a -r $testroot/repo -x master pleasepackthis \ + > $testroot/stdout 2> $testroot/stderr + ret=$? + if [ $ret -eq 0 ]; then + echo "gotadmin pack succeeded unexpectedly" >&2 + test_done "$testroot" "1" + return 1 + fi + + echo "gotadmin: not enough objects to pack" > $testroot/stderr.expected + cmp -s $testroot/stderr.expected $testroot/stderr + ret=$? + if [ $ret -ne 0 ]; then + diff -u $testroot/stderr.expected $testroot/stderr + test_done "$testroot" "$ret" + return 1 + fi + + test_done "$testroot" "$ret" +} test_parseargs "$@" run_test test_pack_all_loose_objects run_test test_pack_exclude @@ -783,3 +852,4 @@ run_test test_pack_all_objects run_test test_pack_bad_ref run_test test_pack_tagged_tag run_test test_pack_exclude_via_ancestor_commit +run_test test_pack_exclude_via_ancestor_commit_packed