hold_locked_index(): align error handling with hold_lockfile_for_update()

Callers of the hold_locked_index() function pass 0 when they want to prepare to write a new version of the index file without wishing to die or emit an error message when the request fails (e.g. somebody else already held the lock), and pass 1 when they want the call to die upon failure. This option is called LOCK_DIE_ON_ERROR by the underlying lockfile API, and the hold_locked_index() function translates the paramter to LOCK_DIE_ON_ERROR when calling the hold_lock_file_for_update(). Replace these hardcoded '1' with LOCK_DIE_ON_ERROR and stop translating. Callers other than the ones that are replaced with this change pass '0' to the function; no behaviour change is intended with this patch. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- Among the callers of hold_locked_index() that passes 0: - diff.c::refresh_index_quietly() at the end of "git diff" is an opportunistic update; it leaks the lockfile structure but it is just before the program exits and nobody should care. - builtin/describe.c::cmd_describe(), builtin/commit.c::cmd_status(), sequencer.c::read_and_refresh_cache() are all opportunistic updates and they are OK. - builtin/update-index.c::cmd_update_index() takes a lock upfront but we may end up not needing to update the index (i.e. the entries may be fully up-to-date), in which case we do not need to issue an error upon failure to acquire the lock. We do diagnose and die if we indeed need to update, so it is OK. - wt-status.c::require_clean_work_tree() IS BUGGY. It asks silence, does not check the returned value. Compare with callsites like cmd_describe() and cmd_status() to notice that it is wrong to call update_index_if_able() unconditionally.

Junio C Hamano committed Dec 7, 2016 at 10:33 UTC b3e83cc752e905e063d0930c682a06de5034074f
18 files changed +27 -29
apply.c
+1 -1
@@ -4688,7 +4688,7 @@ static int apply_patch(struct apply_state *state,
4688 state->index_file,
4689 LOCK_DIE_ON_ERROR);
4690 else
4691 - state->newfd = hold_locked_index(state->lock_file, 1);
4691 + state->newfd = hold_locked_index(state->lock_file, LOCK_DIE_ON_ERROR);
4692 }
4693
4694 if (state->check_index && read_apply_cache(state) < 0) {
builtin/add.c
+1 -1
@@ -361,7 +361,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
361 add_new_files = !take_worktree_changes && !refresh_only;
362 require_pathspec = !(take_worktree_changes || (0 < addremove_explicit));
363
364 - hold_locked_index(&lock_file, 1);
364 + hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);
365
366 flags = ((verbose ? ADD_CACHE_VERBOSE : 0) |
367 (show_only ? ADD_CACHE_PRETEND : 0) |
builtin/am.c
+3 -3
@@ -1119,7 +1119,7 @@ static void refresh_and_write_cache(void)
1119 {
1120 struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));
1121
1122 - hold_locked_index(lock_file, 1);
1122 + hold_locked_index(lock_file, LOCK_DIE_ON_ERROR);
1123 refresh_cache(REFRESH_QUIET);
1124 if (write_locked_index(&the_index, lock_file, COMMIT_LOCK))
1125 die(_("unable to write index file"));
@@ -1976,7 +1976,7 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset)
1976 return -1;
1977
1978 lock_file = xcalloc(1, sizeof(struct lock_file));
1979 - hold_locked_index(lock_file, 1);
1979 + hold_locked_index(lock_file, LOCK_DIE_ON_ERROR);
1980
1981 refresh_cache(REFRESH_QUIET);
1982
@@ -2016,7 +2016,7 @@ static int merge_tree(struct tree *tree)
2016 return -1;
2017
2018 lock_file = xcalloc(1, sizeof(struct lock_file));
2019 - hold_locked_index(lock_file, 1);
2019 + hold_locked_index(lock_file, LOCK_DIE_ON_ERROR);
2020
2021 memset(&opts, 0, sizeof(opts));
2022 opts.head_idx = 1;
builtin/checkout-index.c
+1 -1
@@ -205,7 +205,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
205 if (index_opt && !state.base_dir_len && !to_tempfile) {
206 state.refresh_cache = 1;
207 state.istate = &the_index;
208 - newfd = hold_locked_index(&lock_file, 1);
208 + newfd = hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);
209 }
210
211 /* Check out named files first */
builtin/checkout.c
+2 -2
@@ -274,7 +274,7 @@ static int checkout_paths(const struct checkout_opts *opts,
274
275 lock_file = xcalloc(1, sizeof(struct lock_file));
276
277 - hold_locked_index(lock_file, 1);
277 + hold_locked_index(lock_file, LOCK_DIE_ON_ERROR);
278 if (read_cache_preload(&opts->pathspec) < 0)
279 return error(_("index file corrupt"));
280
@@ -467,7 +467,7 @@ static int merge_working_tree(const struct checkout_opts *opts,
467 int ret;
468 struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));
469
470 - hold_locked_index(lock_file, 1);
470 + hold_locked_index(lock_file, LOCK_DIE_ON_ERROR);
471 if (read_cache_preload(NULL) < 0)
472 return error(_("index file corrupt"));
473
builtin/clone.c
+1 -1
@@ -711,7 +711,7 @@ static int checkout(int submodule_progress)
711 setup_work_tree();
712
713 lock_file = xcalloc(1, sizeof(struct lock_file));
714 - hold_locked_index(lock_file, 1);
714 + hold_locked_index(lock_file, LOCK_DIE_ON_ERROR);
715
716 memset(&opts, 0, sizeof opts);
717 opts.update = 1;
builtin/commit.c
+4 -4
@@ -351,7 +351,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
351
352 if (interactive) {
353 char *old_index_env = NULL;
354 - hold_locked_index(&index_lock, 1);
354 + hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);
355
356 refresh_cache_or_die(refresh_flags);
357
@@ -396,7 +396,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
396 * (B) on failure, rollback the real index.
397 */
398 if (all || (also && pathspec.nr)) {
399 - hold_locked_index(&index_lock, 1);
399 + hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);
400 add_files_to_cache(also ? prefix : NULL, &pathspec, 0);
401 refresh_cache_or_die(refresh_flags);
402 update_main_cache_tree(WRITE_TREE_SILENT);
@@ -416,7 +416,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
416 * We still need to refresh the index here.
417 */
418 if (!only && !pathspec.nr) {
419 - hold_locked_index(&index_lock, 1);
419 + hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);
420 refresh_cache_or_die(refresh_flags);
421 if (active_cache_changed
422 || !cache_tree_fully_valid(active_cache_tree))
@@ -468,7 +468,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
468 if (read_cache() < 0)
469 die(_("cannot read the index"));
470
471 - hold_locked_index(&index_lock, 1);
471 + hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);
472 add_remove_files(&partial);
473 refresh_cache(REFRESH_QUIET);
474 update_main_cache_tree(WRITE_TREE_SILENT);
builtin/merge.c
+3 -3
@@ -634,7 +634,7 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
634 {
635 static struct lock_file lock;
636
637 - hold_locked_index(&lock, 1);
637 + hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
638 refresh_cache(REFRESH_QUIET);
639 if (active_cache_changed &&
640 write_locked_index(&the_index, &lock, COMMIT_LOCK))
@@ -671,7 +671,7 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
671 for (j = common; j; j = j->next)
672 commit_list_insert(j->item, &reversed);
673
674 - hold_locked_index(&lock, 1);
674 + hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
675 clean = merge_recursive(&o, head,
676 remoteheads->item, reversed, &result);
677 if (clean < 0)
@@ -781,7 +781,7 @@ static int merge_trivial(struct commit *head, struct commit_list *remoteheads)
781 struct commit_list *parents, **pptr = &parents;
782 static struct lock_file lock;
783
784 - hold_locked_index(&lock, 1);
784 + hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
785 refresh_cache(REFRESH_QUIET);
786 if (active_cache_changed &&
787 write_locked_index(&the_index, &lock, COMMIT_LOCK))
builtin/mv.c
+1 -1
@@ -126,7 +126,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
126 if (--argc < 1)
127 usage_with_options(builtin_mv_usage, builtin_mv_options);
128
129 - hold_locked_index(&lock_file, 1);
129 + hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);
130 if (read_cache() < 0)
131 die(_("index file corrupt"));
132
builtin/read-tree.c
+1 -1
@@ -150,7 +150,7 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
150 argc = parse_options(argc, argv, unused_prefix, read_tree_options,
151 read_tree_usage, 0);
152
153 - hold_locked_index(&lock_file, 1);
153 + hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);
154
155 prefix_set = opts.prefix ? 1 : 0;
156 if (1 < opts.merge + opts.reset + prefix_set)
builtin/reset.c
+1 -1
@@ -354,7 +354,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
354
355 if (reset_type != SOFT) {
356 struct lock_file *lock = xcalloc(1, sizeof(*lock));
357 - hold_locked_index(lock, 1);
357 + hold_locked_index(lock, LOCK_DIE_ON_ERROR);
358 if (reset_type == MIXED) {
359 int flags = quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN;
360 if (read_from_tree(&pathspec, &oid, intent_to_add))
builtin/rm.c
+1 -1
@@ -292,7 +292,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
292 if (!index_only)
293 setup_work_tree();
294
295 - hold_locked_index(&lock_file, 1);
295 + hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);
296
297 if (read_cache() < 0)
298 die(_("index file corrupt"));
builtin/update-index.c
+1
@@ -1012,6 +1012,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
1012 /* We can't free this memory, it becomes part of a linked list parsed atexit() */
1013 lock_file = xcalloc(1, sizeof(struct lock_file));
1014
1015 + /* we will diagnose later if it turns out that we need to update it */
1016 newfd = hold_locked_index(lock_file, 0);
1017 if (newfd < 0)
1018 lock_error = errno;
merge-recursive.c
+1 -1
@@ -2124,7 +2124,7 @@ int merge_recursive_generic(struct merge_options *o,
2124 }
2125 }
2126
2127 - hold_locked_index(lock, 1);
2127 + hold_locked_index(lock, LOCK_DIE_ON_ERROR);
2128 clean = merge_recursive(o, head_commit, next_commit, ca,
2129 result);
2130 if (clean < 0)
read-cache.c
+2 -5
@@ -1425,12 +1425,9 @@ static int read_index_extension(struct index_state *istate,
1425 return 0;
1426 }
1427
1428 -int hold_locked_index(struct lock_file *lk, int die_on_error)
1428 +int hold_locked_index(struct lock_file *lk, int lock_flags)
1429 {
1430 - return hold_lock_file_for_update(lk, get_index_file(),
1431 - die_on_error
1432 - ? LOCK_DIE_ON_ERROR
1433 - : 0);
1430 + return hold_lock_file_for_update(lk, get_index_file(), lock_flags);
1431 }
1432
1433 int read_index(struct index_state *istate)
rerere.c
+1 -1
@@ -708,7 +708,7 @@ static void update_paths(struct string_list *update)
708 {
709 int i;
710
711 - hold_locked_index(&index_lock, 1);
711 + hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);
712
713 for (i = 0; i < update->nr; i++) {
714 struct string_list_item *item = &update->items[i];
sequencer.c
+1 -1
@@ -370,7 +370,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
370 char **xopt;
371 static struct lock_file index_lock;
372
373 - hold_locked_index(&index_lock, 1);
373 + hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);
374
375 read_cache();
376
t/helper/test-scrap-cache-tree.c
+1 -1
@@ -8,7 +8,7 @@ static struct lock_file index_lock;
8 int cmd_main(int ac, const char **av)
9 {
10 setup_git_directory();
11 - hold_locked_index(&index_lock, 1);
11 + hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);
12 if (read_cache() < 0)
13 die("unable to read index file");
14 active_cache_tree = NULL;