Re: [PATCH 02/11] treewide: prefer lockfiles on the stack
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Oct 2, 2017, 03:37 UTC
- Message-ID
- <xmqqlgkuqkyi.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <f241038eeed0f5f8bb346b394495356c20884a39.1506862824.git.martin.agren@gmail.com>
Martin Ågren <martin.agren@gmail.com> writes:
Show 23 quoted lines
> diff --git a/builtin/diff.c b/builtin/diff.c
> index 7e3ebcea3..91995965d 100644
> --- a/builtin/diff.c
> +++ b/builtin/diff.c
> @@ -203,17 +203,16 @@ static int builtin_diff_combined(struct rev_info *revs,
>
> static void refresh_index_quietly(void)
> {
> - struct lock_file *lock_file;
> + struct lock_file lock_file = LOCK_INIT;
> int fd;
>
> - lock_file = xcalloc(1, sizeof(struct lock_file));
> - fd = hold_locked_index(lock_file, 0);
> + fd = hold_locked_index(&lock_file, 0);
> if (fd < 0)
> return;
> discard_cache();
> read_cache();
> refresh_cache(REFRESH_QUIET|REFRESH_UNMERGED);
> - update_index_if_able(&the_index, lock_file);
> + update_index_if_able(&the_index, &lock_file);
> }This is not a show-stopper for this patch series, but just something I noticed, something that used to be unavoidable in the old world order that requires us to leak lockfiles, but now it becomes more obvious.
update_index_if_able() calls write_lock_index() with the COMMIT_LOCK option, but it does so conditionally. When it does not make the call, the lockfile is left behind to be cleaned up by the atexit() handler, but when it does, it is closed and released.
Perhaps update_index_if_able() needs to be further cleaned up to always release the resource held by the lockfile structure? Its caller cannot differentiate (and more importantly, its caller does not want to care) if the _if_able call actually has updated the index or not and act differently afterwards.