On 2 October 2017 at 07:26, Jeff King <peff@xxxxxxxx> wrote:
> 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.
I'd like to address the feedback on other commits in a re-roll. I will
use that opportunity to try your approach above instead of this patch.
Thanks.
