fix pack exclusion via an ancestor commit

On Mon, Jan 27, 2025 at 10:42:50AM +0100, Stefan Sperling wrote: > On Mon, Jan 27, 2025 at 10:33:32AM +0100, Stefan Sperling wrote: > > Extend test coverage to cover both code paths in gotd > > I just realized this part (the addition of the > modify_protected_branch_with_deep_history test case) is bogus. > > Because with this packing bug fixed, there will always be an empty > pack file sent to gotd, regardless of the number of parent commits > used by this test. > > So I will drop the regress/gotd changes before commit, and write a > better test case later (which would try to replace the tip commit, > rather than simply trying to remove the tip). Here is a patch on top to ensure that got-read-pack's internal commit coloring loop gets the same fix. This is a bit involved because it turns out that we had a state-mismatch between the main process and got-read-pack, so they would run slightly different traversals. This is fixed here as well. OK? 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). M lib/got_lib_privsep.h | 1+ 9- M lib/pack_create_privsep.c | 141+ 63- M lib/privsep.c | 2+ 10- M libexec/got-read-pack/got-read-pack.c | 144+ 15- M regress/cmdline/pack.sh | 70+ 0- 5 files changed, 358 insertions(+), 97 deletions(-) commit - 9d5dd35582418ff1c51c6ccd42d5f4a5af512486 commit + cd031b0200bec58109add5affdf6090289094b8b blob - de309e2be9e023e1af81a25efd0fb50d70776905 blob + a539f5cf968d5bec802bad6082fe43acfc4dc609 --- lib/got_lib_privsep.h +++ lib/got_lib_privsep.h @@ -346,13 +346,6 @@ 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 { struct got_object_id id; @@ -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 - 05666f0e8981ce94a832dd0fd1ae61c5a4fdcb24 blob + b8de462a868ea019db806d3f84da929a32cc590b --- lib/pack_create_privsep.c +++ lib/pack_create_privsep.c @@ -96,6 +96,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) { @@ -211,6 +244,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 * @@ -255,6 +289,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) @@ -267,9 +303,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, @@ -282,11 +318,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; @@ -299,6 +349,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) @@ -338,6 +389,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, @@ -354,6 +432,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; @@ -363,10 +445,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--; @@ -388,25 +523,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)) { @@ -486,44 +602,6 @@ got_pack_paint_commits(int *ncolored, struct got_objec } } - 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; - } - } - got_object_commit_close(commit); commit = NULL; blob - 2c49d4419a4c395afafe7cca1614b53619d50ed5 blob + 9c7646c870a7ddbf9a22618d09e3549ef5d9cdb4 --- lib/privsep.c +++ lib/privsep.c @@ -3387,18 +3387,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) + NULL, 0) == -1) return got_error_from_errno("imsg_compose " "COMMIT_PAINTING_REQUEST"); blob - 8049481513dd138d1818a8eb293862548487a4dd blob + e63820b191b97c51147aab80a31cbca5d23faca3 --- libexec/got-read-pack/got-read-pack.c +++ libexec/got-read-pack/got-read-pack.c @@ -1054,8 +1054,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; @@ -1063,19 +1063,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; @@ -1456,7 +1463,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); @@ -1476,7 +1483,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; @@ -1674,6 +1681,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, @@ -1728,6 +1816,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) @@ -1743,6 +1840,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) @@ -1873,25 +1979,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 @@ -772,6 +772,75 @@ 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

2025-01-27 09:33 Stefan Sperling:
fix pack exclusion via an ancestor commit
- 2025-01-27 09:42 Stefan Sperling:
  fix pack exclusion via an ancestor commit
- - 2025-01-27 12:56 Stefan Sperling:
    fix pack exclusion via an ancestor commit
  - - 2025-01-30 11:15 Mark Jamsek:
      fix pack exclusion via an ancestor commit
- 2025-01-27 13:17 Mark Jamsek:
  fix pack exclusion via an ancestor commit