commit defb6deb66187bde874fc8c8985fabe46ceec2d2 from: Stefan Sperling via: Thomas Adam date: Thu Jan 30 21:24:53 2025 UTC handle missing parent commits while coloring commits in got-read-pack When a parent is not present in the pack file, handle this situation gracefully rather than erroring out. Fixes a regression introduced with recent got-read-pack coloring changes where commands such as 'got send' could fail with 'got-read-pack: object not found'. commit - c3e15b4b25193f9fad9f3114b1635ac5b2a45f9b commit + defb6deb66187bde874fc8c8985fabe46ceec2d2 blob - e51467833a219053542316cacd34baf290c0c336 blob + 123de0a408db2289586165e6e3b8640549ae5a4e --- lib/pack_create_privsep.c +++ lib/pack_create_privsep.c @@ -256,24 +256,29 @@ recv_painted_commit(void *arg, struct got_object_id *i if (err) return err; } - switch (color) { case COLOR_KEEP: - err = got_object_idset_add(a->keep, id, NULL); - if (err) - return err; - (*a->ncolored)++; + if (!got_object_idset_contains(a->keep, id)) { + err = got_object_idset_add(a->keep, id, NULL); + if (err) + return err; + (*a->ncolored)++; + } break; case COLOR_DROP: - err = got_object_idset_add(a->drop, id, NULL); - if (err) - return err; - (*a->ncolored)++; + if (!got_object_idset_contains(a->drop, id)) { + err = got_object_idset_add(a->drop, id, NULL); + if (err) + return err; + (*a->ncolored)++; + } break; case COLOR_SKIP: - err = got_object_idset_add(a->skip, id, NULL); - if (err) - return err; + if (!got_object_idset_contains(a->skip, id)) { + err = got_object_idset_add(a->skip, id, NULL); + if (err) + return err; + } break; default: /* should not happen */ blob - 30fa8f3d2d76c9df5883564176b64116c94fe2dc blob + 2b6875912a07bb15f3641f9d303b26cce3d0d834 --- libexec/got-read-pack/got-read-pack.c +++ libexec/got-read-pack/got-read-pack.c @@ -1678,45 +1678,48 @@ 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, +repaint_parent_commits(struct got_object_id *commit_id, int commit_idx, + int color, struct got_object_idset *set, struct got_object_idset *skip, + struct got_object_id_queue *ids, int *nids, + struct got_object_id_queue *painted, int *npainted, 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; + struct got_commit_object *commit; + struct got_object_id_queue repaint; - STAILQ_INIT(&ids); + STAILQ_INIT(&repaint); - 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); + err = open_commit(&commit, pack, packidx, commit_idx, commit_id, + objcache); if (err) return err; while (commit) { + struct got_object_qid *pid; + int idx; + parents = got_object_commit_get_parent_ids(commit); if (parents) { - struct got_object_qid *pid; STAILQ_FOREACH(pid, parents, entry) { + idx = got_packidx_get_object_idx(packidx, + &pid->id); /* - * No need to traverse parents which are - * already in the desired set or are - * marked for skipping already. + * No need to traverse parents which are not in + * the pack file, are already in the desired + * set, or are marked for skipping already. */ + if (idx == -1) + continue; if (got_object_idset_contains(set, &pid->id)) continue; - if (skip != set && + if (set != skip && got_object_idset_contains(skip, &pid->id)) continue; - err = queue_commit_id(&ids, &pid->id, color); + err = queue_commit_id(&repaint, &pid->id, color); if (err) break; } @@ -1724,40 +1727,71 @@ repaint_parent_commits(struct got_object_id *commit_id 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); + pid = STAILQ_FIRST(&repaint); + if (pid) { + struct got_object_qid *qid, *tmp; + + err = paint_commit(pid, color); + if (err) + break; + + if (!got_object_idset_contains(set, &pid->id)) { + err = got_object_idset_add(set, &pid->id, NULL); if (err) break; } - idx = got_packidx_get_object_idx(packidx, &qid->id); + STAILQ_REMOVE_HEAD(&repaint, entry); + + /* Insert or replace this commit on the painted list. */ + STAILQ_FOREACH(qid, painted, entry) { + if (got_object_id_cmp(&qid->id, &pid->id) != 0) + continue; + paint_commit(qid, color); + got_object_qid_free(pid); + pid = qid; + break; + } + if (qid == NULL) { + STAILQ_INSERT_TAIL(painted, pid, entry); + (*npainted)++; + } + + /* + * In case this commit is on the caller's list of + * pending commits to traverse, repaint it there. + */ + STAILQ_FOREACH_SAFE(qid, ids, entry, tmp) { + if (got_object_id_cmp(&qid->id, &pid->id) != 0) + continue; + paint_commit(qid, color); + break; + } + + idx = got_packidx_get_object_idx(packidx, &pid->id); if (idx == -1) { + /* + * Should not happen because we only queue + * parents which exist in our pack file. + */ err = got_error(GOT_ERR_NO_OBJ); break; } err = open_commit(&commit, pack, packidx, idx, - &qid->id, objcache); + &pid->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); + got_object_id_queue_free(&repaint); 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, @@ -1804,11 +1838,6 @@ paint_commits(struct got_object_id_queue *ids, int *ni switch (color) { case COLOR_KEEP: - if (got_object_idset_contains(keep, &qid->id)) { - got_object_qid_free(qid); - qid = NULL; - continue; - } if (got_object_idset_contains(drop, &qid->id)) { err = paint_commit(qid, COLOR_SKIP); if (err) @@ -1817,22 +1846,22 @@ paint_commits(struct got_object_id_queue *ids, int *ni NULL); if (err) goto done; - err = repaint_parent_commits(&qid->id, - COLOR_SKIP, skip, skip, pack, packidx, + err = repaint_parent_commits(&qid->id, idx, + COLOR_SKIP, skip, skip, ids, nids, + &painted, &npainted, pack, packidx, objcache); + if (err) + goto done; + break; + } + if (!got_object_idset_contains(keep, &qid->id)) { + err = got_object_idset_add(keep, &qid->id, + NULL); if (err) goto done; } - err = got_object_idset_add(keep, &qid->id, NULL); - if (err) - goto done; break; case COLOR_DROP: - if (got_object_idset_contains(drop, &qid->id)) { - got_object_qid_free(qid); - qid = NULL; - continue; - } if (got_object_idset_contains(keep, &qid->id)) { err = paint_commit(qid, COLOR_SKIP); if (err) @@ -1841,24 +1870,27 @@ paint_commits(struct got_object_id_queue *ids, int *ni NULL); if (err) goto done; - err = repaint_parent_commits(&qid->id, - COLOR_SKIP, skip, skip, pack, packidx, + err = repaint_parent_commits(&qid->id, idx, + COLOR_SKIP, skip, skip, ids, nids, + &painted, &npainted, pack, packidx, objcache); if (err) goto done; + break; } - err = got_object_idset_add(drop, &qid->id, NULL); - if (err) - goto done; - break; - case COLOR_SKIP: - if (!got_object_idset_contains(skip, &qid->id)) { - err = got_object_idset_add(skip, &qid->id, + if (!got_object_idset_contains(drop, &qid->id)) { + err = got_object_idset_add(drop, &qid->id, NULL); if (err) goto done; } break; + case COLOR_SKIP: + err = got_object_idset_add(skip, &qid->id, + NULL); + if (err) + goto done; + break; default: /* should not happen */ err = got_error_fmt(GOT_ERR_NOT_IMPL, blob - c9209d96f9e169e5ea494d92a6af8092d68cdca0 blob + 5ec29348fea250da11738c5675c6cfebe185ab4b --- regress/cmdline/pack.sh +++ regress/cmdline/pack.sh @@ -799,7 +799,21 @@ test_pack_exclude_via_ancestor_commit_packed() { return 1 fi - for i in 1 2 3 4 5 6 7; do + for i in 1 2 3; do + echo a new line >> $testroot/wt/alpha + (cd $testroot/wt && got commit -m "edit alpha" >/dev/null) + done + + # pack the repository's current history + gotadmin pack -q -r $testroot/repo > /dev/null 2> $testroot/stderr + ret=$? + if [ $ret -ne 0 ]; then + echo "gotadmin pack failed unexpectedly" >&2 + test_done "$testroot" "$ret" + return 1 + fi + + for i in 4 5 6; do echo a new line >> $testroot/wt/alpha (cd $testroot/wt && got commit -m "edit alpha" >/dev/null) done @@ -810,11 +824,13 @@ test_pack_exclude_via_ancestor_commit_packed() { 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 + # Pack the next batch of repository history. This test is designed + # to# trigger a code path where got-read-pack cannot find a parent + # commit in its pack file. + gotadmin pack -q -r $testroot/repo > /dev/null 2> $testroot/stderr ret=$? if [ $ret -ne 0 ]; then - echo "gotadmin cleanup failed unexpectedly" >&2 + echo "gotadmin pack failed unexpectedly" >&2 test_done "$testroot" "$ret" return 1 fi