lock_file: move static locks into functions

Martin Ågren · gitster · commit 0fa5a2ed8d9f · 2018-05-10T14:55:40.000+09:00
Placing `struct lock_file`s on the stack used to be a bad idea, because the temp- and lockfile-machinery would keep a pointer into the struct. But after 076aa2c (tempfile: auto-allocate tempfiles on heap, 2017-09-05), we can safely have lockfiles on the stack. (This applies even if a user returns early, leaving a locked lock behind.) Each of these `struct lock_file`s is used from within a single function. Move them into the respective functions to make the scope clearer and drop the staticness. For good measure, I have inspected these sites and come to believe that they always release the lock, with the possible exception of bailing out using `die()` or `exit()` or by returning from a `cmd_foo()`. As pointed out by Jeff King, it would be bad if someone held on to a `struct lock_file *` for some reason. After some grepping, I agree with his findings: no-one appears to be doing that. After this commit, the remaining occurrences of "static struct lock_file" are locks that are used from within different functions. That is, they need to remain static. (Short of more intrusive changes like passing around pointers to non-static locks.) Signed-off-by: Martin Ågren <martin.agren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
diff --git a/builtin/add.c b/builtin/add.c
@@ -265,8 +265,6 @@ static int edit_patch(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
-static struct lock_file lock_file;
-
 static const char ignore_error[] =
 N_("The following paths are ignored by one of your .gitignore files:\n");
 
@@ -393,6 +391,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	int add_new_files;
 	int require_pathspec;
 	char *seen = NULL;
+	struct lock_file lock_file = LOCK_INIT;
 
 	git_config(add_config, NULL);
 
diff --git a/builtin/mv.c b/builtin/mv.c
@@ -72,7 +72,6 @@ static const char *add_slash(const char *path)
 	return path;
 }
 
-static struct lock_file lock_file;
 #define SUBMODULE_WITH_GITDIR ((const char *)1)
 
 static void prepare_move_submodule(const char *src, int first,
@@ -131,6 +130,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 	enum update_mode { BOTH = 0, WORKING_DIRECTORY, INDEX } *modes;
 	struct stat st;
 	struct string_list src_for_dst = STRING_LIST_INIT_NODUP;
+	struct lock_file lock_file = LOCK_INIT;
 
 	git_config(git_default_config, NULL);
 
diff --git a/builtin/read-tree.c b/builtin/read-tree.c
@@ -107,15 +107,14 @@ static int git_read_tree_config(const char *var, const char *value, void *cb)
 	return git_default_config(var, value, cb);
 }
 
-static struct lock_file lock_file;
-
 int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
 {
 	int i, stage = 0;
 	struct object_id oid;
 	struct tree_desc t[MAX_UNPACK_TREES];
 	struct unpack_trees_options opts;
 	int prefix_set = 0;
+	struct lock_file lock_file = LOCK_INIT;
 	const struct option read_tree_options[] = {
 		{ OPTION_CALLBACK, 0, "index-output", NULL, N_("file"),
 		  N_("write resulting index to <file>"),
diff --git a/builtin/rm.c b/builtin/rm.c
@@ -233,8 +233,6 @@ static int check_local_mod(struct object_id *head, int index_only)
 	return errs;
 }
 
-static struct lock_file lock_file;
-
 static int show_only = 0, force = 0, index_only = 0, recursive = 0, quiet = 0;
 static int ignore_unmatch = 0;
 
@@ -251,6 +249,7 @@ static struct option builtin_rm_options[] = {
 
 int cmd_rm(int argc, const char **argv, const char *prefix)
 {
+	struct lock_file lock_file = LOCK_INIT;
 	int i;
 	struct pathspec pathspec;
 	char *seen;
diff --git a/rerere.c b/rerere.c
@@ -703,10 +703,9 @@ static int merge(const struct rerere_id *id, const char *path)
 	return ret;
 }
 
-static struct lock_file index_lock;
-
 static void update_paths(struct string_list *update)
 {
+	struct lock_file index_lock = LOCK_INIT;
 	int i;
 
 	hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);
diff --git a/t/helper/test-scrap-cache-tree.c b/t/helper/test-scrap-cache-tree.c
@@ -3,10 +3,10 @@
 #include "tree.h"
 #include "cache-tree.h"
 
-static struct lock_file index_lock;
-
 int cmd_main(int ac, const char **av)
 {
+	struct lock_file index_lock = LOCK_INIT;
+
 	setup_git_directory();
 	hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);
 	if (read_cache() < 0)

Original file line number	Diff line number	Diff line change
`@@ -233,8 +233,6 @@ static int check_local_mod(struct object_id *head, int index_only)`
`233`	`233`	`return errs;`
`234`	`234`	`}`
`235`	`235`
`236`		`-static struct lock_file lock_file;`
`237`		`-`
`238`	`236`	`static int show_only = 0, force = 0, index_only = 0, recursive = 0, quiet = 0;`
`239`	`237`	`static int ignore_unmatch = 0;`
`240`	`238`
`@@ -251,6 +249,7 @@ static struct option builtin_rm_options[] = {`
`251`	`249`
`252`	`250`	`int cmd_rm(int argc, const char *argv, const char prefix)`
`253`	`251`	`{`
	`252`	`+ struct lock_file lock_file = LOCK_INIT;`
`254`	`253`	`int i;`
`255`	`254`	`struct pathspec pathspec;`
`256`	`255`	`char *seen;`
Original file line number	Diff line number	Diff line change
`@@ -703,10 +703,9 @@ static int merge(const struct rerere_id id, const char path)`
`703`	`703`	`return ret;`
`704`	`704`	`}`
`705`	`705`
`706`		`-static struct lock_file index_lock;`
`707`		`-`
`708`	`706`	`static void update_paths(struct string_list *update)`
`709`	`707`	`{`
	`708`	`+ struct lock_file index_lock = LOCK_INIT;`
`710`	`709`	`int i;`
`711`	`710`
`712`	`711`	`hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);`