{"thread":{"id":"48418","subject":"[PATCH 0/5] getting rid of most \"static struct lock_file\"s","startedAt":"2018-05-06T14:10:58Z","lastAt":"2018-05-10T07:47:28Z","messageCount":11,"participants":["Martin Ågren","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"346762","messageId":"20180506141031.30204-1-martin.agren@gmail.com","threadId":"48418","inReplyTo":null,"subject":"[PATCH 0/5] getting rid of most \"static struct lock_file\"s","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-05-06T14:10:26Z","receivedAt":"2018-05-06T14:10:58Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"This series addresses two classes of \"static struct lock_file\", removing\nthe staticness: Those locks that already live inside a function, and\nthose that can simply be moved into the function they are used from.\n\nThe first three patches are some cleanups I noticed along the way, where\nwe first take a lock using LOCK_DIE_ON_ERROR, then check whether we got\nit.\n\nAfter this series, we have a small number of \"static struct lock_file\"\nleft, namely those locks that are used from within more than one\nfunction. Thus, after this series, when one stumbles upon a static\nlock_file, it should be a clear warning that the lock is being\nused by more than one function.\n\nMartin\n\nMartin Ågren (5):\n t/helper/test-write-cache: clean up lock-handling\n refs.c: do not die if locking fails in `write_pseudoref()`\n refs.c: drop dead code checking lock status in `delete_pseudoref()`\n lock_file: make function-local locks non-static\n lock_file: move static locks into functions\n\n t/helper/test-scrap-cache-tree.c | 4 ++--\n t/helper/test-write-cache.c | 14 +++++---------\n apply.c | 2 +-\n builtin/add.c | 3 +--\n builtin/describe.c | 2 +-\n builtin/difftool.c | 2 +-\n builtin/gc.c | 2 +-\n builtin/merge.c | 4 ++--\n builtin/mv.c | 2 +-\n builtin/read-tree.c | 3 +--\n builtin/receive-pack.c | 2 +-\n builtin/rm.c | 3 +--\n bundle.c | 2 +-\n fast-import.c | 2 +-\n refs.c | 12 ++++--------\n refs/files-backend.c | 2 +-\n rerere.c | 3 +--\n shallow.c | 2 +-\n 18 files changed, 27 insertions(+), 39 deletions(-)\n\n-- \n2.17.0.411.g9fd64c8e46\n\n"},{"id":"347005","messageId":"20180508182548.GD7210@sigill.intra.peff.net","threadId":"48418","inReplyTo":"20180506141031.30204-1-martin.agren@gmail.com","subject":"Re: [PATCH 0/5] getting rid of most \"static struct lock_file\"s","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-05-08T18:25:48Z","receivedAt":"2018-05-08T18:25:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, May 06, 2018 at 04:10:26PM +0200, Martin Ågren wrote:\n\n> This series addresses two classes of \"static struct lock_file\", removing\n> the staticness: Those locks that already live inside a function, and\n> those that can simply be moved into the function they are used from.\n> \n> The first three patches are some cleanups I noticed along the way, where\n> we first take a lock using LOCK_DIE_ON_ERROR, then check whether we got\n> it.\n> \n> After this series, we have a small number of \"static struct lock_file\"\n> left, namely those locks that are used from within more than one\n> function. Thus, after this series, when one stumbles upon a static\n> lock_file, it should be a clear warning that the lock is being\n> used by more than one function.\n\nThese all look fine to me. The commit messages all made perfect sense to\nme, but it sounds like some people weren't aware of the new\npost-076aa2cbda rules. So maybe it makes sense to reference that even in\nthe earlier commits, and to explicitly say that it's safe to convert\neven in the case where the lock_file goes out of scope while still\nactive.\n\nThe only dangerous thing left to check for is anybody holding onto a\npointer-to-lockfile. The only such pointer declared outside of a\nparameter list is in create_reflock(), and there it's just to\ntemporarily coerce a void pointer. So unless somebody is doing something\nreally tricky (putting a pointer-to-lock in a \"void *\"), I think these\nconversions all have to be trivially correct (famous last words...).\n\n-Peff\n"},{"id":"347130","messageId":"cover.1525898125.git.martin.agren@gmail.com","threadId":"48418","inReplyTo":"20180508182548.GD7210@sigill.intra.peff.net","subject":"[PATCH v2 0/5] getting rid of most \"static struct lock_file\"s","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-05-09T20:55:34Z","receivedAt":"2018-05-09T20:56:22Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"This is take two of my attempt at making almost all `struct lock_file`s\nnon-static. All patches have been equipped with more detailed commit\nmessages. The only diff that has changed is patch 3/5, where I now take\na small step towards gentle error-handling, rather than going in the\nopposite direction.\n\nThanks all for the valuable feedback on v1. I could have saved everyone\nsome trouble by writing better commit messages from the start, and\nprobably also by using `--thread` when formatting the patches...\n\nMartin\n\nMartin Ågren (5):\n t/helper/test-write-cache: clean up lock-handling\n refs.c: do not die if locking fails in `write_pseudoref()`\n refs.c: do not die if locking fails in `delete_pseudoref()`\n lock_file: make function-local locks non-static\n lock_file: move static locks into functions\n\n t/helper/test-scrap-cache-tree.c | 4 ++--\n t/helper/test-write-cache.c | 14 +++++---------\n apply.c | 2 +-\n builtin/add.c | 3 +--\n builtin/describe.c | 2 +-\n builtin/difftool.c | 2 +-\n builtin/gc.c | 2 +-\n builtin/merge.c | 4 ++--\n builtin/mv.c | 2 +-\n builtin/read-tree.c | 3 +--\n builtin/receive-pack.c | 2 +-\n builtin/rm.c | 3 +--\n bundle.c | 2 +-\n fast-import.c | 2 +-\n refs.c | 16 +++++++++-------\n refs/files-backend.c | 2 +-\n rerere.c | 3 +--\n shallow.c | 2 +-\n 18 files changed, 32 insertions(+), 38 deletions(-)\n\n-- \n2.17.0.411.g9fd64c8e46\n\n"},{"id":"347131","messageId":"2832937c476ed4c53d934274caee3ec6c8c384dc.1525898125.git.martin.agren@gmail.com","threadId":"48418","inReplyTo":"cover.1525898125.git.martin.agren@gmail.com","subject":"[PATCH v2 1/5] t/helper/test-write-cache: clean up lock-handling","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-05-09T20:55:35Z","receivedAt":"2018-05-09T20:56:26Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Die in case writing the index fails, so that the caller can notice\n(instead of, say, being impressed by how performant the writing is).\n\nWhile at it, note that after opening a lock with `LOCK_DIE_ON_ERROR`, we\ndo not need to worry about whether we succeeded. Also, we can move the\n`struct lock_file` into the function and drop the staticness. (Placing\n`struct lock_file`s on the stack used to be a bad idea, because the\ntemp- and lockfile-machinery would keep a pointer into the struct. But\nafter 076aa2cbd (tempfile: auto-allocate tempfiles on heap, 2017-09-05),\nwe can safely have lockfiles on the stack.)\n\nSigned-off-by: Martin Ågren \n---\n t/helper/test-write-cache.c | 14 +++++---------\n 1 file changed, 5 insertions(+), 9 deletions(-)\n\ndiff --git a/t/helper/test-write-cache.c b/t/helper/test-write-cache.c\nindex 017dc30380..8837717d36 100644\n--- a/t/helper/test-write-cache.c\n+++ b/t/helper/test-write-cache.c\n@@ -2,22 +2,18 @@\n #include \"cache.h\"\n #include \"lockfile.h\"\n \n-static struct lock_file index_lock;\n-\n int cmd__write_cache(int argc, const char **argv)\n {\n-\tint i, cnt = 1, lockfd;\n+\tstruct lock_file index_lock = LOCK_INIT;\n+\tint i, cnt = 1;\n \tif (argc == 2)\n \t\tcnt = strtol(argv[1], NULL, 0);\n \tsetup_git_directory();\n \tread_cache();\n \tfor (i = 0; i < cnt; i++) {\n-\t\tlockfd = hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);\n-\t\tif (0 <= lockfd) {\n-\t\t\twrite_locked_index(&the_index, &index_lock, COMMIT_LOCK);\n-\t\t} else {\n-\t\t\trollback_lock_file(&index_lock);\n-\t\t}\n+\t\thold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);\n+\t\tif (write_locked_index(&the_index, &index_lock, COMMIT_LOCK))\n+\t\t\tdie(\"unable to write index file\");\n \t}\n \n \treturn 0;\n-- \n2.17.0.411.g9fd64c8e46\n\n"},{"id":"347132","messageId":"c1240491bc9029e2a72b0330a85820c1c272f537.1525898125.git.martin.agren@gmail.com","threadId":"48418","inReplyTo":"cover.1525898125.git.martin.agren@gmail.com","subject":"[PATCH v2 2/5] refs.c: do not die if locking fails in `write_pseudoref()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-05-09T20:55:36Z","receivedAt":"2018-05-09T20:56:32Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"If we could not take the lock, we add an error to the `strbuf err` and\nreturn. However, this code is dead. The reason is that we take the lock\nusing `LOCK_DIE_ON_ERROR`. Drop the flag to allow our more gentle\nerror-handling to actually kick in.\n\nWe could instead just drop the dead code and die here. But everything is\nprepared for gently propagating the error, so let's do that instead.\n\nThere is similar dead code in `delete_pseudoref()`, but let's save that\nfor the next patch.\n\nWhile at it, make the lock non-static. (Placing `struct lock_file`s on\nthe stack used to be a bad idea, because the temp- and\nlockfile-machinery would keep a pointer into the struct. But after\n076aa2cbd (tempfile: auto-allocate tempfiles on heap, 2017-09-05), we\ncan safely have lockfiles on the stack.)\n\nReviewed-by: Stefan Beller \nSigned-off-by: Martin Ågren \n---\n refs.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 8b7a77fe5e..8c50b8b139 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -644,7 +644,7 @@ static int write_pseudoref(const char *pseudoref, const struct object_id *oid,\n {\n \tconst char *filename;\n \tint fd;\n-\tstatic struct lock_file lock;\n+\tstruct lock_file lock = LOCK_INIT;\n \tstruct strbuf buf = STRBUF_INIT;\n \tint ret = -1;\n \n@@ -654,8 +654,7 @@ static int write_pseudoref(const char *pseudoref, const struct object_id *oid,\n \tstrbuf_addf(&buf, \"%s\\n\", oid_to_hex(oid));\n \n \tfilename = git_path(\"%s\", pseudoref);\n-\tfd = hold_lock_file_for_update_timeout(&lock, filename,\n-\t\t\t\t\t LOCK_DIE_ON_ERROR,\n+\tfd = hold_lock_file_for_update_timeout(&lock, filename, 0,\n \t\t\t\t\t get_files_ref_lock_timeout_ms());\n \tif (fd < 0) {\n \t\tstrbuf_addf(err, \"could not open '%s' for writing: %s\",\n-- \n2.17.0.411.g9fd64c8e46\n\n"},{"id":"347133","messageId":"9446a4b48bab984c097dc391e4e3dc060a40415c.1525898125.git.martin.agren@gmail.com","threadId":"48418","inReplyTo":"cover.1525898125.git.martin.agren@gmail.com","subject":"[PATCH v2 3/5] refs.c: do not die if locking fails in `delete_pseudoref()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-05-09T20:55:37Z","receivedAt":"2018-05-09T20:56:39Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"After taking the lock we check whether we got it and die otherwise. But\nsince we take the lock using `LOCK_DIE_ON_ERROR`, we would already have\ndied.\n\nConsidering the choice between dropping the dead code and dropping the\nflag, let's go for option number three: Drop the flag, write an error\ninstead of dying, then return -1. This function already returns -1 for\nanother error, so the caller (or rather, its callers) should be able to\nhandle this. There is some inconsistency around how we handle errors in\nthis function and elsewhere in this file, but let's take this small step\ntowards gentle error-reporting now and leave the rest for another time.\n\nWhile at it, make the lock non-static and reduce its scope. (Placing\n`struct lock_file`s on the stack used to be a bad idea, because the\ntemp- and lockfile-machinery would keep a pointer into the struct. But\nafter 076aa2cbd (tempfile: auto-allocate tempfiles on heap, 2017-09-05),\nwe can safely have lockfiles on the stack.)\n\nSigned-off-by: Martin Ågren \n---\n refs.c | 11 +++++++----\n 1 file changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 8c50b8b139..c006004bcd 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -689,20 +689,23 @@ static int write_pseudoref(const char *pseudoref, const struct object_id *oid,\n \n static int delete_pseudoref(const char *pseudoref, const struct object_id *old_oid)\n {\n-\tstatic struct lock_file lock;\n \tconst char *filename;\n \n \tfilename = git_path(\"%s\", pseudoref);\n \n \tif (old_oid && !is_null_oid(old_oid)) {\n+\t\tstruct lock_file lock = LOCK_INIT;\n \t\tint fd;\n \t\tstruct object_id actual_old_oid;\n \n \t\tfd = hold_lock_file_for_update_timeout(\n-\t\t\t\t&lock, filename, LOCK_DIE_ON_ERROR,\n+\t\t\t\t&lock, filename, 0,\n \t\t\t\tget_files_ref_lock_timeout_ms());\n-\t\tif (fd < 0)\n-\t\t\tdie_errno(_(\"Could not open '%s' for writing\"), filename);\n+\t\tif (fd < 0) {\n+\t\t\terror_errno(_(\"could not open '%s' for writing\"),\n+\t\t\t\t filename);\n+\t\t\treturn -1;\n+\t\t}\n \t\tif (read_ref(pseudoref, &actual_old_oid))\n \t\t\tdie(\"could not read ref '%s'\", pseudoref);\n \t\tif (oidcmp(&actual_old_oid, old_oid)) {\n-- \n2.17.0.411.g9fd64c8e46\n\n"},{"id":"347134","messageId":"787d85a2cda820ee5c78400691e7da77b3268fdf.1525898125.git.martin.agren@gmail.com","threadId":"48418","inReplyTo":"cover.1525898125.git.martin.agren@gmail.com","subject":"[PATCH v2 4/5] lock_file: make function-local locks non-static","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-05-09T20:55:38Z","receivedAt":"2018-05-09T20:56:46Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Placing `struct lock_file`s on the stack used to be a bad idea, because\nthe temp- and lockfile-machinery would keep a pointer into the struct.\nBut after 076aa2cbd (tempfile: auto-allocate tempfiles on heap,\n2017-09-05), we can safely have lockfiles on the stack. (This applies\neven if a user returns early, leaving a locked lock behind.)\n\nThese `struct lock_file`s are local to their respective functions and we\ncan drop their staticness.\n\nFor good measure, I have inspected these sites and come to believe that\nthey always release the lock, with the possible exception of bailing out\nusing `die()` or `exit()` or by returning from a `cmd_foo()`.\n\nAs pointed out by Jeff King, it would be bad if someone held on to a\n`struct lock_file *` for some reason. After some grepping, I agree with\nhis findings: no-one appears to be doing that.\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":"347135","messageId":"98d10821c6307394d2ed2e21e30db814bac1054e.1525898125.git.martin.agren@gmail.com","threadId":"48418","inReplyTo":"cover.1525898125.git.martin.agren@gmail.com","subject":"[PATCH v2 5/5] lock_file: move static locks into functions","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-05-09T20:55:39Z","receivedAt":"2018-05-09T20:56:51Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Placing `struct lock_file`s on the stack used to be a bad idea, because\nthe temp- and lockfile-machinery would keep a pointer into the struct.\nBut after 076aa2cbd (tempfile: auto-allocate tempfiles on heap,\n2017-09-05), we can safely have lockfiles on the stack. (This applies\neven if a user returns early, leaving a locked lock behind.)\n\nEach of these `struct lock_file`s is used from within a single function.\nMove them into the respective functions to make the scope clearer and\ndrop the staticness.\n\nFor good measure, I have inspected these sites and come to believe that\nthey always release the lock, with the possible exception of bailing out\nusing `die()` or `exit()` or by returning from a `cmd_foo()`.\n\nAs pointed out by Jeff King, it would be bad if someone held on to a\n`struct lock_file *` for some reason. After some grepping, I agree with\nhis findings: no-one appears to be doing that.\n\nAfter this commit, the remaining occurrences of \"static struct\nlock_file\" are locks that are used from within different functions. That\nis, they need to remain static. (Short of more intrusive changes like\npassing around pointers to non-static locks.)\n\nSigned-off-by: Martin Ågren \n---\n t/helper/test-scrap-cache-tree.c | 4 ++--\n builtin/add.c | 3 +--\n builtin/mv.c | 2 +-\n builtin/read-tree.c | 3 +--\n builtin/rm.c | 3 +--\n rerere.c | 3 +--\n 6 files changed, 7 insertions(+), 11 deletions(-)\n\ndiff --git a/t/helper/test-scrap-cache-tree.c b/t/helper/test-scrap-cache-tree.c\nindex d26d3e7c8b..393f1604ff 100644\n--- a/t/helper/test-scrap-cache-tree.c\n+++ b/t/helper/test-scrap-cache-tree.c\n@@ -4,10 +4,10 @@\n #include \"tree.h\"\n #include \"cache-tree.h\"\n \n-static struct lock_file index_lock;\n-\n int cmd__scrap_cache_tree(int ac, const char **av)\n {\n+\tstruct lock_file index_lock = LOCK_INIT;\n+\n \tsetup_git_directory();\n \thold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);\n \tif (read_cache() < 0)\ndiff --git a/builtin/add.c b/builtin/add.c\nindex c9e2619a9a..8a155dd41e 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -265,8 +265,6 @@ static int edit_patch(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n-static struct lock_file lock_file;\n-\n static const char ignore_error[] =\n N_(\"The following paths are ignored by one of your .gitignore files:\\n\");\n \n@@ -393,6 +391,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \tint add_new_files;\n \tint require_pathspec;\n \tchar *seen = NULL;\n+\tstruct lock_file lock_file = LOCK_INIT;\n \n \tgit_config(add_config, NULL);\n \ndiff --git a/builtin/mv.c b/builtin/mv.c\nindex 6d141f7a53..b4692409e3 100644\n--- a/builtin/mv.c\n+++ b/builtin/mv.c\n@@ -72,7 +72,6 @@ static const char *add_slash(const char *path)\n \treturn path;\n }\n \n-static struct lock_file lock_file;\n #define SUBMODULE_WITH_GITDIR ((const char *)1)\n \n static void prepare_move_submodule(const char *src, int first,\n@@ -131,6 +130,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \tenum update_mode { BOTH = 0, WORKING_DIRECTORY, INDEX } *modes;\n \tstruct stat st;\n \tstruct string_list src_for_dst = STRING_LIST_INIT_NODUP;\n+\tstruct lock_file lock_file = LOCK_INIT;\n \n \tgit_config(git_default_config, NULL);\n \ndiff --git a/builtin/read-tree.c b/builtin/read-tree.c\nindex bf87a2710b..ebc43eb805 100644\n--- a/builtin/read-tree.c\n+++ b/builtin/read-tree.c\n@@ -107,8 +107,6 @@ static int git_read_tree_config(const char *var, const char *value, void *cb)\n \treturn git_default_config(var, value, cb);\n }\n \n-static struct lock_file lock_file;\n-\n int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)\n {\n \tint i, stage = 0;\n@@ -116,6 +114,7 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)\n \tstruct tree_desc t[MAX_UNPACK_TREES];\n \tstruct unpack_trees_options opts;\n \tint prefix_set = 0;\n+\tstruct lock_file lock_file = LOCK_INIT;\n \tconst struct option read_tree_options[] = {\n \t\t{ OPTION_CALLBACK, 0, \"index-output\", NULL, N_(\"file\"),\n \t\t N_(\"write resulting index to \"),\ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex 5b6fc7ee81..65b448ef8e 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -233,8 +233,6 @@ static int check_local_mod(struct object_id *head, int index_only)\n \treturn errs;\n }\n \n-static struct lock_file lock_file;\n-\n static int show_only = 0, force = 0, index_only = 0, recursive = 0, quiet = 0;\n static int ignore_unmatch = 0;\n \n@@ -251,6 +249,7 @@ static struct option builtin_rm_options[] = {\n \n int cmd_rm(int argc, const char **argv, const char *prefix)\n {\n+\tstruct lock_file lock_file = LOCK_INIT;\n \tint i;\n \tstruct pathspec pathspec;\n \tchar *seen;\ndiff --git a/rerere.c b/rerere.c\nindex 18cae2d11c..e0862e2778 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -703,10 +703,9 @@ static int merge(const struct rerere_id *id, const char *path)\n \treturn ret;\n }\n \n-static struct lock_file index_lock;\n-\n static void update_paths(struct string_list *update)\n {\n+\tstruct lock_file index_lock = LOCK_INIT;\n \tint i;\n \n \thold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);\n-- \n2.17.0.411.g9fd64c8e46\n\n"},{"id":"347174","messageId":"20180510052137.GC27259@sigill.intra.peff.net","threadId":"48418","inReplyTo":"cover.1525898125.git.martin.agren@gmail.com","subject":"Re: [PATCH v2 0/5] getting rid of most \"static struct lock_file\"s","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-05-10T05:21:37Z","receivedAt":"2018-05-10T05:21:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 09, 2018 at 10:55:34PM +0200, Martin Ågren wrote:\n\n> This is take two of my attempt at making almost all `struct lock_file`s\n> non-static. All patches have been equipped with more detailed commit\n> messages. The only diff that has changed is patch 3/5, where I now take\n> a small step towards gentle error-handling, rather than going in the\n> opposite direction.\n> \n> Thanks all for the valuable feedback on v1. I could have saved everyone\n> some trouble by writing better commit messages from the start, and\n> probably also by using `--thread` when formatting the patches...\n\nIndeed, the world would be a more efficient place if we all did\neverything perfectly all the time. Sadly, that's not how it works. :)\n\nThis version looks good to me. Thanks for modernizing things.\n\nI don't think it's worth re-rolling, but one thing to think about for\nfuture cleanups: you split the patches by touched area, not by\nfunctionality. So the first three patches have a \"while we're here...\"\nthat has to explain why dropping the \"static\" is the right thing over\nand over. If you instead did the error-handling fixes independently\nfirst, then you could lump the \"static\" cleanups together with one\nexplanation (possibly even just as part of the 4th patch).\n\n-Peff\n"},{"id":"347177","messageId":"xmqqwowcjbh6.fsf@gitster-ct.c.googlers.com","threadId":"48418","inReplyTo":"20180510052137.GC27259@sigill.intra.peff.net","subject":"Re: [PATCH v2 0/5] getting rid of most \"static struct lock_file\"s","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-05-10T06:01:09Z","receivedAt":"2018-05-10T06:01:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I don't think it's worth re-rolling, but one thing to think about for\n> future cleanups: you split the patches by touched area, not by\n> functionality. So the first three patches have a \"while we're here...\"\n> that has to explain why dropping the \"static\" is the right thing over\n> and over. If you instead did the error-handling fixes independently\n> first, then you could lump the \"static\" cleanups together with one\n> explanation (possibly even just as part of the 4th patch).\n\nThanks Peff for a good pice of advice. I agree with the assessment\nafter reading the series through (includng \"not worth rerolling\"\npart).\n\nThanks, Martin.\n"},{"id":"347186","messageId":"CAN0heSoM0Vd73o9FmuLcDK+ojEcOwhfZRK24JypBQFz+_18uGA@mail.gmail.com","threadId":"48418","inReplyTo":"xmqqwowcjbh6.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 0/5] getting rid of most \"static struct lock_file\"s","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-05-10T07:47:24Z","receivedAt":"2018-05-10T07:47:28Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 10 May 2018 at 08:01, Junio C Hamano wrote:\n> Jeff King writes:\n>\n>> I don't think it's worth re-rolling, but one thing to think about for\n>> future cleanups: you split the patches by touched area, not by\n>> functionality. So the first three patches have a \"while we're here...\"\n>> that has to explain why dropping the \"static\" is the right thing over\n>> and over. If you instead did the error-handling fixes independently\n>> first, then you could lump the \"static\" cleanups together with one\n>> explanation (possibly even just as part of the 4th patch).\n>\n> Thanks Peff for a good pice of advice. I agree with the assessment\n> after reading the series through (includng \"not worth rerolling\"\n> part).\n\nRight. In the first version, the while-at-its were really while-at-its\n-- and it turned out it needed some motivation. So, in the reroll, I\nfocused on expanding the commit messages. Any benefit from making\npatches four and five somewhat smaller definitely got lost in the blown\nup first three commit messages. Thanks for pointing it out.\n\nMartin\n"}]}