Toon Claes <toon@xxxxxxxxx> writes:
> 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@xxxxxxxxx>
> ...
> +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.
> @@ -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.
