Re: [PATCH v4 1/2] refs: introduce reftable backend

[Jeff King <peff@peff.net>]

On Wed, Feb 07, 2024 at 08:20:31AM +0100, Patrick Steinhardt wrote:

> +static int write_copy_table(struct reftable_writer *writer, void *cb_data)
> +{
> [...]
> +	/*
> +	 * Create the reflog entry for the newly created branch.
> +	 */
> +	ALLOC_GROW(logs, logs_nr + 1, logs_alloc);
> +	memset(&logs[logs_nr], 0, sizeof(logs[logs_nr]));
> +	fill_reftable_log_record(&logs[logs_nr]);
> +	logs[logs_nr].refname = (char *)arg->newname;
> +	logs[logs_nr].update_index = creation_ts;
> +	logs[logs_nr].value.update.message =
> +		xstrndup(arg->logmsg, arg->refs->write_options.block_size / 2);
> +	logs[logs_nr].value.update.new_hash = old_ref.value.val1;
> +	logs_nr++;
> +
> +	/*
> +	 * In addition to writing the reflog entry for the new branch, we also
> +	 * copy over all log entries from the old reflog. Last but not least,
> +	 * when renaming we also have to delete all the old reflog entries.
> +	 */
> +	ret = reftable_merged_table_seek_log(mt, &it, arg->oldname);
> +	if (ret < 0)
> +		return ret;

Should this last line be "goto done" as is used elsewhere in the
function? Otherwise we are at least leaking the "logs" array (and
possibly some of the other cleanup is important, too).

> +	while (1) {
> +		ret = reftable_iterator_next_log(&it, &old_log);
> +		if (ret < 0)
> +			goto done;
> +		if (ret > 0 || strcmp(old_log.refname, arg->oldname)) {
> +			ret = 0;
> +			break;
> +		}

This "ret = 0" doesn't have any effect. We break out of the loop, and
then...

> +	}
> +
> +	ret = reftable_writer_add_logs(writer, logs, logs_nr);
> +	if (ret < 0)
> +		goto done;

...the first thing we do is write over it. I dunno if it's worth keeping
as a maintenance precaution, though (if the code after the loop changed
to omit that assignment, then setting "ret" would become important).

Both were noticed by Coverity (along with several other false
positives).

-Peff