On 2022/09/10 12:16:48 +0200, Stefan Sperling <stsp@stsp.name> wrote:
> On Thu, Sep 08, 2022 at 04:35:16PM +0200, Omar Polo wrote:
> > 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.
> 
> It should be enough to return GOT_ERR_ITER_COMPLETED if there are no
> more IDs, correct? Callers should assume that 'id' is only valid if NULL
> was returned from the most recent call, i.e. no error occurred.

yes, exactly.

> > 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?
> 
> In most cases, checking the returned error for NULL vs non-NULL should
> be good enough. But using a separate variable like 'id_seen' to keep
> such state is fine, too. You could name it 'have_id' instead of 'id_seen'.
> We already use the have_* naming convention elsewhere for such cases.

have_id also reads better :)

the change to blame.c were the only one that made me doubt of this
diff, other callers are improved I think, so maybe after all it isn't
a bad idea.  Here's an updated diff with the better variable naming.

diff /tmp/got
commit - f0f62654dd608d01879094d1f491287f0f7029f1
path + /tmp/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, &regex);
+			err = match_commit(&have_match, &id, commit, &regex);
 			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, &regex);
 			if (have_match == 0 && show_patch) {
-				err = match_patch(&have_match, commit, id,
+				err = match_patch(&have_match, commit, &id,
 				    path, diff_context, repo, &regex,
 				    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, have_id = 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;
+		have_id = 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 (have_id && 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 - fafcec33d71453b09074d2b18e0708b52a197fbe
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)