Re: [PATCH v2 3/3] bundle: remove "ref_list" in favor of string-list.c API

From: Jeff King <peff@peff.net>
Date: Jun 24, 2021, 17:11 UTC
Message-ID: <YNS8vzPc1KyRaSR5@coredump.intra.peff.net>
In-Reply-To: <patch-3.3-9d9cb5aaf9e-20210621T151357Z-avarab@gmail.com>

On Mon, Jun 21, 2021 at 05:16:14PM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 9 quoted lines

> Move away from the "struct ref_list" in bundle.c in favor of the
> almost identical string-list.c API.
> 
> That API fits this use-case perfectly, but did not exist in its
> current form when this code was added in 2e0afafebd (Add git-bundle:
> move objects and references by archive, 2007-02-22), with hindsight we
> could have used the path-list API, which later got renamed to
> string-list. See 8fd2cb4069 (Extract helper bits from
> c-merge-recursive work, 2006-07-25)

I think this is a good direction, and I didn't see any errors in the code. It's slightly sad that we end up with more lines than we started with, but I think that's mostly because you're actually freeing the memory now.

Two small nitpicks:

Show 16 quoted lines

> @@ -103,19 +94,22 @@ static int parse_bundle_header(int fd, struct bundle_header *header,
>  		 * Prerequisites have object name that is optionally
>  		 * followed by SP and subject line.
>  		 */
> -		if (parse_oid_hex_algop(buf.buf, &oid, &p, header->hash_algo) ||
> +		oid = xmalloc(sizeof(struct object_id));
> +		if (parse_oid_hex_algop(buf.buf, oid, &p, header->hash_algo) ||
>  		    (*p && !isspace(*p)) ||
>  		    (!is_prereq && !*p)) {
>  			if (report_path)
>  				error(_("unrecognized header: %s%s (%d)"),
>  				      (is_prereq ? "-" : ""), buf.buf, (int)buf.len);
>  			status = -1;
> +			free(oid);
>  			break;
>  		} else {

This would be slightly simpler if you kept a local "struct object_id", and then called:

  string_list_append(list, string)->util = oiddup(&oid);

later when you know you want to save it. And then you don't have to worry about the extra cleanup here. That does require an extra oidcpy() under the hood, but I suspect that is lost in the noise.

I'm OK with it either way.

Show 9 quoted lines

> -			if (is_prereq)
> -				add_to_ref_list(&oid, "", &header->prerequisites);
> -			else
> -				add_to_ref_list(&oid, p + 1, &header->references);
> +			const char *string = is_prereq ? "" : p + 1;
> +			struct string_list *list = is_prereq
> +				? &header->prerequisites
> +				: &header->references;
> +			string_list_append(list, string)->util = oid;

I'm usually a big fan of the ternary operator, and using variable indirection to make it clear that we always call a function. But here I think it makes things more confusing. The two sides of the if/else are sufficiently simple that it's easy to see they both make the same function call. And because there are two variables, we check is_prereq twice, making it much harder to see the two cases.

I.e., I think:

  if (is_prereq)
          string_list_append(&header->prerequisites, "")->util = oid;
  else
          string_list_append(&header->references, p + 1)->util = oid;

is much more obvious.

-Peff