From 75d9a25e1f461c0908aad67d3c32bb9b2fea70ec Mon Sep 17 00:00:00 2001 From: Martin Ågren Date: Wed, 9 May 2018 22:55:35 +0200 Subject: t/helper/test-write-cache: clean up lock-handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Die in case writing the index fails, so that the caller can notice (instead of, say, being impressed by how performant the writing is). While at it, note that after opening a lock with `LOCK_DIE_ON_ERROR`, we do not need to worry about whether we succeeded. Also, we can move the `struct lock_file` into the function and drop the staticness. (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 076aa2cbd (tempfile: auto-allocate tempfiles on heap, 2017-09-05), we can safely have lockfiles on the stack.) Signed-off-by: Martin Ågren Signed-off-by: Junio C Hamano --- t/helper/test-write-cache.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) (limited to 't') diff --git a/t/helper/test-write-cache.c b/t/helper/test-write-cache.c index b7ee039669..3d78c728a5 100644 --- a/t/helper/test-write-cache.c +++ b/t/helper/test-write-cache.c @@ -1,22 +1,18 @@ #include "cache.h" #include "lockfile.h" -static struct lock_file index_lock; - int cmd_main(int argc, const char **argv) { - int i, cnt = 1, lockfd; + struct lock_file index_lock = LOCK_INIT; + int i, cnt = 1; if (argc == 2) cnt = strtol(argv[1], NULL, 0); setup_git_directory(); read_cache(); for (i = 0; i < cnt; i++) { - lockfd = hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR); - if (0 <= lockfd) { - write_locked_index(&the_index, &index_lock, COMMIT_LOCK); - } else { - rollback_lock_file(&index_lock); - } + hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR); + if (write_locked_index(&the_index, &index_lock, COMMIT_LOCK)) + die("unable to write index file"); } return 0; -- cgit v1.3 From 0fa5a2ed8d9f6d987f1ea479fe8ea56a26b89303 Mon Sep 17 00:00:00 2001 From: Martin Ågren Date: Wed, 9 May 2018 22:55:39 +0200 Subject: lock_file: move static locks into functions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 076aa2cbd (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 Signed-off-by: Junio C Hamano --- builtin/add.c | 3 +-- builtin/mv.c | 2 +- builtin/read-tree.c | 3 +-- builtin/rm.c | 3 +-- rerere.c | 3 +-- t/helper/test-scrap-cache-tree.c | 4 ++-- 6 files changed, 7 insertions(+), 11 deletions(-) (limited to 't') diff --git a/builtin/add.c b/builtin/add.c index 9ef7fb02d5..80152bcaed 100644 --- 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 index 6d141f7a53..b4692409e3 100644 --- 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 index bf87a2710b..ebc43eb805 100644 --- a/builtin/read-tree.c +++ b/builtin/read-tree.c @@ -107,8 +107,6 @@ 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; @@ -116,6 +114,7 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix) 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 "), diff --git a/builtin/rm.c b/builtin/rm.c index 4447bb4d0f..0fcb952e9b 100644 --- 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 index ea24d4c2f4..04d25691e4 100644 --- a/rerere.c +++ b/rerere.c @@ -703,10 +703,9 @@ out: 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 index d2a63bea43..34596201d4 100644 --- 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) -- cgit v1.3