{"thread":{"id":"48422","subject":"[PATCH 4/5] lock_file: make function-local locks non-static","startedAt":"2018-05-06T14:11:23Z","lastAt":"2018-05-10T04:26:29Z","messageCount":10,"participants":["Martin Ågren","Duy Nguyen","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"346766","messageId":"20180506141031.30204-5-martin.agren@gmail.com","threadId":"48422","inReplyTo":null,"subject":"[PATCH 4/5] lock_file: make function-local locks non-static","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-05-06T14:10:30Z","receivedAt":"2018-05-06T14:11:23Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"These `struct lock_file`s are local to their respective functions and we\ncan drop their staticness.\n\nSigned-off-by: Martin Ågren \n---\n apply.c | 2 +-\n builtin/describe.c | 2 +-\n builtin/difftool.c | 2 +-\n builtin/gc.c | 2 +-\n builtin/merge.c | 4 ++--\n builtin/receive-pack.c | 2 +-\n bundle.c | 2 +-\n fast-import.c | 2 +-\n refs/files-backend.c | 2 +-\n shallow.c | 2 +-\n 10 files changed, 11 insertions(+), 11 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 7e5792c996..07b14d1127 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -4058,7 +4058,7 @@ static int build_fake_ancestor(struct apply_state *state, struct patch *list)\n {\n \tstruct patch *patch;\n \tstruct index_state result = { NULL };\n-\tstatic struct lock_file lock;\n+\tstruct lock_file lock = LOCK_INIT;\n \tint res;\n \n \t/* Once we start supporting the reverse patch, it may be\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex b5afc45846..8bd95cf371 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -612,7 +612,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n \t\t\t\tsuffix = broken;\n \t\t\t}\n \t\t} else if (dirty) {\n-\t\t\tstatic struct lock_file index_lock;\n+\t\t\tstruct lock_file index_lock = LOCK_INIT;\n \t\t\tstruct rev_info revs;\n \t\t\tstruct argv_array args = ARGV_ARRAY_INIT;\n \t\t\tint fd, result;\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex aad0e073ee..162806f238 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -610,7 +610,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \t\t\tcontinue;\n \n \t\tif (!indices_loaded) {\n-\t\t\tstatic struct lock_file lock;\n+\t\t\tstruct lock_file lock = LOCK_INIT;\n \t\t\tstrbuf_reset(&buf);\n \t\t\tstrbuf_addf(&buf, \"%s/wtindex\", tmpdir);\n \t\t\tif (hold_lock_file_for_update(&lock, buf.buf, 0) < 0 ||\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 3e67124eaa..274660d9dc 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -234,7 +234,7 @@ static int need_to_gc(void)\n /* return NULL on success, else hostname running the gc */\n static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n {\n-\tstatic struct lock_file lock;\n+\tstruct lock_file lock = LOCK_INIT;\n \tchar my_host[HOST_NAME_MAX + 1];\n \tstruct strbuf sb = STRBUF_INIT;\n \tstruct stat st;\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 9db5a2cf16..de62b2c5c6 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -647,7 +647,7 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,\n \t\t\t struct commit_list *remoteheads,\n \t\t\t struct commit *head)\n {\n-\tstatic struct lock_file lock;\n+\tstruct lock_file lock = LOCK_INIT;\n \tconst char *head_arg = \"HEAD\";\n \n \thold_locked_index(&lock, LOCK_DIE_ON_ERROR);\n@@ -805,7 +805,7 @@ static int merge_trivial(struct commit *head, struct commit_list *remoteheads)\n {\n \tstruct object_id result_tree, result_commit;\n \tstruct commit_list *parents, **pptr = &parents;\n-\tstatic struct lock_file lock;\n+\tstruct lock_file lock = LOCK_INIT;\n \n \thold_locked_index(&lock, LOCK_DIE_ON_ERROR);\n \trefresh_cache(REFRESH_QUIET);\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 4b68a28e92..d3cf36cef5 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -876,7 +876,7 @@ static void refuse_unconfigured_deny_delete_current(void)\n static int command_singleton_iterator(void *cb_data, struct object_id *oid);\n static int update_shallow_ref(struct command *cmd, struct shallow_info *si)\n {\n-\tstatic struct lock_file shallow_lock;\n+\tstruct lock_file shallow_lock = LOCK_INIT;\n \tstruct oid_array extra = OID_ARRAY_INIT;\n \tstruct check_connected_options opt = CHECK_CONNECTED_INIT;\n \tuint32_t mask = 1 << (cmd->index % 32);\ndiff --git a/bundle.c b/bundle.c\nindex 902c9b5448..160bbfdc64 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -409,7 +409,7 @@ static int write_bundle_refs(int bundle_fd, struct rev_info *revs)\n int create_bundle(struct bundle_header *header, const char *path,\n \t\t int argc, const char **argv)\n {\n-\tstatic struct lock_file lock;\n+\tstruct lock_file lock = LOCK_INIT;\n \tint bundle_fd = -1;\n \tint bundle_to_stdout;\n \tint ref_count = 0;\ndiff --git a/fast-import.c b/fast-import.c\nindex 05d1079d23..09443c1a9d 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1817,7 +1817,7 @@ static void dump_marks_helper(FILE *f,\n \n static void dump_marks(void)\n {\n-\tstatic struct lock_file mark_lock;\n+\tstruct lock_file mark_lock = LOCK_INIT;\n \tFILE *f;\n \n \tif (!export_marks_file || (import_marks_file && !import_marks_file_done))\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex a92a2aa821..7b2da7b8c9 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2995,7 +2995,7 @@ static int files_reflog_expire(struct ref_store *ref_store,\n {\n \tstruct files_ref_store *refs =\n \t\tfiles_downcast(ref_store, REF_STORE_WRITE, \"reflog_expire\");\n-\tstatic struct lock_file reflog_lock;\n+\tstruct lock_file reflog_lock = LOCK_INIT;\n \tstruct expire_reflog_cb cb;\n \tstruct ref_lock *lock;\n \tstruct strbuf log_file_sb = STRBUF_INIT;\ndiff --git a/shallow.c b/shallow.c\nindex df4d44ea7a..85313619ea 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -353,7 +353,7 @@ void advertise_shallow_grafts(int fd)\n */\n void prune_shallow(int show_only)\n {\n-\tstatic struct lock_file shallow_lock;\n+\tstruct lock_file shallow_lock = LOCK_INIT;\n \tstruct strbuf sb = STRBUF_INIT;\n \tint fd;\n \n-- \n2.17.0.411.g9fd64c8e46\n\n"},{"id":"346774","messageId":"CACsJy8DyRhQ0DKy8UyK+r7Kmw=0hHD=W6aXXKutk4e-wtGTdNg@mail.gmail.com","threadId":"48422","inReplyTo":"20180506141031.30204-5-martin.agren@gmail.com","subject":"Re: [PATCH 4/5] lock_file: make function-local locks non-static","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-05-06T17:26:37Z","receivedAt":"2018-05-06T17:27:16Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, May 6, 2018 at 4:10 PM, Martin Ågren wrote:\n> These `struct lock_file`s are local to their respective functions and we\n> can drop their staticness.\n>\n> Signed-off-by: Martin Ågren \n> ---\n> apply.c | 2 +-\n> builtin/describe.c | 2 +-\n> builtin/difftool.c | 2 +-\n> builtin/gc.c | 2 +-\n> builtin/merge.c | 4 ++--\n> builtin/receive-pack.c | 2 +-\n> bundle.c | 2 +-\n> fast-import.c | 2 +-\n> refs/files-backend.c | 2 +-\n> shallow.c | 2 +-\n> 10 files changed, 11 insertions(+), 11 deletions(-)\n>\n> diff --git a/apply.c b/apply.c\n> index 7e5792c996..07b14d1127 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -4058,7 +4058,7 @@ static int build_fake_ancestor(struct apply_state *state, struct patch *list)\n> {\n> struct patch *patch;\n> struct index_state result = { NULL };\n> - static struct lock_file lock;\n> + struct lock_file lock = LOCK_INIT;\n\nIs it really safe to do this? I vaguely remember something about\n(global) linked list and signal handling which could trigger any time\nand probably at atexit() time too (i.e. die()). You don't want to\ndepend on stack-based variables in that case.\n-- \nDuy\n"},{"id":"346775","messageId":"CACsJy8Btuc2J4aCTymkvLYyMV5zJrdMUdtV5NDnPqXOjsTVw4w@mail.gmail.com","threadId":"48422","inReplyTo":"CACsJy8DyRhQ0DKy8UyK+r7Kmw=0hHD=W6aXXKutk4e-wtGTdNg@mail.gmail.com","subject":"Re: [PATCH 4/5] lock_file: make function-local locks non-static","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-05-06T17:42:50Z","receivedAt":"2018-05-06T17:43:23Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, May 6, 2018 at 7:26 PM, Duy Nguyen wrote:\n> On Sun, May 6, 2018 at 4:10 PM, Martin Ågren wrote:\n>> These `struct lock_file`s are local to their respective functions and we\n>> can drop their staticness.\n>>\n>> Signed-off-by: Martin Ågren \n>> ---\n>> apply.c | 2 +-\n>> builtin/describe.c | 2 +-\n>> builtin/difftool.c | 2 +-\n>> builtin/gc.c | 2 +-\n>> builtin/merge.c | 4 ++--\n>> builtin/receive-pack.c | 2 +-\n>> bundle.c | 2 +-\n>> fast-import.c | 2 +-\n>> refs/files-backend.c | 2 +-\n>> shallow.c | 2 +-\n>> 10 files changed, 11 insertions(+), 11 deletions(-)\n>>\n>> diff --git a/apply.c b/apply.c\n>> index 7e5792c996..07b14d1127 100644\n>> --- a/apply.c\n>> +++ b/apply.c\n>> @@ -4058,7 +4058,7 @@ static int build_fake_ancestor(struct apply_state *state, struct patch *list)\n>> {\n>> struct patch *patch;\n>> struct index_state result = { NULL };\n>> - static struct lock_file lock;\n>> + struct lock_file lock = LOCK_INIT;\n>\n> Is it really safe to do this? I vaguely remember something about\n> (global) linked list and signal handling which could trigger any time\n> and probably at atexit() time too (i.e. die()). You don't want to\n> depend on stack-based variables in that case.\n\nSo I dug in a bit more about this. The original implementation does\nnot allow stack-based lock files at all in 415e96c8b7 ([PATCH]\nImplement git-checkout-cache -u to update stat information in the\ncache. - 2005-05-15). The situation has changed since 422a21c6a0\n(tempfile: remove deactivated list entries - 2017-09-05). At the end\nof that second commit, Jeff mentioned \"We can clean them up\nindividually\" which I guess is what these patches do. Though I do not\nknow if we need to make sure to call \"release\" function or something/\nEither way you need more explanation and assurance than just \"we can\ndrop their staticness\" in the commit mesage.\n-- \nDuy\n"},{"id":"346782","messageId":"CAN0heSpA5H7Gwwx0TEY9=iFJrgKb0SPXqKOwHK=4NxPYoGjZ7A@mail.gmail.com","threadId":"48422","inReplyTo":"CACsJy8Btuc2J4aCTymkvLYyMV5zJrdMUdtV5NDnPqXOjsTVw4w@mail.gmail.com","subject":"Re: [PATCH 4/5] lock_file: make function-local locks non-static","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-05-06T19:32:00Z","receivedAt":"2018-05-06T19:32:04Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 6 May 2018 at 19:42, Duy Nguyen wrote:\n> On Sun, May 6, 2018 at 7:26 PM, Duy Nguyen wrote:\n>> On Sun, May 6, 2018 at 4:10 PM, Martin Ågren wrote:\n>>> These `struct lock_file`s are local to their respective functions and we\n>>> can drop their staticness.\n\n>>> - static struct lock_file lock;\n>>> + struct lock_file lock = LOCK_INIT;\n>>\n>> Is it really safe to do this? I vaguely remember something about\n>> (global) linked list and signal handling which could trigger any time\n>> and probably at atexit() time too (i.e. die()). You don't want to\n>> depend on stack-based variables in that case.\n>\n> So I dug in a bit more about this. The original implementation does\n> not allow stack-based lock files at all in 415e96c8b7 ([PATCH]\n> Implement git-checkout-cache -u to update stat information in the\n> cache. - 2005-05-15). The situation has changed since 422a21c6a0\n> (tempfile: remove deactivated list entries - 2017-09-05). At the end\n> of that second commit, Jeff mentioned \"We can clean them up\n> individually\" which I guess is what these patches do. Though I do not\n> know if we need to make sure to call \"release\" function or something/\n> Either way you need more explanation and assurance than just \"we can\n> drop their staticness\" in the commit mesage.\n\nThank you Duy for your comments. How about I write the commit message\nlike so:\n\n After 076aa2cbd (tempfile: auto-allocate tempfiles on heap, 2017-09-05),\n we can have lockfiles on the stack. These `struct lock_file`s are local\n to their respective functions and we can drop their staticness.\n\n Each of these users either commits or rolls back the lock in every\n codepath, with these possible exceptions:\n\n * We bail using a call to `die()` or `exit()`. The lock will be\n cleaned up automatically.\n\n * We return early from a function `cmd_foo()` in builtin/, i.e., we\n are just about to exit. The lock will be cleaned up automatically.\n\n If I have missed some codepath where we do not exit, yet leave a locked\n lock around, that was so also before this patch. If we would later\n re-enter the same function, then before this patch, we would be retaking\n a lock for the very same `struct lock_file`, which feels awkward, but to\n the best of my reading has well-defined behavior. Whereas after this\n patch, we would attempt to take the lock with a completely fresh `struct\n lock_file`. In both cases, the result would simply be that the lock can\n not be taken, which is a situation we already handle.\n"},{"id":"346878","messageId":"CACsJy8DDyrUinwXx1b66DCHB+2DLt1KBmFt_83R1+HWjbzGH2A@mail.gmail.com","threadId":"48422","inReplyTo":"CAN0heSpA5H7Gwwx0TEY9=iFJrgKb0SPXqKOwHK=4NxPYoGjZ7A@mail.gmail.com","subject":"Re: [PATCH 4/5] lock_file: make function-local locks non-static","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-05-07T15:24:05Z","receivedAt":"2018-05-07T15:24:40Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, May 6, 2018 at 9:32 PM, Martin Ågren wrote:\n> On 6 May 2018 at 19:42, Duy Nguyen wrote:\n>> On Sun, May 6, 2018 at 7:26 PM, Duy Nguyen wrote:\n>>> On Sun, May 6, 2018 at 4:10 PM, Martin Ågren wrote:\n>>>> These `struct lock_file`s are local to their respective functions and we\n>>>> can drop their staticness.\n>\n>>>> - static struct lock_file lock;\n>>>> + struct lock_file lock = LOCK_INIT;\n>>>\n>>> Is it really safe to do this? I vaguely remember something about\n>>> (global) linked list and signal handling which could trigger any time\n>>> and probably at atexit() time too (i.e. die()). You don't want to\n>>> depend on stack-based variables in that case.\n>>\n>> So I dug in a bit more about this. The original implementation does\n>> not allow stack-based lock files at all in 415e96c8b7 ([PATCH]\n>> Implement git-checkout-cache -u to update stat information in the\n>> cache. - 2005-05-15). The situation has changed since 422a21c6a0\n>> (tempfile: remove deactivated list entries - 2017-09-05). At the end\n>> of that second commit, Jeff mentioned \"We can clean them up\n>> individually\" which I guess is what these patches do. Though I do not\n>> know if we need to make sure to call \"release\" function or something/\n>> Either way you need more explanation and assurance than just \"we can\n>> drop their staticness\" in the commit mesage.\n>\n> Thank you Duy for your comments. How about I write the commit message\n> like so:\n\n+Jeff. Since he made it possible to remove lock file from the global\nlinked list, he probably knows well what to check when switching from\na static lock file to a stack-local one.\n\n>\n> After 076aa2cbd (tempfile: auto-allocate tempfiles on heap, 2017-09-05),\n> we can have lockfiles on the stack. These `struct lock_file`s are local\n> to their respective functions and we can drop their staticness.\n>\n> Each of these users either commits or rolls back the lock in every\n> codepath, with these possible exceptions:\n>\n> * We bail using a call to `die()` or `exit()`. The lock will be\n> cleaned up automatically.\n>\n> * We return early from a function `cmd_foo()` in builtin/, i.e., we\n> are just about to exit. The lock will be cleaned up automatically.\n\nThere are also signals which can be caught and run on its own stack (I\nthink) so whatever variable on the current stack should be safe, I\nguess.\n\n> If I have missed some codepath where we do not exit, yet leave a locked\n> lock around, that was so also before this patch. If we would later\n> re-enter the same function, then before this patch, we would be retaking\n> a lock for the very same `struct lock_file`, which feels awkward, but to\n> the best of my reading has well-defined behavior. Whereas after this\n> patch, we would attempt to take the lock with a completely fresh `struct\n> lock_file`. In both cases, the result would simply be that the lock can\n> not be taken, which is a situation we already handle.\n\nThere is a difference here, if the lock is not released properly,\npreviously the lockfile is still untouched. If it's on stack, it may\nbe overwritten which can corrupt the linked list to get to the next\nlock file. (and this is about calling the function in question just\n_once_ not the second time).\n-- \nDuy\n"},{"id":"346909","messageId":"CAN0heSr20XFj=W28_kZAp9BWWHExQGMYa4Om=ieYWw+ucrLhRA@mail.gmail.com","threadId":"48422","inReplyTo":"CACsJy8DDyrUinwXx1b66DCHB+2DLt1KBmFt_83R1+HWjbzGH2A@mail.gmail.com","subject":"Re: [PATCH 4/5] lock_file: make function-local locks non-static","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-05-07T21:19:42Z","receivedAt":"2018-05-07T21:19:47Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 7 May 2018 at 17:24, Duy Nguyen wrote:\n> On Sun, May 6, 2018 at 9:32 PM, Martin Ågren wrote:\n>> On 6 May 2018 at 19:42, Duy Nguyen wrote:\n>> Thank you Duy for your comments. How about I write the commit message\n>> like so:\n>\n> +Jeff. Since he made it possible to remove lock file from the global\n> linked list, he probably knows well what to check when switching from\n> a static lock file to a stack-local one.\n>\n>>\n>> After 076aa2cbd (tempfile: auto-allocate tempfiles on heap, 2017-09-05),\n>> we can have lockfiles on the stack. These `struct lock_file`s are local\n>> to their respective functions and we can drop their staticness.\n>>\n>> Each of these users either commits or rolls back the lock in every\n>> codepath, with these possible exceptions:\n>>\n>> * We bail using a call to `die()` or `exit()`. The lock will be\n>> cleaned up automatically.\n>>\n>> * We return early from a function `cmd_foo()` in builtin/, i.e., we\n>> are just about to exit. The lock will be cleaned up automatically.\n>\n> There are also signals which can be caught and run on its own stack (I\n> think) so whatever variable on the current stack should be safe, I\n> guess.\n\nRight, that could also happen.\n\n>> If I have missed some codepath where we do not exit, yet leave a locked\n>> lock around, that was so also before this patch. If we would later\n>> re-enter the same function, then before this patch, we would be retaking\n>> a lock for the very same `struct lock_file`, which feels awkward, but to\n>> the best of my reading has well-defined behavior. Whereas after this\n>> patch, we would attempt to take the lock with a completely fresh `struct\n>> lock_file`. In both cases, the result would simply be that the lock can\n>> not be taken, which is a situation we already handle.\n>\n> There is a difference here, if the lock is not released properly,\n> previously the lockfile is still untouched. If it's on stack, it may\n> be overwritten which can corrupt the linked list to get to the next\n> lock file. (and this is about calling the function in question just\n> _once_ not the second time).\n\nOne thing I can try to make clearer is that no-one is holding on to the\naddress of a `struct lock_file`. Not anyone, anywhere. So whether that\nlocation gets filled with random junk after the function has returned is\nirrelevant when it comes to the eventual cleaning up. The only thing\nthat anyone is keeping track of is the *temp*file. The `struct\nlock_file` is just a fancy wrapper for keeping a pointer to a tempfile.\nThe tempfile infrastructure keeps track of the tempfiles directly,\nwithout tracking any lockfiles.\n\nMaybe it is a bit optimistic to tackle all of these at one fell swoop.\nFiddling with lockfiles makes people nervous, for good reasons. I could\ngo for the ones where it is easy to see that we always clean things up,\nthen handle the less trivial ones in a second run-over, or not at all.\n\nMartin\n"},{"id":"347002","messageId":"20180508181839.GC7210@sigill.intra.peff.net","threadId":"48422","inReplyTo":"CACsJy8DDyrUinwXx1b66DCHB+2DLt1KBmFt_83R1+HWjbzGH2A@mail.gmail.com","subject":"Re: [PATCH 4/5] lock_file: make function-local locks non-static","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-05-08T18:18:40Z","receivedAt":"2018-05-08T18:18:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 07, 2018 at 05:24:05PM +0200, Duy Nguyen wrote:\n\n> >>>> - static struct lock_file lock;\n> >>>> + struct lock_file lock = LOCK_INIT;\n> >>>\n> >>> Is it really safe to do this? I vaguely remember something about\n> >>> (global) linked list and signal handling which could trigger any time\n> >>> and probably at atexit() time too (i.e. die()). You don't want to\n> >>> depend on stack-based variables in that case.\n> >>\n> >> So I dug in a bit more about this. The original implementation does\n> >> not allow stack-based lock files at all in 415e96c8b7 ([PATCH]\n> >> Implement git-checkout-cache -u to update stat information in the\n> >> cache. - 2005-05-15). The situation has changed since 422a21c6a0\n> >> (tempfile: remove deactivated list entries - 2017-09-05). At the end\n> >> of that second commit, Jeff mentioned \"We can clean them up\n> >> individually\" which I guess is what these patches do. Though I do not\n> >> know if we need to make sure to call \"release\" function or something/\n> >> Either way you need more explanation and assurance than just \"we can\n> >> drop their staticness\" in the commit mesage.\n> >\n> > Thank you Duy for your comments. How about I write the commit message\n> > like so:\n> \n> +Jeff. Since he made it possible to remove lock file from the global\n> linked list, he probably knows well what to check when switching from\n> a static lock file to a stack-local one.\n\nIt should be totally safe. If you look at \"struct lock_file\", it is now\nsimply a pointer to a tempfile allocated on the heap (in fact, I thought\nabout getting rid of lock_file entirely, but the diff is noisy and it\nactually has some value as an abstraction over a pure tempfile).\n\nIf you fail to call a release function, it will just hang around until\nprogram exit, which is more or less what the static version would do.\nThe big difference is that if we re-enter the function while still\nholding the lock, then the static version would BUG() on trying to use\nthe already-active lockfile. Whereas after this series, we'd try to\ncreate a new lockfile and say \"woah, somebody else is holding the lock\".\n\n> > After 076aa2cbd (tempfile: auto-allocate tempfiles on heap, 2017-09-05),\n> > we can have lockfiles on the stack. These `struct lock_file`s are local\n> > to their respective functions and we can drop their staticness.\n> >\n> > Each of these users either commits or rolls back the lock in every\n> > codepath, with these possible exceptions:\n> >\n> > * We bail using a call to `die()` or `exit()`. The lock will be\n> > cleaned up automatically.\n> >\n> > * We return early from a function `cmd_foo()` in builtin/, i.e., we\n> > are just about to exit. The lock will be cleaned up automatically.\n> \n> There are also signals which can be caught and run on its own stack (I\n> think) so whatever variable on the current stack should be safe, I\n> guess.\n\nYes, the stack variables should all be intact during an exit or a\nsignal.\n\n> > If I have missed some codepath where we do not exit, yet leave a locked\n> > lock around, that was so also before this patch. If we would later\n> > re-enter the same function, then before this patch, we would be retaking\n> > a lock for the very same `struct lock_file`, which feels awkward, but to\n> > the best of my reading has well-defined behavior. Whereas after this\n> > patch, we would attempt to take the lock with a completely fresh `struct\n> > lock_file`. In both cases, the result would simply be that the lock can\n> > not be taken, which is a situation we already handle.\n> \n> There is a difference here, if the lock is not released properly,\n> previously the lockfile is still untouched. If it's on stack, it may\n> be overwritten which can corrupt the linked list to get to the next\n> lock file. (and this is about calling the function in question just\n> _once_ not the second time).\n\nThe only bits on the stack are just a pointer to the list item. So the\nlinked list is fine if it goes out of scope while the tempfile is still\nactive. That was the point of 076aa2cbd.\n\n-Peff\n"},{"id":"347095","messageId":"CACsJy8BDaHUs+DMOM7h6Lgbn8qB8yE4-O4iG=t85nG6sQ77zOw@mail.gmail.com","threadId":"48422","inReplyTo":"20180508181839.GC7210@sigill.intra.peff.net","subject":"Re: [PATCH 4/5] lock_file: make function-local locks non-static","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-05-09T16:19:56Z","receivedAt":"2018-05-09T16:20:30Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, May 8, 2018 at 8:18 PM, Jeff King wrote:\n> On Mon, May 07, 2018 at 05:24:05PM +0200, Duy Nguyen wrote:\n>\n>> >>>> - static struct lock_file lock;\n>> >>>> + struct lock_file lock = LOCK_INIT;\n>> >>>\n>> >>> Is it really safe to do this? I vaguely remember something about\n>> >>> (global) linked list and signal handling which could trigger any time\n>> >>> and probably at atexit() time too (i.e. die()). You don't want to\n>> >>> depend on stack-based variables in that case.\n>> >>\n>> >> So I dug in a bit more about this. The original implementation does\n>> >> not allow stack-based lock files at all in 415e96c8b7 ([PATCH]\n>> >> Implement git-checkout-cache -u to update stat information in the\n>> >> cache. - 2005-05-15). The situation has changed since 422a21c6a0\n>> >> (tempfile: remove deactivated list entries - 2017-09-05). At the end\n>> >> of that second commit, Jeff mentioned \"We can clean them up\n>> >> individually\" which I guess is what these patches do. Though I do not\n>> >> know if we need to make sure to call \"release\" function or something/\n>> >> Either way you need more explanation and assurance than just \"we can\n>> >> drop their staticness\" in the commit mesage.\n>> >\n>> > Thank you Duy for your comments. How about I write the commit message\n>> > like so:\n>>\n>> +Jeff. Since he made it possible to remove lock file from the global\n>> linked list, he probably knows well what to check when switching from\n>> a static lock file to a stack-local one.\n>\n> It should be totally safe. If you look at \"struct lock_file\", it is now\n> simply a pointer to a tempfile allocated on the heap (in fact, I thought\n> about getting rid of lock_file entirely, but the diff is noisy and it\n> actually has some value as an abstraction over a pure tempfile).\n>\n> If you fail to call a release function, it will just hang around until\n> program exit, which is more or less what the static version would do.\n> The big difference is that if we re-enter the function while still\n> holding the lock, then the static version would BUG() on trying to use\n> the already-active lockfile. Whereas after this series, we'd try to\n> create a new lockfile and say \"woah, somebody else is holding the lock\".\n\nAh.. I did miss that \"everything on heap\" thing. Sorry for the noise\nMartin and thanks for clarification Jeff :)\n-- \nDuy\n"},{"id":"347104","messageId":"CAN0heSr7mdaMRxsy-Aev3RpBf-9Xvg6Gw_=UyRqV0FPCVx0KNQ@mail.gmail.com","threadId":"48422","inReplyTo":"CACsJy8BDaHUs+DMOM7h6Lgbn8qB8yE4-O4iG=t85nG6sQ77zOw@mail.gmail.com","subject":"Re: [PATCH 4/5] lock_file: make function-local locks non-static","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-05-09T17:07:36Z","receivedAt":"2018-05-09T17:07:40Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 9 May 2018 at 18:19, Duy Nguyen wrote:\n> On Tue, May 8, 2018 at 8:18 PM, Jeff King wrote:\n\n>> It should be totally safe. If you look at \"struct lock_file\", it is now\n>> simply a pointer to a tempfile allocated on the heap (in fact, I thought\n>> about getting rid of lock_file entirely, but the diff is noisy and it\n>> actually has some value as an abstraction over a pure tempfile).\n>\n> Ah.. I did miss that \"everything on heap\" thing. Sorry for the noise\n> Martin and thanks for clarification Jeff :)\n\nHey, no problem. In fact, the \"noise\" as you call it had some signal in\nit: the commit messages should obviously say more about this.\n\nMartin\n"},{"id":"347171","messageId":"xmqq1sekkufj.fsf@gitster-ct.c.googlers.com","threadId":"48422","inReplyTo":"CAN0heSr7mdaMRxsy-Aev3RpBf-9Xvg6Gw_=UyRqV0FPCVx0KNQ@mail.gmail.com","subject":"Re: [PATCH 4/5] lock_file: make function-local locks non-static","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-05-10T04:26:24Z","receivedAt":"2018-05-10T04:26:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n> On 9 May 2018 at 18:19, Duy Nguyen wrote:\n>> On Tue, May 8, 2018 at 8:18 PM, Jeff King wrote:\n>\n>>> It should be totally safe. If you look at \"struct lock_file\", it is now\n>>> simply a pointer to a tempfile allocated on the heap (in fact, I thought\n>>> about getting rid of lock_file entirely, but the diff is noisy and it\n>>> actually has some value as an abstraction over a pure tempfile).\n>>\n>> Ah.. I did miss that \"everything on heap\" thing. Sorry for the noise\n>> Martin and thanks for clarification Jeff :)\n>\n> Hey, no problem. In fact, the \"noise\" as you call it had some signal in\n> it: the commit messages should obviously say more about this.\n\nThanks all for working it out.\n"}]}