Re: [PATCH 01/11] sha1_file: do not leak `lock_file`

> On Sun, Oct 01, 2017 at 04:56:02PM +0200, Martin Ågren wrote:
>
> The original code is actually a bit confusing/unsafe, as we set the
> "found" flag early and rollback here:
>
>> @@ -481,17 +481,15 @@ void add_to_alternates_file(const char *reference)
>>               strbuf_release(&line);
>>               fclose(in);
>>
>> -             if (found) {
>> -                     rollback_lock_file(lock);
>> -                     lock = NULL;
>> -             }
>> +             if (found)
>> +                     rollback_lock_file(&lock);
>
> and that leaves the "out" filehandle dangling. It's correct because of
> the later "do we still have the lock" check:
>
>> -     if (lock) {
>> +     if (is_lock_file_locked(&lock)) {
>>               fprintf_or_die(out, "%s\n", reference);
>
> but the two spots must remain in sync. If I were writing it from scratch
> I might have bumped "found" to the scope of the whole function, and then
> replaced this final "do we still have the lock" with:
>
>   if (found)
>         rollback_lock_file(&lock);
>   else {
>         fprintf_or_die(out, "%s\n", reference);
>         if (commit_lock_file(&lock))
>         ...etc...
>   }
>
> I don't know if it's worth changing now or not.