got_send_pack in send.c stuffed into a single reflist only to

1) resolve these references multiple times over
2) convert the damn thing to a pathlist in the end because the reflist isn't
   linked into the privsep handlers.

Building the pathlist directly seems like the saner approach.  This also
moves the validation that the reference is 'sendable' (by got policy rules)
to insert_ref.

The reference names are now provided directly as an insert_ref
parameter instead of getting this from the resolved reference (which
we now only do to check that it is not a symlink and is a valid type).
This will simplify the cvg patch as it will allow us to put arbitrary
target reference names into the pathlist, which correspond to object
ids (pe->data).

Passes send.sh regress.

diff /home/jrick/src/got
commit - 51b56d129f9e0504bdf70209c1011a5b077742a6
path + /home/jrick/src/got
blob - 2de21da09b554d15bc73174e05f9df16eaa3e746
file + lib/send.c
--- lib/send.c
+++ lib/send.c
@@ -1,6 +1,7 @@
 /*
  * Copyright (c) 2018, 2019 Ori Bernstein <ori@openbsd.org>
  * Copyright (c) 2021 Stefan Sperling <stsp@openbsd.org>
+ * Copyright (c) 2023 Josh Rickmar <jrick@zettaport.com>
  *
  * Permission to use, copy, modify, and distribute this software for any
  * purpose with or without fee is hereby granted, provided that the above
@@ -143,21 +144,45 @@ insert_ref(struct got_reflist_head *refs, const char *
 }
 
 static const struct got_error *
-insert_ref(struct got_reflist_head *refs, const char *refname,
+insert_ref(struct got_pathlist_head *refs, const char *refname,
     struct got_repository *repo)
 {
 	const struct got_error *err;
 	struct got_reference *ref;
-	struct got_reflist_entry *new;
+	struct got_object_id *id;
+	int obj_type;
 
 	err = got_ref_open(&ref, repo, refname, 0);
 	if (err)
 		return err;
 
-	err = got_reflist_insert(&new, refs, ref, got_ref_cmp_by_name, NULL);
-	if (err || new == NULL /* duplicate */)
+	if (got_ref_is_symbolic(ref)) {
+		err = got_error_fmt(GOT_ERR_BAD_REF_TYPE,
+		    "cannot send symbolic reference %s",
+		    refname);
+		goto done;
+	}
+
+	err = got_ref_resolve(&id, repo, ref);
+	if (err)
+		goto done;
+	err = got_object_get_type(&obj_type, repo, id);
+	if (err)
+		goto done;
+	switch (obj_type) {
+	case GOT_OBJ_TYPE_COMMIT:
+	case GOT_OBJ_TYPE_TAG:
+		break;
+	default:
+		err = got_error_fmt(GOT_ERR_OBJ_TYPE,
+		    "cannot send %s", refname);
+		goto done;
+	}
+
+	err = got_pathlist_insert(NULL, refs, refname, id);
+done:
+	if (ref)
 		got_ref_close(ref);
-
 	return err;
 }
 
@@ -210,31 +235,14 @@ static struct got_reference *
 	return NULL;
 }
 
-static struct got_reference *
-find_ref(struct got_reflist_head *refs, const char *refname)
-{
-	struct got_reflist_entry *re;
-
-	TAILQ_FOREACH(re, refs, entry) {
-		if (got_path_cmp(got_ref_get_name(re->ref), refname,
-		    strlen(got_ref_get_name(re->ref)),
-		    strlen(refname)) == 0) {
-			return re->ref;
-		}
-	}
-
-	return NULL;
-}
-
 static struct got_pathlist_entry *
-find_their_ref(struct got_pathlist_head *their_refs, const char *refname)
+find_ref(struct got_pathlist_head *refs, const char *refname)
 {
 	struct got_pathlist_entry *pe;
 
-	TAILQ_FOREACH(pe, their_refs, entry) {
-		const char *their_refname = pe->path;
-		if (got_path_cmp(their_refname, refname,
-		    strlen(their_refname), strlen(refname)) == 0) {
+	TAILQ_FOREACH(pe, refs, entry) {
+		if (got_path_cmp(pe->path, refname, strlen(pe->path),
+		    strlen(refname)) == 0) {
 			return pe;
 		}
 	}
@@ -259,22 +267,18 @@ update_remote_ref(struct got_reference *my_ref, const 
 }
 
 static const struct got_error *
-update_remote_ref(struct got_reference *my_ref, const char *remote_name,
+update_remote_ref(struct got_pathlist_entry *my_ref, const char *remote_name,
     struct got_repository *repo)
 {
 	const struct got_error *err, *unlock_err;
-	struct got_object_id *my_id;
+	const char *refname = my_ref->path;
+	struct got_object_id *my_id = my_ref->data;
 	struct got_reference *ref = NULL;
 	char *remote_refname = NULL;
 	int ref_locked = 0;
 
-	err = got_ref_resolve(&my_id, repo, my_ref);
+	err = get_remote_refname(&remote_refname, remote_name, refname);
 	if (err)
-		return err;
-
-	err = get_remote_refname(&remote_refname, remote_name,
-	    got_ref_get_name(my_ref));
-	if (err)
 		goto done;
 
 	err = got_ref_open(&ref, repo, remote_refname, 1 /* lock */);
@@ -301,7 +305,6 @@ done:
 		}
 		got_ref_close(ref);
 	}
-	free(my_id);
 	free(remote_refname);
 	return err;
 }
@@ -320,11 +323,9 @@ got_send_pack(const char *remote_name, struct got_path
 	const struct got_error *err;
 	struct imsgbuf sendibuf;
 	pid_t sendpid = -1;
-	struct got_reflist_head refs;
 	struct got_pathlist_head have_refs;
 	struct got_pathlist_head their_refs;
 	struct got_pathlist_entry *pe;
-	struct got_reflist_entry *re;
 	struct got_object_id **our_ids = NULL;
 	struct got_object_id **their_ids = NULL;
 	int i, nours = 0, ntheirs = 0;
@@ -336,7 +337,6 @@ got_send_pack(const char *remote_name, struct got_path
 	int packfd = -1;
 	FILE *delta_cache = NULL;
 
-	TAILQ_INIT(&refs);
 	TAILQ_INIT(&have_refs);
 	TAILQ_INIT(&their_refs);
 
@@ -348,10 +348,9 @@ got_send_pack(const char *remote_name, struct got_path
 				err = got_error_from_errno("asprintf");
 				goto done;
 			}
-			err = insert_ref(&refs, s, repo);
-			free(s);
+			err = insert_ref(&have_refs, s, repo);
 		} else {
-			err = insert_ref(&refs, branchname, repo);
+			err = insert_ref(&have_refs, strdup(branchname), repo);
 		}
 		if (err)
 			goto done;
@@ -359,13 +358,13 @@ got_send_pack(const char *remote_name, struct got_path
 
 	TAILQ_FOREACH(pe, delete_branches, entry) {
 		const char *branchname = pe->path;
-		struct got_reference *ref;
+		struct got_pathlist_entry *ref;
 		if (strncmp(branchname, "refs/heads/", 11) != 0) {
 			err = got_error_fmt(GOT_ERR_SEND_DELETE_REF, "%s",
 			    branchname);
 			goto done;
 		}
-		ref = find_ref(&refs, branchname);
+		ref = find_ref(&have_refs, branchname);
 		if (ref) {
 			err = got_error_fmt(GOT_ERR_SEND_DELETE_REF,
 			    "changes on %s will be sent to server",
@@ -382,49 +381,19 @@ got_send_pack(const char *remote_name, struct got_path
 				err = got_error_from_errno("asprintf");
 				goto done;
 			}
-			err = insert_ref(&refs, s, repo);
-			free(s);
+			err = insert_ref(&have_refs, s, repo);
 		} else {
-			err = insert_ref(&refs, tagname, repo);
+			err = insert_ref(&have_refs, strdup(tagname), repo);
 		}
 		if (err)
 			goto done;
 	}
 
-	if (TAILQ_EMPTY(&refs) && TAILQ_EMPTY(delete_branches)) {
+	if (TAILQ_EMPTY(&have_refs) && TAILQ_EMPTY(delete_branches)) {
 		err = got_error(GOT_ERR_SEND_EMPTY);
 		goto done;
 	}
 
-	TAILQ_FOREACH(re, &refs, entry) {
-		struct got_object_id *id;
-		int obj_type;
-
-		if (got_ref_is_symbolic(re->ref)) {
-			err = got_error_fmt(GOT_ERR_BAD_REF_TYPE,
-			    "cannot send symbolic reference %s",
-			    got_ref_get_name(re->ref));
-			goto done;
-		}
-
-		err = got_ref_resolve(&id, repo, re->ref);
-		if (err)
-			goto done;
-		err = got_object_get_type(&obj_type, repo, id);
-		free(id);
-		if (err)
-			goto done;
-		switch (obj_type) {
-		case GOT_OBJ_TYPE_COMMIT:
-		case GOT_OBJ_TYPE_TAG:
-			break;
-		default:
-			err = got_error_fmt(GOT_ERR_OBJ_TYPE,
-			    "cannot send %s", got_ref_get_name(re->ref));
-			goto done;
-		}
-	}
-
 	packfd = got_opentempfd();
 	if (packfd == -1) {
 		err = got_error_from_errno("got_opentempfd");
@@ -463,22 +432,12 @@ got_send_pack(const char *remote_name, struct got_path
 	}
 
 	/*
-	 * Convert reflist to pathlist since the privsep layer
-	 * is linked into helper programs which lack reference.c.
+	 * Also prepare the array of our object IDs which
+	 * will be needed for generating a pack file.
 	 */
-	TAILQ_FOREACH(re, &refs, entry) {
-		struct got_object_id *id;
-		err = got_ref_resolve(&id, repo, re->ref);
-		if (err)
-			goto done;
-		err = got_pathlist_append(&have_refs,
-		    got_ref_get_name(re->ref), id);
-		if (err)
-			goto done;
-		/*
-		 * Also prepare the array of our object IDs which
-		 * will be needed for generating a pack file.
-		 */
+	TAILQ_FOREACH(pe, &have_refs, entry) {
+		struct got_object_id *id = pe->data;
+
 		err = realloc_ids(&our_ids, &nalloc_ours, nours + 1);
 		if (err)
 			goto done;
@@ -507,7 +466,7 @@ got_send_pack(const char *remote_name, struct got_path
 		struct got_object_id *their_id = pe->data;
 		int have_their_id;
 		struct got_object *obj;
-		struct got_reference *my_ref = NULL;
+		struct got_pathlist_entry *my_ref = NULL;
 		int is_tag = 0;
 
 		/* Don't blindly trust the server to send us valid names. */
@@ -521,23 +480,18 @@ got_send_pack(const char *remote_name, struct got_path
 		 * Otherwise we can still use this reference as a hint to
 		 * avoid uploading any objects the server already has.
 		 */
-		my_ref = find_ref(&refs, refname);
+		my_ref = find_ref(&have_refs, refname);
 		if (my_ref) {
-			struct got_object_id *my_id;
-			err = got_ref_resolve(&my_id, repo, my_ref);
-			if (err)
-				goto done;
+			struct got_object_id *my_id = my_ref->data;
 			if (got_object_id_cmp(my_id, their_id) != 0) {
 				if (!overwrite_refs && is_tag) {
 					err = got_error_fmt(
 					    GOT_ERR_SEND_TAG_EXISTS,
 					    "%s", refname);
-					free(my_id);
 					goto done;
 				}
 				refs_to_send++;
 			}
-			free(my_id);
 		}
 
 		/* Check if their object exists locally. */
@@ -563,14 +517,9 @@ got_send_pack(const char *remote_name, struct got_path
 		if (have_their_id) {
 			/* Enforce linear ancestry if required. */
 			if (!overwrite_refs && my_ref && !is_tag) {
-				struct got_object_id *my_id;
-				err = got_ref_resolve(&my_id, repo, my_ref);
-				if (err)
-					goto done;
+				struct got_object_id *my_id = my_ref->data;
 				err = check_common_ancestry(refname, my_id,
 				    their_id, repo, cancel_cb, cancel_arg);
-				free(my_id);
-				my_id = NULL;
 				if (err)
 					goto done;
 			}
@@ -609,16 +558,16 @@ got_send_pack(const char *remote_name, struct got_path
 	}
 
 	/* Account for any new references we are going to upload. */
-	TAILQ_FOREACH(re, &refs, entry) {
-		if (find_their_ref(&their_refs,
-		    got_ref_get_name(re->ref)) == NULL)
+	TAILQ_FOREACH(pe, &have_refs, entry) {
+		const char *ref = pe->path;
+		if (find_ref(&their_refs, ref) == NULL)
 			refs_to_send++;
 	}
 
 	/* Account for any existing references we are going to delete. */
 	TAILQ_FOREACH(pe, delete_branches, entry) {
 		const char *branchname = pe->path;
-		if (find_their_ref(&their_refs, branchname))
+		if (find_ref(&their_refs, branchname))
 			refs_to_delete++;
 	}
 
@@ -670,12 +619,12 @@ got_send_pack(const char *remote_name, struct got_path
 			goto done;
 		if (refname && got_ref_name_is_valid(refname) && success &&
 		    strncmp(refname, "refs/tags/", 10) != 0) {
-			struct got_reference *my_ref;
+			struct got_pathlist_entry *my_ref;
 			/*
 			 * The server has accepted our changes.
 			 * Update our reference in refs/remotes/ accordingly.
 			 */
-			my_ref = find_ref(&refs, refname);
+			my_ref = find_ref(&have_refs, refname);
 			if (my_ref) {
 				err = update_remote_ref(my_ref, remote_name,
 				    repo);
@@ -716,7 +665,6 @@ done:
 	if (npackfd != -1 && close(npackfd) == -1 && err == NULL)
 		err = got_error_from_errno("close");
 
-	got_ref_list_free(&refs);
 	got_pathlist_free(&have_refs, GOT_PATHLIST_FREE_NONE);
 	got_pathlist_free(&their_refs, GOT_PATHLIST_FREE_NONE);
 	for (i = 0; i < nours; i++)