[PATCH 4/5] lock_file: make function-local locks non-static

[PATCH 4/5] lock_file: make function-local locks non-static

[Martin Ågren]

These `struct lock_file`s are local to their respective functions and we
can drop their staticness.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 apply.c                | 2 +-
 builtin/describe.c     | 2 +-
 builtin/difftool.c     | 2 +-
 builtin/gc.c           | 2 +-
 builtin/merge.c        | 4 ++--
 builtin/receive-pack.c | 2 +-
 bundle.c               | 2 +-
 fast-import.c          | 2 +-
 refs/files-backend.c   | 2 +-
 shallow.c              | 2 +-
 10 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/apply.c b/apply.c
index 7e5792c996..07b14d1127 100644
--- a/apply.c
+++ b/apply.c
@@ -4058,7 +4058,7 @@ static int build_fake_ancestor(struct apply_state *state, struct patch *list)
 {
 	struct patch *patch;
 	struct index_state result = { NULL };
-	static struct lock_file lock;
+	struct lock_file lock = LOCK_INIT;
 	int res;
 
 	/* Once we start supporting the reverse patch, it may be
diff --git a/builtin/describe.c b/builtin/describe.c
index b5afc45846..8bd95cf371 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -612,7 +612,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 				suffix = broken;
 			}
 		} else if (dirty) {
-			static struct lock_file index_lock;
+			struct lock_file index_lock = LOCK_INIT;
 			struct rev_info revs;
 			struct argv_array args = ARGV_ARRAY_INIT;
 			int fd, result;
diff --git a/builtin/difftool.c b/builtin/difftool.c
index aad0e073ee..162806f238 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -610,7 +610,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 			continue;
 
 		if (!indices_loaded) {
-			static struct lock_file lock;
+			struct lock_file lock = LOCK_INIT;
 			strbuf_reset(&buf);
 			strbuf_addf(&buf, "%s/wtindex", tmpdir);
 			if (hold_lock_file_for_update(&lock, buf.buf, 0) < 0 ||
diff --git a/builtin/gc.c b/builtin/gc.c
index 3e67124eaa..274660d9dc 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -234,7 +234,7 @@ static int need_to_gc(void)
 /* return NULL on success, else hostname running the gc */
 static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
 {
-	static struct lock_file lock;
+	struct lock_file lock = LOCK_INIT;
 	char my_host[HOST_NAME_MAX + 1];
 	struct strbuf sb = STRBUF_INIT;
 	struct stat st;
diff --git a/builtin/merge.c b/builtin/merge.c
index 9db5a2cf16..de62b2c5c6 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -647,7 +647,7 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
 			      struct commit_list *remoteheads,
 			      struct commit *head)
 {
-	static struct lock_file lock;
+	struct lock_file lock = LOCK_INIT;
 	const char *head_arg = "HEAD";
 
 	hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
@@ -805,7 +805,7 @@ static int merge_trivial(struct commit *head, struct commit_list *remoteheads)
 {
 	struct object_id result_tree, result_commit;
 	struct commit_list *parents, **pptr = &parents;
-	static struct lock_file lock;
+	struct lock_file lock = LOCK_INIT;
 
 	hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
 	refresh_cache(REFRESH_QUIET);
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 4b68a28e92..d3cf36cef5 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -876,7 +876,7 @@ static void refuse_unconfigured_deny_delete_current(void)
 static int command_singleton_iterator(void *cb_data, struct object_id *oid);
 static int update_shallow_ref(struct command *cmd, struct shallow_info *si)
 {
-	static struct lock_file shallow_lock;
+	struct lock_file shallow_lock = LOCK_INIT;
 	struct oid_array extra = OID_ARRAY_INIT;
 	struct check_connected_options opt = CHECK_CONNECTED_INIT;
 	uint32_t mask = 1 << (cmd->index % 32);
diff --git a/bundle.c b/bundle.c
index 902c9b5448..160bbfdc64 100644
--- a/bundle.c
+++ b/bundle.c
@@ -409,7 +409,7 @@ static int write_bundle_refs(int bundle_fd, struct rev_info *revs)
 int create_bundle(struct bundle_header *header, const char *path,
 		  int argc, const char **argv)
 {
-	static struct lock_file lock;
+	struct lock_file lock = LOCK_INIT;
 	int bundle_fd = -1;
 	int bundle_to_stdout;
 	int ref_count = 0;
diff --git a/fast-import.c b/fast-import.c
index 05d1079d23..09443c1a9d 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1817,7 +1817,7 @@ static void dump_marks_helper(FILE *f,
 
 static void dump_marks(void)
 {
-	static struct lock_file mark_lock;
+	struct lock_file mark_lock = LOCK_INIT;
 	FILE *f;
 
 	if (!export_marks_file || (import_marks_file && !import_marks_file_done))
diff --git a/refs/files-backend.c b/refs/files-backend.c
index a92a2aa821..7b2da7b8c9 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2995,7 +2995,7 @@ static int files_reflog_expire(struct ref_store *ref_store,
 {
 	struct files_ref_store *refs =
 		files_downcast(ref_store, REF_STORE_WRITE, "reflog_expire");
-	static struct lock_file reflog_lock;
+	struct lock_file reflog_lock = LOCK_INIT;
 	struct expire_reflog_cb cb;
 	struct ref_lock *lock;
 	struct strbuf log_file_sb = STRBUF_INIT;
diff --git a/shallow.c b/shallow.c
index df4d44ea7a..85313619ea 100644
--- a/shallow.c
+++ b/shallow.c
@@ -353,7 +353,7 @@ void advertise_shallow_grafts(int fd)
  */
 void prune_shallow(int show_only)
 {
-	static struct lock_file shallow_lock;
+	struct lock_file shallow_lock = LOCK_INIT;
 	struct strbuf sb = STRBUF_INIT;
 	int fd;
 
-- 
2.17.0.411.g9fd64c8e46

Re: [PATCH 4/5] lock_file: make function-local locks non-static

[Duy Nguyen]

On Sun, May 6, 2018 at 4:10 PM, Martin Ågren <martin.agren@gmail.com> wrote:
> These `struct lock_file`s are local to their respective functions and we
> can drop their staticness.
>
> Signed-off-by: Martin Ågren <martin.agren@gmail.com>
> ---
>  apply.c                | 2 +-
>  builtin/describe.c     | 2 +-
>  builtin/difftool.c     | 2 +-
>  builtin/gc.c           | 2 +-
>  builtin/merge.c        | 4 ++--
>  builtin/receive-pack.c | 2 +-
>  bundle.c               | 2 +-
>  fast-import.c          | 2 +-
>  refs/files-backend.c   | 2 +-
>  shallow.c              | 2 +-
>  10 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/apply.c b/apply.c
> index 7e5792c996..07b14d1127 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -4058,7 +4058,7 @@ static int build_fake_ancestor(struct apply_state *state, struct patch *list)
>  {
>         struct patch *patch;
>         struct index_state result = { NULL };
> -       static struct lock_file lock;
> +       struct lock_file lock = LOCK_INIT;

Is it really safe to do this? I vaguely remember something about
(global) linked list and signal handling which could trigger any time
and probably at atexit() time too (i.e. die()). You don't want to
depend on stack-based variables in that case.
-- 
Duy

Re: [PATCH 4/5] lock_file: make function-local locks non-static

[Duy Nguyen]

On Sun, May 6, 2018 at 7:26 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Sun, May 6, 2018 at 4:10 PM, Martin Ågren <martin.agren@gmail.com> wrote:
>> These `struct lock_file`s are local to their respective functions and we
>> can drop their staticness.
>>
>> Signed-off-by: Martin Ågren <martin.agren@gmail.com>
>> ---
>>  apply.c                | 2 +-
>>  builtin/describe.c     | 2 +-
>>  builtin/difftool.c     | 2 +-
>>  builtin/gc.c           | 2 +-
>>  builtin/merge.c        | 4 ++--
>>  builtin/receive-pack.c | 2 +-
>>  bundle.c               | 2 +-
>>  fast-import.c          | 2 +-
>>  refs/files-backend.c   | 2 +-
>>  shallow.c              | 2 +-
>>  10 files changed, 11 insertions(+), 11 deletions(-)
>>
>> diff --git a/apply.c b/apply.c
>> index 7e5792c996..07b14d1127 100644
>> --- a/apply.c
>> +++ b/apply.c
>> @@ -4058,7 +4058,7 @@ static int build_fake_ancestor(struct apply_state *state, struct patch *list)
>>  {
>>         struct patch *patch;
>>         struct index_state result = { NULL };
>> -       static struct lock_file lock;
>> +       struct lock_file lock = LOCK_INIT;
>
> Is it really safe to do this? I vaguely remember something about
> (global) linked list and signal handling which could trigger any time
> and probably at atexit() time too (i.e. die()). You don't want to
> depend on stack-based variables in that case.

So I dug in a bit more about this. The original implementation does
not allow stack-based lock files at all in 415e96c8b7 ([PATCH]
Implement git-checkout-cache -u to update stat information in the
cache. - 2005-05-15). The situation has changed since 422a21c6a0
(tempfile: remove deactivated list entries - 2017-09-05). At the end
of that second commit, Jeff mentioned "We can clean them up
individually" which I guess is what these patches do. Though I do not
know if we need to make sure to call "release" function or something/
Either way you need more explanation and assurance than just "we can
drop their staticness" in the commit mesage.
-- 
Duy

Re: [PATCH 4/5] lock_file: make function-local locks non-static

[Martin Ågren]

On 6 May 2018 at 19:42, Duy Nguyen <pclouds@gmail.com> wrote:
> On Sun, May 6, 2018 at 7:26 PM, Duy Nguyen <pclouds@gmail.com> wrote:
>> On Sun, May 6, 2018 at 4:10 PM, Martin Ågren <martin.agren@gmail.com> wrote:
>>> These `struct lock_file`s are local to their respective functions and we
>>> can drop their staticness.

>>> -       static struct lock_file lock;
>>> +       struct lock_file lock = LOCK_INIT;
>>
>> Is it really safe to do this? I vaguely remember something about
>> (global) linked list and signal handling which could trigger any time
>> and probably at atexit() time too (i.e. die()). You don't want to
>> depend on stack-based variables in that case.
>
> So I dug in a bit more about this. The original implementation does
> not allow stack-based lock files at all in 415e96c8b7 ([PATCH]
> Implement git-checkout-cache -u to update stat information in the
> cache. - 2005-05-15). The situation has changed since 422a21c6a0
> (tempfile: remove deactivated list entries - 2017-09-05). At the end
> of that second commit, Jeff mentioned "We can clean them up
> individually" which I guess is what these patches do. Though I do not
> know if we need to make sure to call "release" function or something/
> Either way you need more explanation and assurance than just "we can
> drop their staticness" in the commit mesage.

Thank you Duy for your comments. How about I write the commit message
like so:

  After 076aa2cbd (tempfile: auto-allocate tempfiles on heap, 2017-09-05),
  we can have lockfiles on the stack. These `struct lock_file`s are local
  to their respective functions and we can drop their staticness.

  Each of these users either commits or rolls back the lock in every
  codepath, with these possible exceptions:

    * We bail using a call to `die()` or `exit()`. The lock will be
      cleaned up automatically.

    * We return early from a function `cmd_foo()` in builtin/, i.e., we
      are just about to exit. The lock will be cleaned up automatically.

  If I have missed some codepath where we do not exit, yet leave a locked
  lock around, that was so also before this patch. If we would later
  re-enter the same function, then before this patch, we would be retaking
  a lock for the very same `struct lock_file`, which feels awkward, but to
  the best of my reading has well-defined behavior. Whereas after this
  patch, we would attempt to take the lock with a completely fresh `struct
  lock_file`. In both cases, the result would simply be that the lock can
  not be taken, which is a situation we already handle.

Re: [PATCH 4/5] lock_file: make function-local locks non-static

[Duy Nguyen]

On Sun, May 6, 2018 at 9:32 PM, Martin Ågren <martin.agren@gmail.com> wrote:
> On 6 May 2018 at 19:42, Duy Nguyen <pclouds@gmail.com> wrote:
>> On Sun, May 6, 2018 at 7:26 PM, Duy Nguyen <pclouds@gmail.com> wrote:
>>> On Sun, May 6, 2018 at 4:10 PM, Martin Ågren <martin.agren@gmail.com> wrote:
>>>> These `struct lock_file`s are local to their respective functions and we
>>>> can drop their staticness.
>
>>>> -       static struct lock_file lock;
>>>> +       struct lock_file lock = LOCK_INIT;
>>>
>>> Is it really safe to do this? I vaguely remember something about
>>> (global) linked list and signal handling which could trigger any time
>>> and probably at atexit() time too (i.e. die()). You don't want to
>>> depend on stack-based variables in that case.
>>
>> So I dug in a bit more about this. The original implementation does
>> not allow stack-based lock files at all in 415e96c8b7 ([PATCH]
>> Implement git-checkout-cache -u to update stat information in the
>> cache. - 2005-05-15). The situation has changed since 422a21c6a0
>> (tempfile: remove deactivated list entries - 2017-09-05). At the end
>> of that second commit, Jeff mentioned "We can clean them up
>> individually" which I guess is what these patches do. Though I do not
>> know if we need to make sure to call "release" function or something/
>> Either way you need more explanation and assurance than just "we can
>> drop their staticness" in the commit mesage.
>
> Thank you Duy for your comments. How about I write the commit message
> like so:

+Jeff. Since he made it possible to remove lock file from the global
linked list, he probably knows well what to check when switching from
a static lock file to a stack-local one.

>
>   After 076aa2cbd (tempfile: auto-allocate tempfiles on heap, 2017-09-05),
>   we can have lockfiles on the stack. These `struct lock_file`s are local
>   to their respective functions and we can drop their staticness.
>
>   Each of these users either commits or rolls back the lock in every
>   codepath, with these possible exceptions:
>
>     * We bail using a call to `die()` or `exit()`. The lock will be
>       cleaned up automatically.
>
>     * We return early from a function `cmd_foo()` in builtin/, i.e., we
>       are just about to exit. The lock will be cleaned up automatically.

There are also signals which can be caught and run on its own stack (I
think) so whatever variable on the current stack should be safe, I
guess.

>   If I have missed some codepath where we do not exit, yet leave a locked
>   lock around, that was so also before this patch. If we would later
>   re-enter the same function, then before this patch, we would be retaking
>   a lock for the very same `struct lock_file`, which feels awkward, but to
>   the best of my reading has well-defined behavior. Whereas after this
>   patch, we would attempt to take the lock with a completely fresh `struct
>   lock_file`. In both cases, the result would simply be that the lock can
>   not be taken, which is a situation we already handle.

There is a difference here, if the lock is not released properly,
previously the lockfile is still untouched. If it's on stack, it may
be overwritten which can corrupt the linked list to get to the next
lock file.  (and this is about calling the function in question just
_once_ not the second time).
-- 
Duy

Re: [PATCH 4/5] lock_file: make function-local locks non-static

[Martin Ågren]

On 7 May 2018 at 17:24, Duy Nguyen <pclouds@gmail.com> wrote:
> On Sun, May 6, 2018 at 9:32 PM, Martin Ågren <martin.agren@gmail.com> wrote:
>> On 6 May 2018 at 19:42, Duy Nguyen <pclouds@gmail.com> wrote:
>> Thank you Duy for your comments. How about I write the commit message
>> like so:
>
> +Jeff. Since he made it possible to remove lock file from the global
> linked list, he probably knows well what to check when switching from
> a static lock file to a stack-local one.
>
>>
>>   After 076aa2cbd (tempfile: auto-allocate tempfiles on heap, 2017-09-05),
>>   we can have lockfiles on the stack. These `struct lock_file`s are local
>>   to their respective functions and we can drop their staticness.
>>
>>   Each of these users either commits or rolls back the lock in every
>>   codepath, with these possible exceptions:
>>
>>     * We bail using a call to `die()` or `exit()`. The lock will be
>>       cleaned up automatically.
>>
>>     * We return early from a function `cmd_foo()` in builtin/, i.e., we
>>       are just about to exit. The lock will be cleaned up automatically.
>
> There are also signals which can be caught and run on its own stack (I
> think) so whatever variable on the current stack should be safe, I
> guess.

Right, that could also happen.

>>   If I have missed some codepath where we do not exit, yet leave a locked
>>   lock around, that was so also before this patch. If we would later
>>   re-enter the same function, then before this patch, we would be retaking
>>   a lock for the very same `struct lock_file`, which feels awkward, but to
>>   the best of my reading has well-defined behavior. Whereas after this
>>   patch, we would attempt to take the lock with a completely fresh `struct
>>   lock_file`. In both cases, the result would simply be that the lock can
>>   not be taken, which is a situation we already handle.
>
> There is a difference here, if the lock is not released properly,
> previously the lockfile is still untouched. If it's on stack, it may
> be overwritten which can corrupt the linked list to get to the next
> lock file.  (and this is about calling the function in question just
> _once_ not the second time).

One thing I can try to make clearer is that no-one is holding on to the
address of a `struct lock_file`. Not anyone, anywhere. So whether that
location gets filled with random junk after the function has returned is
irrelevant when it comes to the eventual cleaning up. The only thing
that anyone is keeping track of is the *temp*file. The `struct
lock_file` is just a fancy wrapper for keeping a pointer to a tempfile.
The tempfile infrastructure keeps track of the tempfiles directly,
without tracking any lockfiles.

Maybe it is a bit optimistic to tackle all of these at one fell swoop.
Fiddling with lockfiles makes people nervous, for good reasons. I could
go for the ones where it is easy to see that we always clean things up,
then handle the less trivial ones in a second run-over, or not at all.

Martin

Re: [PATCH 4/5] lock_file: make function-local locks non-static

[Jeff King]

On Mon, May 07, 2018 at 05:24:05PM +0200, Duy Nguyen wrote:

> >>>> -       static struct lock_file lock;
> >>>> +       struct lock_file lock = LOCK_INIT;
> >>>
> >>> Is it really safe to do this? I vaguely remember something about
> >>> (global) linked list and signal handling which could trigger any time
> >>> and probably at atexit() time too (i.e. die()). You don't want to
> >>> depend on stack-based variables in that case.
> >>
> >> So I dug in a bit more about this. The original implementation does
> >> not allow stack-based lock files at all in 415e96c8b7 ([PATCH]
> >> Implement git-checkout-cache -u to update stat information in the
> >> cache. - 2005-05-15). The situation has changed since 422a21c6a0
> >> (tempfile: remove deactivated list entries - 2017-09-05). At the end
> >> of that second commit, Jeff mentioned "We can clean them up
> >> individually" which I guess is what these patches do. Though I do not
> >> know if we need to make sure to call "release" function or something/
> >> Either way you need more explanation and assurance than just "we can
> >> drop their staticness" in the commit mesage.
> >
> > Thank you Duy for your comments. How about I write the commit message
> > like so:
> 
> +Jeff. Since he made it possible to remove lock file from the global
> linked list, he probably knows well what to check when switching from
> a static lock file to a stack-local one.

It should be totally safe. If you look at "struct lock_file", it is now
simply a pointer to a tempfile allocated on the heap (in fact, I thought
about getting rid of lock_file entirely, but the diff is noisy and it
actually has some value as an abstraction over a pure tempfile).

If you fail to call a release function, it will just hang around until
program exit, which is more or less what the static version would do.
The big difference is that if we re-enter the function while still
holding the lock, then the static version would BUG() on trying to use
the already-active lockfile. Whereas after this series, we'd try to
create a new lockfile and say "woah, somebody else is holding the lock".

> >   After 076aa2cbd (tempfile: auto-allocate tempfiles on heap, 2017-09-05),
> >   we can have lockfiles on the stack. These `struct lock_file`s are local
> >   to their respective functions and we can drop their staticness.
> >
> >   Each of these users either commits or rolls back the lock in every
> >   codepath, with these possible exceptions:
> >
> >     * We bail using a call to `die()` or `exit()`. The lock will be
> >       cleaned up automatically.
> >
> >     * We return early from a function `cmd_foo()` in builtin/, i.e., we
> >       are just about to exit. The lock will be cleaned up automatically.
> 
> There are also signals which can be caught and run on its own stack (I
> think) so whatever variable on the current stack should be safe, I
> guess.

Yes, the stack variables should all be intact during an exit or a
signal.

> >   If I have missed some codepath where we do not exit, yet leave a locked
> >   lock around, that was so also before this patch. If we would later
> >   re-enter the same function, then before this patch, we would be retaking
> >   a lock for the very same `struct lock_file`, which feels awkward, but to
> >   the best of my reading has well-defined behavior. Whereas after this
> >   patch, we would attempt to take the lock with a completely fresh `struct
> >   lock_file`. In both cases, the result would simply be that the lock can
> >   not be taken, which is a situation we already handle.
> 
> There is a difference here, if the lock is not released properly,
> previously the lockfile is still untouched. If it's on stack, it may
> be overwritten which can corrupt the linked list to get to the next
> lock file.  (and this is about calling the function in question just
> _once_ not the second time).

The only bits on the stack are just a pointer to the list item. So the
linked list is fine if it goes out of scope while the tempfile is still
active. That was the point of 076aa2cbd.

-Peff

Re: [PATCH 4/5] lock_file: make function-local locks non-static

[Duy Nguyen]

On Tue, May 8, 2018 at 8:18 PM, Jeff King <peff@peff.net> wrote:
> On Mon, May 07, 2018 at 05:24:05PM +0200, Duy Nguyen wrote:
>
>> >>>> -       static struct lock_file lock;
>> >>>> +       struct lock_file lock = LOCK_INIT;
>> >>>
>> >>> Is it really safe to do this? I vaguely remember something about
>> >>> (global) linked list and signal handling which could trigger any time
>> >>> and probably at atexit() time too (i.e. die()). You don't want to
>> >>> depend on stack-based variables in that case.
>> >>
>> >> So I dug in a bit more about this. The original implementation does
>> >> not allow stack-based lock files at all in 415e96c8b7 ([PATCH]
>> >> Implement git-checkout-cache -u to update stat information in the
>> >> cache. - 2005-05-15). The situation has changed since 422a21c6a0
>> >> (tempfile: remove deactivated list entries - 2017-09-05). At the end
>> >> of that second commit, Jeff mentioned "We can clean them up
>> >> individually" which I guess is what these patches do. Though I do not
>> >> know if we need to make sure to call "release" function or something/
>> >> Either way you need more explanation and assurance than just "we can
>> >> drop their staticness" in the commit mesage.
>> >
>> > Thank you Duy for your comments. How about I write the commit message
>> > like so:
>>
>> +Jeff. Since he made it possible to remove lock file from the global
>> linked list, he probably knows well what to check when switching from
>> a static lock file to a stack-local one.
>
> It should be totally safe. If you look at "struct lock_file", it is now
> simply a pointer to a tempfile allocated on the heap (in fact, I thought
> about getting rid of lock_file entirely, but the diff is noisy and it
> actually has some value as an abstraction over a pure tempfile).
>
> If you fail to call a release function, it will just hang around until
> program exit, which is more or less what the static version would do.
> The big difference is that if we re-enter the function while still
> holding the lock, then the static version would BUG() on trying to use
> the already-active lockfile. Whereas after this series, we'd try to
> create a new lockfile and say "woah, somebody else is holding the lock".

Ah.. I did miss that "everything on heap" thing. Sorry for the noise
Martin and thanks for clarification Jeff :)
-- 
Duy

Re: [PATCH 4/5] lock_file: make function-local locks non-static

[Martin Ågren]

On 9 May 2018 at 18:19, Duy Nguyen <pclouds@gmail.com> wrote:
> On Tue, May 8, 2018 at 8:18 PM, Jeff King <peff@peff.net> wrote:

>> It should be totally safe. If you look at "struct lock_file", it is now
>> simply a pointer to a tempfile allocated on the heap (in fact, I thought
>> about getting rid of lock_file entirely, but the diff is noisy and it
>> actually has some value as an abstraction over a pure tempfile).
>
> Ah.. I did miss that "everything on heap" thing. Sorry for the noise
> Martin and thanks for clarification Jeff :)

Hey, no problem. In fact, the "noise" as you call it had some signal in
it: the commit messages should obviously say more about this.

Martin

Re: [PATCH 4/5] lock_file: make function-local locks non-static

[Junio C Hamano]

Martin Ågren <martin.agren@gmail.com> writes:

> On 9 May 2018 at 18:19, Duy Nguyen <pclouds@gmail.com> wrote:
>> On Tue, May 8, 2018 at 8:18 PM, Jeff King <peff@peff.net> wrote:
>
>>> It should be totally safe. If you look at "struct lock_file", it is now
>>> simply a pointer to a tempfile allocated on the heap (in fact, I thought
>>> about getting rid of lock_file entirely, but the diff is noisy and it
>>> actually has some value as an abstraction over a pure tempfile).
>>
>> Ah.. I did miss that "everything on heap" thing. Sorry for the noise
>> Martin and thanks for clarification Jeff :)
>
> Hey, no problem. In fact, the "noise" as you call it had some signal in
> it: the commit messages should obviously say more about this.

Thanks all for working it out.