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

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 2495 bytes --]

On Wed, Feb 07, 2024 at 05:31:20PM -0500, Jeff King wrote:
> 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).

Indeed we should.

> > +	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).

Yeah, I think this one we should keep. It's also a repeating pattern
that we have in many other places, so it helps lower the mental burden
when it looks similar to all the others.

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

Is the Coverity instance publicly accessible? If not, can you maybe
invite me so that I get a better feeling for them?

I'll send a patch to fix the missing `goto done` above, thanks!

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]