Stefan Sperling <stsp@stsp.name> writes:

> On Sun, Mar 24, 2024 at 06:48:05PM -0500, Kyle Ackerman wrote:
>> Here is alternative diff. Rather than checking after every message
>> received, we can just allocate the got_object_id once we receive a
>> GOT_IMSG_COMMIT_TRAVERSAL_DONE. This is here in case checking to see if
>> changed_commit_id is not null after each imsg is ugly.`
>> 
>> diff /home/kyle/src/got
>> commit - 39910b637a9a53cc48b0c63766da691dec0af593
>> path + /home/kyle/src/got
>> blob - 857c09bbb3a5d9425176ed93727a912b2242aac6
>> file + lib/privsep.c
>> --- lib/privsep.c
>> +++ lib/privsep.c
>> @@ -2730,8 +2730,7 @@ got_privsep_recv_traversed_commits(struct got_commit_o
>>  
>>  				/* The last commit may contain a change. */
>>  				if (i == icommits->ncommits - 1) {
>> -					*changed_commit_id =
>> -					    got_object_id_dup(&qid->id);
>> +					*changed_commit_id = &qid->id;
>>  					if (*changed_commit_id == NULL) {
>
> The pointer can no longer be NULL here.
>
>>  						err = got_error_from_errno(
>>  						    "got_object_id_dup");
>> @@ -2749,6 +2748,7 @@ got_privsep_recv_traversed_commits(struct got_commit_o
>>  			    datalen, ibuf);
>>  			break;
>>  		case GOT_IMSG_COMMIT_TRAVERSAL_DONE:
>> +			*changed_commit_id = got_object_id_dup(*changed_commit_id);
>
> And the above allocation failure check would now need to be here.
>
> Howveer, this whole mechanism is a bit silly. The caller receives the
> full list of traversed commits, and *changed_commit points at a newly
> allocated copy of the last commit on this list.
>
> Couldn't the caller use STAILQ_LAST to find the same commit, so we
> can get rid of the unneccesary copy and **changed_commit_id in this
> function entirely?
>
>>  			done = 1;
>>  			break;
>>  		default:
>> 
>> 

Here is a diff with the rewritten logic.

diff /home/kyle/src/got
commit - 39910b637a9a53cc48b0c63766da691dec0af593
path + /home/kyle/src/got
blob - d517f36f2ccbda4c48348016a20e6d6e87fbd1a4
file + lib/got_lib_privsep.h
--- lib/got_lib_privsep.h
+++ lib/got_lib_privsep.h
@@ -787,8 +787,8 @@ const struct got_error *got_privsep_recv_gotconfig_rem
 const struct got_error *got_privsep_send_commit_traversal_request(
     struct imsgbuf *, struct got_object_id *, int, const char *);
 const struct got_error *got_privsep_recv_traversed_commits(
-    struct got_commit_object **, struct got_object_id **,
-    struct got_object_id_queue *, struct imsgbuf *);
+    struct got_commit_object **, struct got_object_id_queue *,
+    struct imsgbuf *);
 const struct got_error *got_privsep_send_enumerated_tree(size_t *,
     struct imsgbuf *, struct got_object_id *, const char *,
     struct got_parsed_tree_entry *, int);
blob - c956fa4a7551d6ffe10a3bd5888209dde0a148b8
file + lib/object_open_privsep.c
--- lib/object_open_privsep.c
+++ lib/object_open_privsep.c
@@ -24,6 +24,7 @@
 
 #include <errno.h>
 #include <imsg.h>
+#include <stddef.h>
 #include <stdio.h>
 #include <stdint.h>
 #include <stdlib.h>
@@ -1214,7 +1215,7 @@ got_traverse_packed_commits(struct got_object_id_queue
 	struct got_packidx *packidx = NULL;
 	char *path_packfile = NULL;
 	struct got_commit_object *changed_commit = NULL;
-	struct got_object_id *changed_commit_id = NULL;
+	struct got_object_qid *changed_commit_qid = NULL;
 	int idx;
 
 	err = got_repo_search_packidx(&packidx, &idx, repo, commit_id);
@@ -1248,7 +1249,7 @@ got_traverse_packed_commits(struct got_object_id_queue
 		goto done;
 
 	err = got_privsep_recv_traversed_commits(&changed_commit,
-	    &changed_commit_id, traversed_commits, pack->privsep_child->ibuf);
+	    traversed_commits, pack->privsep_child->ibuf);
 	if (err)
 		goto done;
 
@@ -1258,13 +1259,13 @@ got_traverse_packed_commits(struct got_object_id_queue
 		 * This commit might be opened again soon.
 		 */
 		changed_commit->refcnt++;
-		err = got_repo_cache_commit(repo, changed_commit_id,
+		changed_commit_qid = STAILQ_LAST(traversed_commits, got_object_qid, entry);
+		err = got_repo_cache_commit(repo, &changed_commit_qid->id,
 		    changed_commit);
 		got_object_commit_close(changed_commit);
 	}
 done:
 	free(path_packfile);
-	free(changed_commit_id);
 	return err;
 }
 
blob - 857c09bbb3a5d9425176ed93727a912b2242aac6
file + lib/privsep.c
--- lib/privsep.c
+++ lib/privsep.c
@@ -2691,7 +2691,6 @@ got_privsep_send_commit_traversal_request(struct imsgb
 
 const struct got_error *
 got_privsep_recv_traversed_commits(struct got_commit_object **changed_commit,
-    struct got_object_id **changed_commit_id,
     struct got_object_id_queue *commit_ids, struct imsgbuf *ibuf)
 {
 	const struct got_error *err = NULL;
@@ -2702,7 +2701,6 @@ got_privsep_recv_traversed_commits(struct got_commit_o
 	int i, done = 0;
 
 	*changed_commit = NULL;
-	*changed_commit_id = NULL;
 
 	while (!done) {
 		err = got_privsep_recv_imsg(&imsg, ibuf, 0);
@@ -2728,23 +2726,9 @@ got_privsep_recv_traversed_commits(struct got_commit_o
 				memcpy(&qid->id, &ids[i], sizeof(ids[i]));
 				STAILQ_INSERT_TAIL(commit_ids, qid, entry);
 
-				/* The last commit may contain a change. */
-				if (i == icommits->ncommits - 1) {
-					*changed_commit_id =
-					    got_object_id_dup(&qid->id);
-					if (*changed_commit_id == NULL) {
-						err = got_error_from_errno(
-						    "got_object_id_dup");
-						break;
-					}
-				}
 			}
 			break;
 		case GOT_IMSG_COMMIT:
-			if (*changed_commit_id == NULL) {
-				err = got_error(GOT_ERR_PRIVSEP_MSG);
-				break;
-			}
 			err = get_commit_from_imsg(changed_commit, &imsg,
 			    datalen, ibuf);
 			break;

Thanks for your guidance,
Kyle