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

[Jeff King <peff@peff.net>]

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

> 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:

> @@ -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.

> -			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