Commit 6402fb3cc1e54689a0b577e5958bf75a5a7e798b

Stefan Sperling 2018-09-15T13:21:51

change got_object_get_id() semantics; avoids pointless allocations

diff --git a/got/got.c b/got/got.c
index 35b21af..a506816 100644
--- a/got/got.c
+++ b/got/got.c
@@ -386,19 +386,9 @@ detect_change(int *changed, struct got_object_id *commit_id,
 	}
 
 	id = got_object_get_id(obj);
-	if (id == NULL)
-		return got_error_from_errno();
 	pid = got_object_get_id(pobj);
-	if (pid == NULL) {
-		err = got_error_from_errno();
-		free(id);
-		return err;
-	}
-
 	*changed = (got_object_id_cmp(id, pid) != 0);
 	got_object_close(pobj);
-	free(id);
-	free(pid);
 	return NULL;
 }
 
@@ -601,7 +591,7 @@ cmd_log(int argc, char *argv[])
 			    start_commit);
 			if (error != NULL)
 				return error;
-			id = got_object_get_id(obj);
+			id = got_object_id_dup(got_object_get_id(obj));
 			if (id == NULL)
 				error = got_error_from_errno();
 		}
@@ -652,7 +642,6 @@ cmd_diff(int argc, char *argv[])
 {
 	const struct got_error *error;
 	struct got_repository *repo = NULL;
-	struct got_object_id *id1 = NULL, *id2 = NULL;
 	struct got_object *obj1 = NULL, *obj2 = NULL;
 	char *repo_path = NULL;
 	char *obj_id_str1 = NULL, *obj_id_str2 = NULL;
@@ -698,21 +687,11 @@ cmd_diff(int argc, char *argv[])
 		goto done;
 
 	error = got_object_open_by_id_str(&obj1, repo, obj_id_str1);
-	if (error == NULL) {
-		id1 = got_object_get_id(obj1);
-		if (id1 == NULL)
-			error = got_error_from_errno();
-	}
-	if (error != NULL)
+	if (error)
 		goto done;
 
 	error = got_object_open_by_id_str(&obj2, repo, obj_id_str2);
-	if (error == NULL) {
-		id2 = got_object_get_id(obj2);
-		if (id2 == NULL)
-			error = got_error_from_errno();
-	}
-	if (error != NULL)
+	if (error)
 		goto done;
 
 	if (got_object_get_type(obj1) != got_object_get_type(obj2)) {
@@ -741,10 +720,6 @@ done:
 		got_object_close(obj1);
 	if (obj2)
 		got_object_close(obj2);
-	if (id1)
-		free(id1);
-	if (id2)
-		free(id2);
 	if (repo) {
 		const struct got_error *repo_error;
 		repo_error = got_repo_close(repo);
@@ -837,7 +812,7 @@ cmd_blame(int argc, char *argv[])
 		error = got_object_open_by_id_str(&obj, repo, commit_id_str);
 		if (error != NULL)
 			goto done;
-		commit_id = got_object_get_id(obj);
+		commit_id = got_object_id_dup(got_object_get_id(obj));
 		if (commit_id == NULL)
 			error = got_error_from_errno();
 		got_object_close(obj);
diff --git a/include/got_object.h b/include/got_object.h
index e59ee13..3b8ab2c 100644
--- a/include/got_object.h
+++ b/include/got_object.h
@@ -88,7 +88,8 @@ struct got_object_id *got_object_id_dup(struct got_object_id *);
 
 /*
  * Get a newly allocated copy of an object's ID.
- * The caller should dispose of it with free(3).
+ * The caller must treat the ID as read-only and must not call free(3) on it.
+ * Use got_object_id_dup() to get a writable copy.
  */
 struct got_object_id *got_object_get_id(struct got_object *);
 
diff --git a/lib/object.c b/lib/object.c
index e0a3cb4..f3e4cc8 100644
--- a/lib/object.c
+++ b/lib/object.c
@@ -77,7 +77,7 @@ got_object_id_dup(struct got_object_id *id1)
 struct got_object_id *
 got_object_get_id(struct got_object *obj)
 {
-	return got_object_id_dup(&obj->id);
+	return &obj->id;
 }
 
 const struct got_error *
diff --git a/tog/tog.c b/tog/tog.c
index d7c5c68..1e419f4 100644
--- a/tog/tog.c
+++ b/tog/tog.c
@@ -842,25 +842,10 @@ queue_commits(struct got_commit_graph *graph, struct commit_queue *commits,
 				} else {
 					struct got_object_id *id, *pid;
 					id = got_object_get_id(obj);
-					if (id == NULL) {
-						err = got_error_from_errno();
-						got_object_close(obj);
-						got_object_close(pobj);
-						break;
-					}
 					pid = got_object_get_id(pobj);
-					if (pid == NULL) {
-						err = got_error_from_errno();
-						free(id);
-						got_object_close(obj);
-						got_object_close(pobj);
-						break;
-					}
 					changed =
 					    (got_object_id_cmp(id, pid) != 0);
 					got_object_close(pobj);
-					free(id);
-					free(pid);
 				}
 			}
 			got_object_close(obj);
@@ -1455,7 +1440,7 @@ cmd_log(int argc, char *argv[])
 		struct got_object *obj;
 		error = got_object_open_by_id_str(&obj, repo, start_commit);
 		if (error == NULL) {
-			start_id = got_object_get_id(obj);
+			start_id = got_object_id_dup(got_object_get_id(obj));
 			if (start_id == NULL)
 				error = got_error_from_errno();
 				goto done;
@@ -1607,8 +1592,6 @@ open_diff_view(struct tog_view *view, struct got_object *obj1,
 
 	view->state.diff.id1 = obj1 ? got_object_get_id(obj1) : NULL;
 	view->state.diff.id2 = got_object_get_id(obj2);
-	if (view->state.diff.id2 == NULL)
-		return got_error_from_errno();
 	view->state.diff.f = f;
 	view->state.diff.first_displayed_line = 1;
 	view->state.diff.last_displayed_line = view->nlines;
@@ -1627,8 +1610,6 @@ close_diff_view(struct tog_view *view)
 
 	if (view->state.diff.f && fclose(view->state.diff.f) == EOF)
 		err = got_error_from_errno();
-	free(view->state.diff.id1);
-	free(view->state.diff.id2);
 	return err;
 }
 
@@ -2356,13 +2337,7 @@ input_blame_view(struct tog_view **new_view, struct tog_view **dead_view,
 				got_object_close(pobj);
 				pobj = NULL;
 			}
-			if (id == NULL) {
-				err = got_error_from_errno();
-				break;
-			}
-			err = got_object_qid_alloc(
-			    &s->blamed_commit, id);
-			free(id);
+			err = got_object_qid_alloc(&s->blamed_commit, id);
 			if (err)
 				goto done;
 			SIMPLEQ_INSERT_HEAD(&s->blamed_commits,
@@ -2550,7 +2525,7 @@ cmd_blame(int argc, char *argv[])
 		error = got_object_open_by_id_str(&obj, repo, commit_id_str);
 		if (error != NULL)
 			goto done;
-		commit_id = got_object_get_id(obj);
+		commit_id = got_object_id_dup(got_object_get_id(obj));
 		if (commit_id == NULL)
 			error = got_error_from_errno();
 		got_object_close(obj);
@@ -3097,7 +3072,7 @@ cmd_tree(int argc, char *argv[])
 		struct got_object *obj;
 		error = got_object_open_by_id_str(&obj, repo, commit_id_arg);
 		if (error == NULL) {
-			commit_id = got_object_get_id(obj);
+			commit_id = got_object_id_dup(got_object_get_id(obj));
 			if (commit_id == NULL)
 				error = got_error_from_errno();
 		}