Subject: Re: [PATCH 1/2] connected: allow supplying different view of reachable objects

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Fri, 28 Oct 2022 16:54:56 +0200

On Fri, Oct 28 2022, Patrick Steinhardt wrote:

> @@ -125,6 +125,13 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
>  
>  	rev_list_in = xfdopen(rev_list.in, "w");
>  
> +	if (opt->reachable_oids_fn) {
> +		const struct object_id *reachable_oid;
> +		while ((reachable_oid = opt->reachable_oids_fn(opt->reachable_oids_data)) != NULL)
> +			if (fprintf(rev_list_in, "^%s\n", oid_to_hex(reachable_oid)) < 0)
> +				break;

Just a style nit, we tend to avoid != NULL, != 0 etc. comparisons. I see
connected.c has some of that already, but for new code let's just check
truthiness.

Also for such a small scope a shorter variable name helps us stay at the
usual column limits:

	if (opt->reachable_oids_fn) {
		const struct object_id *oid;
		while ((oid = opt->reachable_oids_fn(opt->reachable_oids_data)))
			if (fprintf(rev_list_in, "^%s\n", oid_to_hex(oid)) < 0)
				break;

The fprintf() return value checking seemed a bit odd, not because we
shouldn't do it, but because we usually don't bother. For other
reviewers: We have that form already in connected.c, so at least locally
we're not being diligently careful, only to have it undone by adjacent
code...

Looks good!