Patrick Steinhardt <ps@xxxxxx> 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.
Attachment:
signature.asc
Description: PGP signature
