From: James Cook Subject: [patch] simplify ancestry checks To: gameoftrees@openbsd.org Date: Sun, 21 May 2023 03:09:33 +0000 As far as I can tell, the below patch doesn't change any behaviour except maybe to save a bit of time. I added a comment to got_commit_graph_find_youngest_ancestor mostly to make it easier to verify that yca_id can't be NULL when the new line 11313 is reached. The calls to check_same_branch that I replaced were equivalent to just checking whether one commit was the youngest common ancestor of both. It is complicated a bit by whether non-linear ancestors were considered, i.e. the first_parent_traversal argument to got_commit_graph_find_youngest_common_ancestor, but I believe it all works out: - The check in cmd_rebase only considers linear history, before and after this change. - The check in cmd_merge considers full history, both before and after this change. (The reason it considered full history before this change is that one of the first things check_same_branch did was to compare commit_id to yca_id, and yca_id was based on full history.) I was working on making "got merge" fast-forward when possible as discussed in the other thread when I noticed this apparently unneeded complexity. If this change is correct, I think it makes sense to do it first as a separate change, because it's already tricky enough to reason about on its own. -- James diff b156a14e478c9485a080a24f237f70b6c8984182 refs/heads/mg_simplify_br commit - b156a14e478c9485a080a24f237f70b6c8984182 commit + 8b32aba2f499aa19f79a72f5bc04a327c6ff5c26 blob - 8d34fc2c0463ba9b0482b97a427cd03c82b875d8 blob + 123dedc51cae9b07a29394e9816496121697cf7a --- got/got.c +++ got/got.c @@ -2902,26 +2902,18 @@ check_same_branch(struct got_object_id *commit_id, static const struct got_error * check_same_branch(struct got_object_id *commit_id, - struct got_reference *head_ref, struct got_object_id *yca_id, - struct got_repository *repo) + struct got_reference *head_ref, struct got_repository *repo) { const struct got_error *err = NULL; struct got_commit_graph *graph = NULL; struct got_object_id *head_commit_id = NULL; - int is_same_branch = 0; err = got_ref_resolve(&head_commit_id, repo, head_ref); if (err) goto done; - if (got_object_id_cmp(head_commit_id, commit_id) == 0) { - is_same_branch = 1; + if (got_object_id_cmp(head_commit_id, commit_id) == 0) goto done; - } - if (yca_id && got_object_id_cmp(commit_id, yca_id) == 0) { - is_same_branch = 1; - goto done; - } err = got_commit_graph_open(&graph, "/", 1); if (err) @@ -2939,23 +2931,17 @@ check_same_branch(struct got_object_id *commit_id, check_cancelled, NULL); if (err) { if (err->code == GOT_ERR_ITER_COMPLETED) - err = NULL; + err = got_error(GOT_ERR_ANCESTRY); break; } - if (yca_id && got_object_id_cmp(&id, yca_id) == 0) + if (got_object_id_cmp(&id, commit_id) == 0) break; - if (got_object_id_cmp(&id, commit_id) == 0) { - is_same_branch = 1; - break; - } } done: if (graph) got_commit_graph_close(graph); free(head_commit_id); - if (!err && !is_same_branch) - err = got_error(GOT_ERR_ANCESTRY); return err; } @@ -3158,7 +3144,7 @@ cmd_checkout(int argc, char *argv[]) } goto done; } - error = check_same_branch(commit_id, head_ref, NULL, repo); + error = check_same_branch(commit_id, head_ref, repo); if (error) { if (error->code == GOT_ERR_ANCESTRY) { error = checkout_ancestry_error( @@ -3614,7 +3600,7 @@ cmd_update(int argc, char *argv[]) free(head_commit_id); if (error != NULL) goto done; - error = check_same_branch(commit_id, head_ref, NULL, repo); + error = check_same_branch(commit_id, head_ref, repo); if (error) goto done; error = switch_head_ref(head_ref, commit_id, worktree, repo); @@ -3628,7 +3614,7 @@ cmd_update(int argc, char *argv[]) error = got_error(GOT_ERR_BRANCH_MOVED); goto done; } - error = check_same_branch(commit_id, head_ref, NULL, repo); + error = check_same_branch(commit_id, head_ref, repo); if (error) goto done; } @@ -11324,12 +11310,7 @@ cmd_rebase(int argc, char *argv[]) goto done; } - error = check_same_branch(base_commit_id, branch, yca_id, repo); - if (error) { - if (error->code != GOT_ERR_ANCESTRY) - goto done; - error = NULL; - } else { + if (got_object_id_cmp(base_commit_id, yca_id) == 0) { struct got_pathlist_head paths; printf("%s is already based on %s\n", got_ref_get_name(branch), @@ -13323,24 +13304,16 @@ cmd_merge(int argc, char *argv[]) GOT_ERR_MERGE_PATH, repo); if (error) goto done; - if (yca_id) { - error = check_same_branch(wt_branch_tip, branch, - yca_id, repo); - if (error) { - if (error->code != GOT_ERR_ANCESTRY) - goto done; - error = NULL; - } else { - static char msg[512]; - snprintf(msg, sizeof(msg), - "cannot create a merge commit because " - "%s is based on %s; %s can be integrated " - "with 'got integrate' instead", branch_name, - got_worktree_get_head_ref_name(worktree), - branch_name); - error = got_error_msg(GOT_ERR_SAME_BRANCH, msg); - goto done; - } + if (yca_id && got_object_id_cmp(wt_branch_tip, yca_id) == 0) { + static char msg[512]; + snprintf(msg, sizeof(msg), + "cannot create a merge commit because %s is based " + "on %s; %s can be integrated with 'got integrate' " + "instead", branch_name, + got_worktree_get_head_ref_name(worktree), + branch_name); + error = got_error_msg(GOT_ERR_SAME_BRANCH, msg); + goto done; } error = got_worktree_merge_prepare(&fileindex, worktree, branch, repo); blob - 45ed91c002a5e8a63378941757461aee195a0547 blob + f25ec1e90750a30eab3d01e03d67faabeaefb27b --- lib/commit_graph.c +++ lib/commit_graph.c @@ -643,6 +643,12 @@ const struct got_error * return got_object_idset_add(commit_ids, &id, NULL); } +/* + * Sets *yca_id to the last commit that can be reached from both commit_id and + * commit_id2. Returns got_error(GOT_ERR_ANCESTRY) if there are none. + * + * If first_parent_traversal is nonzero, only linear history is considered. + */ const struct got_error * got_commit_graph_find_youngest_common_ancestor(struct got_object_id **yca_id, struct got_object_id *commit_id, struct got_object_id *commit_id2,