Re: [PATCHv3 6/6] refs.c: enable large transactions

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 24, 2015, 00:38 UTC
Message-ID: <xmqqtwzhnhbl.fsf@gitster.dls.corp.google.com>
In-Reply-To: <CAGZ79kYbCBHVhL+FvKu-yKMinxGCMwGfqG96rSp+0-7frgO2wQ@mail.gmail.com>

Stefan Beller <sbeller@google.com> writes:

> On Fri, Jan 23, 2015 at 4:14 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> yeah that's the goal. Though as we're in one transaction, as soon
> as we have an early exit, the transaction will abort.

An early exit I am talking about is this:

static int write_ref_sha1(struct ref_lock *lock,
	const unsigned char *sha1, const char *logmsg)
{
	static char term = '\n';
	struct object *o;

	if (!lock) {
		errno = EINVAL;
		return -1;
	}
	if (!lock->force_write && !hashcmp(lock->old_sha1, sha1))
		return 0;

It returns 0 and then the transaction side has this call in a loop:

		if (!is_null_sha1(update->new_sha1)) {
			if (write_ref_sha1(update->lock, update->new_sha1,
					   update->msg)) {
				strbuf_addf(err, "Cannot write to the ref lock '%s'.",
					    update->refname);
				ret = TRANSACTION_GENERIC_ERROR;
				goto cleanup;
			}
		}

Show 6 quoted lines

>> If so, shouldn't the write function at least close the file
>> descriptor even when it knows that the $ref.lock already has the
>> correct object name?  The call to close_ref() is never made when the
>> early-return codepath is taken as far as I can see.
>
> The  goto cleanup; will take care of unlocking (and closing fds of) all refs

Yes, if write_ref_sha1() returns with non-zero signaling an error, then the goto will trigger.

But if it short-cuts and returns zero, that goto will not be reached.