Re: [PATCH v4 1/2] cat-file: extract printing batch error message into function

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 3, 2023, 20:26 UTC
Message-ID: <xmqqcz5peg3v.fsf@gitster.g>
In-Reply-To: <20230303191708.77894-2-toon@iotcl.com>

Toon Claes <toon@iotcl.com> writes:

Show 12 quoted lines

> There are two callsites that were formatting an error message in batch
> mode if an object could not be found. We're about to make changes to
> that and to avoid doing that twice, we extract this into a separate
> function.
>
> Signed-off-by: Toon Claes <toon@iotcl.com>
> ...
> +static void batch_print_error(const char *obj_name,
> +			      enum get_oid_result result,
> +			      struct batch_options* opt)
> +{
> +		switch (result) {

The body of this function is indented way too deep. You should lose one HT at the beginning from all its lines.

> +		case MISSING_OBJECT:
> +			printf("%s missing\n", obj_name);
> +			break;

This one expects that obj_name is always usable as a string.

Show 7 quoted lines

> @@ -455,9 +486,7 @@ static void batch_object_write(const char *obj_name,
>  						       &data->oid, &data->info,
>  						       OBJECT_INFO_LOOKUP_REPLACE);
>  		if (ret < 0) {
> -			printf("%s missing\n",
> -			       obj_name ? obj_name : oid_to_hex(&data->oid));
> -			fflush(stdout);

This caller used to be prepared for the case where obj_name is NULL and used the hexadecimal object name stored in data->oid in such a case, but now ...

> +			batch_print_error(obj_name, MISSING_OBJECT, opt);

... the updated caller assumes that obj_name is always safe to feed to printf("%s") above.

Maybe obj_name is always not-NULL when the control reaches this point in which case the new code would be safe, but if that is the case, the proposed log message should explain how that is true to justify this change.

As batch_object_cb() makes a call to batch_object_write() with obj_name set to NULL, I do not think this change is defensible, though.