On Sun, Oct 01, 2017 at 04:56:02PM +0200, Martin Ågren wrote:
> There is no longer any need to allocate and leak a `struct lock_file`.
> Initialize it on the stack instead.
>
> Instead of setting `lock = NULL` to signal that we have already rolled
> back, and that we should not do any more work, check with
> `is_lock_file_locked()`. Since we take the lock with
> `LOCK_DIE_ON_ERROR`, it evaluates to false exactly when we have called
> `rollback_lock_file()`.
This looks correct (and is a good direction, as it drops a leak).
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.
-Peff
