From: Omar Polo Subject: tweak the commit graph api To: gameoftrees@openbsd.org Date: Thu, 08 Sep 2022 16:35:16 +0200 this is what i had in mind when i was trying to fix the memleak in the commit graph, i.e. make iter_next write the id to a user-provided storage. I'm still a bit unsure though. One the one hand I like many parts of diff below: it makes the management of the id a bit more explicit instead of the current rule of the 'pointer valid up until next call'. On the other hand however returning a pointer (or NULL when reaching the end and returning GOT_ERR_ITER_COMPLETED) was nice. Diff below makes blame_open need an extra variable `id_seen' to keep track of whether `id' is valid or not. meh :/ What do you think? diff /home/op/w/got commit - 41d6d08ab3f441b8bcef117efcf654e2253bf4e6 path + /home/op/w/got blob - 5826d03babb194bb636de184730aa9fb5c2f3690 file + got/got.c --- got/got.c +++ got/got.c @@ -2863,7 +2863,8 @@ check_same_branch(struct got_object_id *commit_id, goto done; for (;;) { - struct got_object_id *id; + struct got_object_id id; + err = got_commit_graph_iter_next(&id, graph, repo, check_cancelled, NULL); if (err) { @@ -2872,13 +2873,11 @@ check_same_branch(struct got_object_id *commit_id, break; } - if (id) { - if (yca_id && got_object_id_cmp(id, yca_id) == 0) - break; - if (got_object_id_cmp(id, commit_id) == 0) { - is_same_branch = 1; - break; - } + if (yca_id && got_object_id_cmp(&id, yca_id) == 0) + break; + if (got_object_id_cmp(&id, commit_id) == 0) { + is_same_branch = 1; + break; } } done: @@ -4225,7 +4224,7 @@ print_commits(struct got_object_id *root_id, struct go if (err) goto done; for (;;) { - struct got_object_id *id; + struct got_object_id id; if (sigint_received || sigpipe_received) break; @@ -4237,10 +4236,8 @@ print_commits(struct got_object_id *root_id, struct go err = NULL; break; } - if (id == NULL) - break; - err = got_object_open_as_commit(&commit, repo, id); + err = got_object_open_as_commit(&commit, repo, &id); if (err) break; @@ -4251,7 +4248,7 @@ print_commits(struct got_object_id *root_id, struct go } if (search_pattern) { - err = match_commit(&have_match, id, commit, ®ex); + err = match_commit(&have_match, &id, commit, ®ex); if (err) { got_object_commit_close(commit); break; @@ -4260,7 +4257,7 @@ print_commits(struct got_object_id *root_id, struct go match_changed_paths(&have_match, &changed_paths, ®ex); if (have_match == 0 && show_patch) { - err = match_patch(&have_match, commit, id, + err = match_patch(&have_match, commit, &id, path, diff_context, repo, ®ex, tmpfile); if (err) @@ -4278,17 +4275,17 @@ print_commits(struct got_object_id *root_id, struct go } if (reverse_display_order) { - err = got_object_qid_alloc(&qid, id); + err = got_object_qid_alloc(&qid, &id); if (err) break; STAILQ_INSERT_HEAD(&reversed_commits, qid, entry); got_object_commit_close(commit); } else { if (one_line) - err = print_commit_oneline(commit, id, + err = print_commit_oneline(commit, &id, repo, refs_idmap); else - err = print_commit(commit, id, repo, path, + err = print_commit(commit, &id, repo, path, show_changed_paths ? &changed_paths : NULL, show_patch, diff_context, refs_idmap, NULL); got_object_commit_close(commit); @@ -4296,7 +4293,7 @@ print_commits(struct got_object_id *root_id, struct go break; } if ((limit && --limit == 0) || - (end_id && got_object_id_cmp(id, end_id) == 0)) + (end_id && got_object_id_cmp(&id, end_id) == 0)) break; TAILQ_FOREACH(pe, &changed_paths, entry) { @@ -9705,10 +9702,8 @@ collect_commits(struct got_object_id_queue *commits, { const struct got_error *err = NULL; struct got_commit_graph *graph = NULL; - struct got_object_id *parent_id = NULL; + struct got_object_id parent_id, commit_id; struct got_object_qid *qid; - struct got_object_id *commit_id = initial_commit_id; - struct got_object_id *tmp = NULL; err = got_commit_graph_open(&graph, "/", 1); if (err) @@ -9718,7 +9713,9 @@ collect_commits(struct got_object_id_queue *commits, check_cancelled, NULL); if (err) goto done; - while (got_object_id_cmp(commit_id, iter_stop_id) != 0) { + + memcpy(&commit_id, initial_commit_id, sizeof(commit_id)); + while (got_object_id_cmp(&commit_id, iter_stop_id) != 0) { err = got_commit_graph_iter_next(&parent_id, graph, repo, check_cancelled, NULL); if (err) { @@ -9730,28 +9727,20 @@ collect_commits(struct got_object_id_queue *commits, } goto done; } else { - err = check_path_prefix(parent_id, commit_id, + err = check_path_prefix(&parent_id, &commit_id, path_prefix, path_prefix_errcode, repo); if (err) goto done; - err = got_object_qid_alloc(&qid, commit_id); + err = got_object_qid_alloc(&qid, &commit_id); if (err) goto done; STAILQ_INSERT_HEAD(commits, qid, entry); - free(tmp); - tmp = got_object_id_dup(parent_id); - if (tmp == NULL) { - err = got_error_from_errno( - "got_object_id_dup"); - goto done; - } - commit_id = tmp; + memcpy(&commit_id, &parent_id, sizeof(commit_id)); } } done: - free(tmp); got_commit_graph_close(graph); return err; } blob - c6db1d281f02e6030400a87b216c3aec949a37f9 file + gotweb/gotweb.c --- gotweb/gotweb.c +++ gotweb/gotweb.c @@ -3540,7 +3540,7 @@ gw_init_header() static const struct got_error * gw_get_commits(struct gw_trans * gw_trans, struct gw_header *header, - int limit, struct got_object_id *id) + int limit, struct got_object_id *iter_start_id) { const struct got_error *error = NULL; struct got_commit_graph *graph = NULL; @@ -3552,12 +3552,14 @@ gw_get_commits(struct gw_trans * gw_trans, struct gw_h if (error) return error; - error = got_commit_graph_iter_start(graph, id, gw_trans->repo, NULL, - NULL); + error = got_commit_graph_iter_start(graph, iter_start_id, + gw_trans->repo, NULL, NULL); if (error) goto err; for (;;) { + struct got_object_id id; + error = got_commit_graph_iter_next(&id, graph, gw_trans->repo, NULL, NULL); if (error) { @@ -3565,15 +3567,13 @@ gw_get_commits(struct gw_trans * gw_trans, struct gw_h error = NULL; goto done; } - if (id == NULL) - goto err; - error = got_object_open_as_commit(&commit, gw_trans->repo, id); + error = got_object_open_as_commit(&commit, gw_trans->repo, &id); if (error) goto err; if (limit == 1 && chk_multi == 0 && gw_trans->gw_conf->got_max_commits_display != 1) { - error = gw_get_commit(gw_trans, header, commit, id); + error = gw_get_commit(gw_trans, header, commit, &id); if (error) goto err; commit_found = 1; @@ -3591,7 +3591,7 @@ gw_get_commits(struct gw_trans * gw_trans, struct gw_h if (error) goto err; - error = gw_get_commit(gw_trans, n_header, commit, id); + error = gw_get_commit(gw_trans, n_header, commit, &id); if (error) goto err; got_ref_list_free(&n_header->refs); blob - 9f9ebd2dc4bfd5836cf4d21e7ae32215f8db04b1 file + gotwebd/got_operations.c --- gotwebd/got_operations.c +++ gotwebd/got_operations.c @@ -434,7 +434,7 @@ got_get_repo_commits(struct request *c, int limit) goto done; for (;;) { - struct got_object_id *next_id; + struct got_object_id next_id; error = got_commit_graph_iter_next(&next_id, graph, repo, NULL, NULL); @@ -444,7 +444,7 @@ got_get_repo_commits(struct request *c, int limit) goto done; } - error = got_object_open_as_commit(&commit, repo, next_id); + error = got_object_open_as_commit(&commit, repo, &next_id); if (error) goto done; @@ -458,7 +458,7 @@ got_get_repo_commits(struct request *c, int limit) goto done; error = got_get_repo_commit(c, repo_commit, commit, - &refs, next_id); + &refs, &next_id); if (error) { gotweb_free_repo_commit(repo_commit); goto done; blob - 617697ddc049b408054512990ba787719cfad80a file + include/got_commit_graph.h --- include/got_commit_graph.h +++ include/got_commit_graph.h @@ -23,7 +23,7 @@ void got_commit_graph_close(struct got_commit_graph *) const struct got_error *got_commit_graph_iter_start( struct got_commit_graph *, struct got_object_id *, struct got_repository *, got_cancel_cb, void *); -const struct got_error *got_commit_graph_iter_next(struct got_object_id **, +const struct got_error *got_commit_graph_iter_next(struct got_object_id *, struct got_commit_graph *, struct got_repository *, got_cancel_cb, void *); const struct got_error *got_commit_graph_intersect(struct got_object_id **, struct got_commit_graph *, struct got_commit_graph *, blob - a884480d0412cfdbd1365bccb5edfed89963a94a file + lib/blame.c --- lib/blame.c +++ lib/blame.c @@ -503,8 +503,8 @@ blame_open(struct got_blame **blamep, const char *path struct got_object_id *obj_id = NULL; struct got_blob_object *blob = NULL; struct got_blame *blame = NULL; - struct got_object_id *id = NULL; - int lineno; + struct got_object_id id; + int lineno, id_seen = 0; struct got_commit_graph *graph = NULL; *blamep = NULL; @@ -584,8 +584,7 @@ blame_open(struct got_blame **blamep, const char *path if (err) goto done; for (;;) { - struct got_object_id *next_id; - err = got_commit_graph_iter_next(&next_id, graph, repo, + err = got_commit_graph_iter_next(&id, graph, repo, cancel_cb, cancel_arg); if (err) { if (err->code == GOT_ERR_ITER_COMPLETED) { @@ -594,35 +593,29 @@ blame_open(struct got_blame **blamep, const char *path } goto done; } - if (next_id) { - free(id); - id = got_object_id_dup(next_id); - if (id == NULL) { - err = got_error_from_errno("got_object_id_dup"); - goto done; - } - err = blame_commit(blame, id, path, repo, cb, arg); - if (err) { - if (err->code == GOT_ERR_ITER_COMPLETED) - err = NULL; - goto done; - } - if (blame->nannotated == blame->nlines) - break; + id_seen = 1; - err = flip_files(blame); - if (err) - goto done; + err = blame_commit(blame, &id, path, repo, cb, arg); + if (err) { + if (err->code == GOT_ERR_ITER_COMPLETED) + err = NULL; + goto done; } + if (blame->nannotated == blame->nlines) + break; + + err = flip_files(blame); + if (err) + goto done; } - if (id && blame->nannotated < blame->nlines) { + if (id_seen && blame->nannotated < blame->nlines) { /* Annotate remaining non-annotated lines with last commit. */ - err = got_object_open_as_commit(&last_commit, repo, id); + err = got_object_open_as_commit(&last_commit, repo, &id); if (err) goto done; for (lineno = 0; lineno < blame->nlines; lineno++) { - err = annotate_line(blame, lineno, last_commit, id, + err = annotate_line(blame, lineno, last_commit, &id, cb, arg); if (err) goto done; @@ -633,7 +626,6 @@ done: if (graph) got_commit_graph_close(graph); free(obj_id); - free(id); if (blob) got_object_blob_close(blob); if (start_commit) blob - 92bc6630d663a353d140360680f6e62605ba1842 file + lib/commit_graph.c --- lib/commit_graph.c +++ lib/commit_graph.c @@ -90,12 +90,6 @@ struct got_commit_graph { * commit timestmap. */ struct got_commit_graph_iter_list iter_list; - - /* - * Temporary storage for the id returned by - * got_commit_graph_iter_next. - */ - struct got_object_id id; }; static const struct got_error * @@ -597,15 +591,13 @@ got_commit_graph_iter_start(struct got_commit_graph *g } const struct got_error * -got_commit_graph_iter_next(struct got_object_id **id, +got_commit_graph_iter_next(struct got_object_id *id, struct got_commit_graph *graph, struct got_repository *repo, got_cancel_cb cancel_cb, void *cancel_arg) { const struct got_error *err = NULL; struct got_commit_graph_node *node; - *id = NULL; - node = TAILQ_FIRST(&graph->iter_list); if (node == NULL) { /* We are done iterating, or iteration was not started. */ @@ -620,14 +612,36 @@ got_commit_graph_iter_next(struct got_object_id **id, return err; } - memcpy(&graph->id, &node->id, sizeof(graph->id)); - *id = &graph->id; + memcpy(id, &node->id, sizeof(*id)); TAILQ_REMOVE(&graph->iter_list, node, entry); free(node); return NULL; } +static const struct got_error * +find_yca_add_id(struct got_object_id **yca_id, struct got_commit_graph *graph, + struct got_object_idset *commit_ids, struct got_repository *repo, + got_cancel_cb cancel_cb, void *cancel_arg) +{ + const struct got_error *err = NULL; + struct got_object_id id; + + err = got_commit_graph_iter_next(&id, graph, repo, cancel_cb, + cancel_arg); + if (err) + return err; + + if (got_object_idset_contains(commit_ids, &id)) { + *yca_id = got_object_id_dup(&id); + if (*yca_id == NULL) + err = got_error_from_errno("got_object_id_dup"); + return err; + } + + return got_object_idset_add(commit_ids, &id, NULL); +} + 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, @@ -664,8 +678,6 @@ got_commit_graph_find_youngest_common_ancestor(struct goto done; for (;;) { - struct got_object_id *id = NULL, *id2 = NULL; - if (cancel_cb) { err = (*cancel_cb)(cancel_arg); if (err) @@ -673,7 +685,7 @@ got_commit_graph_find_youngest_common_ancestor(struct } if (!completed) { - err = got_commit_graph_iter_next(&id, graph, repo, + err = find_yca_add_id(yca_id, graph, commit_ids, repo, cancel_cb, cancel_arg); if (err) { if (err->code != GOT_ERR_ITER_COMPLETED) @@ -681,10 +693,12 @@ got_commit_graph_find_youngest_common_ancestor(struct err = NULL; completed = 1; } + if (*yca_id) + break; } if (!completed2) { - err = got_commit_graph_iter_next(&id2, graph2, repo, + err = find_yca_add_id(yca_id, graph2, commit_ids, repo, cancel_cb, cancel_arg); if (err) { if (err->code != GOT_ERR_ITER_COMPLETED) @@ -692,40 +706,14 @@ got_commit_graph_find_youngest_common_ancestor(struct err = NULL; completed2 = 1; } - } - - if (id) { - if (got_object_idset_contains(commit_ids, id)) { - *yca_id = got_object_id_dup(id); - if (*yca_id) - break; - err = got_error_from_errno("got_object_id_dup"); + if (*yca_id) break; - - } - err = got_object_idset_add(commit_ids, id, NULL); - if (err) - break; } - if (id2) { - if (got_object_idset_contains(commit_ids, id2)) { - *yca_id = got_object_id_dup(id2); - if (*yca_id) - break; - err = got_error_from_errno("got_object_id_dup"); - break; - } - err = got_object_idset_add(commit_ids, id2, NULL); - if (err) - break; - } - if (completed && completed2) { err = got_error(GOT_ERR_ANCESTRY); break; } - } done: got_object_idset_free(commit_ids); blob - 7c4a3a3ec931571a98d0d6e4cf5ece1a0c3a0fc5 file + tog/tog.c --- tog/tog.c +++ tog/tog.c @@ -2171,20 +2171,20 @@ queue_commits(struct tog_log_thread_args *a) * while updating the display. */ do { - struct got_object_id *id; + struct got_object_id id; struct got_commit_object *commit; struct commit_queue_entry *entry; int errcode; err = got_commit_graph_iter_next(&id, a->graph, a->repo, NULL, NULL); - if (err || id == NULL) + if (err) break; - err = got_object_open_as_commit(&commit, a->repo, id); + err = got_object_open_as_commit(&commit, a->repo, &id); if (err) break; - entry = alloc_commit_queue_entry(commit, id); + entry = alloc_commit_queue_entry(commit, &id); if (entry == NULL) { err = got_error_from_errno("alloc_commit_queue_entry"); break; @@ -2204,7 +2204,7 @@ queue_commits(struct tog_log_thread_args *a) if (*a->searching == TOG_SEARCH_FORWARD && !*a->search_next_done) { int have_match; - err = match_commit(&have_match, id, commit, a->regex); + err = match_commit(&have_match, &id, commit, a->regex); if (err) break; if (have_match)