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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Oct 28, 2022, 14:54 UTC
Message-ID: <221028.86bkpw805n.gmgdl@evledraar.gmail.com>
In-Reply-To: <a32e3d6146dd41af36f525a744d6cc099b42d6fb.1666967670.git.ps@pks.im>

On Fri, Oct 28 2022, Patrick Steinhardt wrote:

Show 9 quoted lines

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