On Mon, Oct 14, 2024 at 03:02:37PM +0200, Patrick Steinhardt wrote:
> @@ -846,7 +846,10 @@ int reftable_addition_add(struct reftable_addition *add,
> int tab_fd;
>
> reftable_buf_reset(&next_name);
> - format_name(&next_name, add->next_update_index, add->next_update_index);
> +
> + err = format_name(&next_name, add->next_update_index, add->next_update_index);
> + if (err < 0)
> + goto done;
>
> stack_filename(&temp_tab_file_name, add->stack, next_name.buf);
> reftable_buf_addstr(&temp_tab_file_name, ".temp.XXXXXX");
The conversion to store the return value of 'format_name()' here makes
sense. I was going to ask why this call to reftable_buf_addstr() does
not have its own return value checked similarly, but I see that it is
handled specially a few commits later on.
I think that what you wrote here is fine, but there are a couple of
alternatives IMHO that may be worth considering in the future:
- You could do these conversions function by function, where each
patch handles all potential allocation failures.
This generates more patches, but makes each individual patch a
little easier to review in isolation, since the reviewer does not
have to page in and out the context of what different functions do,
etc.
- Alternatively, you could mention something along the lines of "this
step does not make any of these functions entirely resilient against
allocation failures, but future commits will address the remaining
components" to avoid temporary confusion on the reader's part
wondering why only part of the code appears to handle allocation
failures.
Thanks,
Taylor
