"GOT", but the "O" is a cute, smiling pufferfish. Index | Thread | Search

From:
Josh Rickmar <jrick@zettaport.com>
Subject:
Re: simplify send.c for target refnames
To:
gameoftrees@openbsd.org
Date:
Wed, 5 Jul 2023 10:46:38 -0400

Download raw body.

Thread
  • Josh Rickmar:

    simplify send.c for target refnames

  • On Wed, Jul 05, 2023 at 10:40:43AM -0400, Josh Rickmar wrote:
    > On Wed, Jul 05, 2023 at 08:38:09AM -0400, Josh Rickmar wrote:
    > > 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.
    > 
    > Updated with feedback.  Also changed their_ids to borrow the object
    > ids from their_refs so the cleanup is the same for both arrays.
    
    tb@ found a leak of the asprintf/strdup if insert_sendable_ref fails.
    
    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,47 @@ 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_sendable_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 = NULL;
    +	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 */)
    -		got_ref_close(ref);
    +	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 (err != NULL) {
    +		if (ref)
    +			got_ref_close(ref);
    +		free(id);
    +	}
     	return err;
     }
     
    @@ -210,31 +237,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 +269,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 +307,6 @@ done:
     		}
     		got_ref_close(ref);
     	}
    -	free(my_id);
     	free(remote_refname);
     	return err;
     }
    @@ -320,14 +325,12 @@ 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;
    +	int nours = 0, ntheirs = 0;
     	size_t nalloc_ours = 0, nalloc_theirs = 0;
     	int refs_to_send = 0, refs_to_delete = 0;
     	off_t bytes_sent = 0, bytes_sent_cur = 0;
    @@ -335,37 +338,39 @@ got_send_pack(const char *remote_name, struct got_path
     	uint8_t packsha1[SHA1_DIGEST_LENGTH];
     	int packfd = -1;
     	FILE *delta_cache = NULL;
    +	char *s = NULL;
     
    -	TAILQ_INIT(&refs);
     	TAILQ_INIT(&have_refs);
     	TAILQ_INIT(&their_refs);
     
     	TAILQ_FOREACH(pe, branch_names, entry) {
     		const char *branchname = pe->path;
     		if (strncmp(branchname, "refs/heads/", 11) != 0) {
    -			char *s;
     			if (asprintf(&s, "refs/heads/%s", branchname) == -1) {
     				err = got_error_from_errno("asprintf");
     				goto done;
     			}
    -			err = insert_ref(&refs, s, repo);
    -			free(s);
     		} else {
    -			err = insert_ref(&refs, branchname, repo);
    +			if ((s = strdup(branchname)) == NULL) {
    +				err = got_error_from_errno("strdup");
    +				goto done;
    +			}
     		}
    +		err = insert_sendable_ref(&have_refs, s, repo);
     		if (err)
     			goto done;
    +		s = NULL;
     	}
     
     	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",
    @@ -377,54 +382,27 @@ got_send_pack(const char *remote_name, struct got_path
     	TAILQ_FOREACH(pe, tag_names, entry) {
     		const char *tagname = pe->path;
     		if (strncmp(tagname, "refs/tags/", 10) != 0) {
    -			char *s;
     			if (asprintf(&s, "refs/tags/%s", tagname) == -1) {
     				err = got_error_from_errno("asprintf");
     				goto done;
     			}
    -			err = insert_ref(&refs, s, repo);
    -			free(s);
     		} else {
    -			err = insert_ref(&refs, tagname, repo);
    +			if ((s = strdup(pe->path)) == NULL) {
    +				err = got_error_from_errno("strdup");
    +				goto done;
    +			}
     		}
    +		err = insert_sendable_ref(&have_refs, s, repo);
     		if (err)
     			goto done;
    +		s = NULL;
     	}
     
    -	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 +441,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 +475,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 +489,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,23 +526,14 @@ 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;
     			}
     			/* Exclude any objects reachable via their ID. */
    -			their_ids[ntheirs] = got_object_id_dup(their_id);
    -			if (their_ids[ntheirs] == NULL) {
    -				err = got_error_from_errno("got_object_id_dup");
    -				goto done;
    -			}
    +			their_ids[ntheirs] = their_id;
     			ntheirs++;
     		} else if (!is_tag) {
     			char *remote_refname;
    @@ -609,16 +563,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 *refname = pe->path;
    +		if (find_ref(&their_refs, refname) == 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 +624,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,14 +670,14 @@ 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++)
    -		free(our_ids[i]);
    +	got_pathlist_free(&have_refs, GOT_PATHLIST_FREE_ALL);
    +	got_pathlist_free(&their_refs, GOT_PATHLIST_FREE_ALL);
    +	/*
    +	 * Object ids are owned by have_refs/their_refs and are already freed;
    +	 * Only the arrays must be freed.
    +	 */
     	free(our_ids);
    -	for (i = 0; i < ntheirs; i++)
    -		free(their_ids[i]);
     	free(their_ids);
    +	free(s);
     	return err;
     }
    
    
  • Josh Rickmar:

    simplify send.c for target refnames