Patrick Steinhardt <ps@pks.im> writes:
>> +static int create_and_commit_symref(struct files_ref_store *refs,
>> +				    struct ref_lock *lock, const char *refname,
>> +				    const char *target, const char *logmsg)
>> +{
>> +	int ret;
>> +
>>  	if (prefer_symlink_refs && !create_ref_symlink(lock, target)) {
>>  		update_symref_reflog(refs, lock, refname, target, logmsg);
>>  		return 0;
>>  	}
>>
>> -	if (!fdopen_lock_file(&lock->lk, "w"))
>> -		return error("unable to fdopen %s: %s",
>> -			     get_lock_file_path(&lock->lk), strerror(errno));
>> +	ret = create_symref_lock(refs, lock, refname, target);
>> +	if (!ret) {
>> +		update_symref_reflog(refs, lock, refname, target, logmsg);
>
> I feel like the resulting code here is a bit hard to read because the
> successful path is now nested into the condition. This does not really
> conform to our typical coding style. Exiting early in case the function
> returns an error would be easier to read.

Agreed, will modify this to exit early.

>> -	update_symref_reflog(refs, lock, refname, target, logmsg);
>> +		if (commit_ref(lock) < 0)
>> +			return error("unable to write symref for %s: %s", refname,
>> +				     strerror(errno));
>> +	}
>>
>> -	/* no error check; commit_ref will check ferror */
>> -	fprintf(get_lock_file_fp(&lock->lk), "ref: %s\n", target);
>> -	if (commit_ref(lock) < 0)
>> -		return error("unable to write symref for %s: %s", refname,
>> -			     strerror(errno));
>>  	return 0;
>
> Also, is it correct to `return 0` here in case `create_symref_lock()`
> returns an error? If so it certainly requires an in-code comment to
> explain what is going on. If this is a bug I feel like we have
> identified a test gap that we might want to plug.
>

It's wrong, we should definitely be catching and returning that error.
Regarding fixing the test gap, it is kinda hard to do so, since this
is capturing a filesystem error. It would be easier to do so in the
upcoming commits where I could possibly do:

1. Start transaction
2. Add symref creation to transaction
3. Before preparing the transaction, manually create the lock file.
4. This should hit this error.

I'll add something in corresponding commit.