From: Stefan Sperling <stsp@stsp.name>
Subject: Re: simplify send.c for target refnames
To: Josh Rickmar <jrick@zettaport.com>
Cc: gameoftrees@openbsd.org
Date: Wed, 5 Jul 2023 16:19:55 +0200

Looks ok to me overall, thanks! Some remarks inline:

On Wed, Jul 05, 2023 at 09:14:20AM -0400, Josh Rickmar wrote:
> @@ -348,10 +353,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);

Please check for NULL return from strdup() and return got_error_from_errno()
in case it has failed.

strdup() would ideally be called inside insert_ref() but that would
lead to an unfortunate double-allocation in the above case where we
pass the asprintf() result stored in 's'. So putting this burden on
the callers seems better.
Which means this case below needs to handle this, too:

> @@ -382,49 +386,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);

... here ^

>  		}
>  		if (err)
>  			goto done;
>  	}
>  

> @@ -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 *ref = pe->path;

'refname' would be a more suitable name for this variable. Otherwise it
could easily be confused with struct got_reference *ref which we use a lot.
No big deal, just an observation.

> +		if (find_ref(&their_refs, ref) == NULL)
>  			refs_to_send++;