write_locked_index(): add flag to avoid writing unchanged index

We have several callers like

if (active_cache_changed && write_locked_index(...))
handle_error();
rollback_lock_file(...);

where the final rollback is needed because "!active_cache_changed"
shortcuts the if-expression. There are also a few variants of this,
including some if-else constructs that make it more clear when the
explicit rollback is really needed.

Teach `write_locked_index()` to take a new flag SKIP_IF_UNCHANGED and
simplify the callers. Leave the most complicated of the callers (in
builtin/update-index.c) unchanged. Rewriting it to use this new flag
would end up duplicating logic.

We could have made the new flag behave the other way round
("FORCE_WRITE"), but that could break existing users behind their backs.
Let's take the more conservative approach. We can still migrate existing
callers to use our new flag. Later we might even be able to flip the
default, possibly without entirely ignoring the risk to in-flight or
out-of-tree topics.

Suggested-by: Jeff King <peff@peff.net>
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
index bf01d89e28eaef0f8113d42c6b9d4142b8e912e4..9e5a80cc6f3a036e1d1f9e420825f990f6df53da 100644 (file)
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -534,10 +534,9 @@ int cmd_add(int argc, const char **argv, const char *prefix)
        unplug_bulk_checkin();
 
 finish:
        unplug_bulk_checkin();
 
 finish:

-       if (active_cache_changed) {
-               if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
-                       die(_("Unable to write new index file"));
-       }
+       if (write_locked_index(&the_index, &lock_file,
+                              COMMIT_LOCK | SKIP_IF_UNCHANGED))
+               die(_("Unable to write new index file"));

 
        UNLEAK(pathspec);
        UNLEAK(dir);
 
        UNLEAK(pathspec);
        UNLEAK(dir);

diff --git a/builtin/commit.c b/builtin/commit.c
index e8e8d13be4016e94014e98bcbe3489fad373d204..ff6dac4b9c2a1df9b8ddee9bd2f7f9eee7537034 100644 (file)
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -389,13 +389,9 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
                if (active_cache_changed
                    || !cache_tree_fully_valid(active_cache_tree))
                        update_main_cache_tree(WRITE_TREE_SILENT);
                if (active_cache_changed
                    || !cache_tree_fully_valid(active_cache_tree))
                        update_main_cache_tree(WRITE_TREE_SILENT);

-               if (active_cache_changed) {
-                       if (write_locked_index(&the_index, &index_lock,
-                                              COMMIT_LOCK))
-                               die(_("unable to write new_index file"));
-               } else {
-                       rollback_lock_file(&index_lock);
-               }
+               if (write_locked_index(&the_index, &index_lock,
+                                      COMMIT_LOCK | SKIP_IF_UNCHANGED))
+                       die(_("unable to write new_index file"));

                commit_style = COMMIT_AS_IS;
                ret = get_index_file();
                goto out;
                commit_style = COMMIT_AS_IS;
                ret = get_index_file();
                goto out;

diff --git a/builtin/merge.c b/builtin/merge.c
index 92ba99a1a5efff8598251c2ed806bae12832ae0a..7efa3c041d16fe9da504e967ef31dcd19b065d1e 100644 (file)
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -651,10 +651,9 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
 
        hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
        refresh_cache(REFRESH_QUIET);
 
        hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
        refresh_cache(REFRESH_QUIET);

-       if (active_cache_changed &&
-           write_locked_index(&the_index, &lock, COMMIT_LOCK))
+       if (write_locked_index(&the_index, &lock,
+                              COMMIT_LOCK | SKIP_IF_UNCHANGED))

                return error(_("Unable to write index."));
                return error(_("Unable to write index."));

-       rollback_lock_file(&lock);

 
        if (!strcmp(strategy, "recursive") || !strcmp(strategy, "subtree")) {
                int clean, x;
 
        if (!strcmp(strategy, "recursive") || !strcmp(strategy, "subtree")) {
                int clean, x;


@@ -691,10 +690,9 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
                                remoteheads->item, reversed, &result);
                if (clean < 0)
                        exit(128);
                                remoteheads->item, reversed, &result);
                if (clean < 0)
                        exit(128);

-               if (active_cache_changed &&
-                   write_locked_index(&the_index, &lock, COMMIT_LOCK))
+               if (write_locked_index(&the_index, &lock,
+                                      COMMIT_LOCK | SKIP_IF_UNCHANGED))

                        die (_("unable to write %s"), get_index_file());
                        die (_("unable to write %s"), get_index_file());

-               rollback_lock_file(&lock);

                return clean ? 0 : 1;
        } else {
                return try_merge_command(strategy, xopts_nr, xopts,
                return clean ? 0 : 1;
        } else {
                return try_merge_command(strategy, xopts_nr, xopts,


@@ -810,10 +808,9 @@ static int merge_trivial(struct commit *head, struct commit_list *remoteheads)
 
        hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
        refresh_cache(REFRESH_QUIET);
 
        hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
        refresh_cache(REFRESH_QUIET);

-       if (active_cache_changed &&
-           write_locked_index(&the_index, &lock, COMMIT_LOCK))
+       if (write_locked_index(&the_index, &lock,
+                              COMMIT_LOCK | SKIP_IF_UNCHANGED))

                return error(_("Unable to write index."));
                return error(_("Unable to write index."));

-       rollback_lock_file(&lock);

 
        write_tree_trivial(&result_tree);
        printf(_("Wonderful.\n"));
 
        write_tree_trivial(&result_tree);
        printf(_("Wonderful.\n"));

diff --git a/builtin/mv.c b/builtin/mv.c
index cf3684d907a2fe3408581502c682d6290c790827..ae013d6d20030640e11ea53b09530fa37bebba00 100644 (file)
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -293,8 +293,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
        if (gitmodules_modified)
                stage_updated_gitmodules(&the_index);
 
        if (gitmodules_modified)
                stage_updated_gitmodules(&the_index);
 

-       if (active_cache_changed &&
-           write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
+       if (write_locked_index(&the_index, &lock_file,
+                              COMMIT_LOCK | SKIP_IF_UNCHANGED))

                die(_("Unable to write new index file"));
 
        return 0;
                die(_("Unable to write new index file"));
 
        return 0;

diff --git a/builtin/rm.c b/builtin/rm.c
index 4a2fcca27b3f722ca520c2411b80e6984ecf780a..5d59a0aa65729a5d5c8e3c3c13ab743ce7b064b3 100644 (file)
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -385,10 +385,9 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
                        stage_updated_gitmodules(&the_index);
        }
 
                        stage_updated_gitmodules(&the_index);
        }
 

-       if (active_cache_changed) {
-               if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
-                       die(_("Unable to write new index file"));
-       }
+       if (write_locked_index(&the_index, &lock_file,
+                              COMMIT_LOCK | SKIP_IF_UNCHANGED))
+               die(_("Unable to write new index file"));

 
        return 0;
 }
 
        return 0;
 }

diff --git a/cache.h b/cache.h
index 21fbcc2414953d00929185aeb97f3a6ae9a3f73a..5c880318e0c016a6a5eca49d284963d4a1afcb9d 100644 (file)
--- a/cache.h
+++ b/cache.h
@@ -599,6 +599,7 @@ extern int read_index_unmerged(struct index_state *);
 
 /* For use with `write_locked_index()`. */
 #define COMMIT_LOCK            (1 << 0)
 
 /* For use with `write_locked_index()`. */
 #define COMMIT_LOCK            (1 << 0)

+#define SKIP_IF_UNCHANGED      (1 << 1)

 
 /*
  * Write the index while holding an already-taken lock. Close the lock,
 
 /*
  * Write the index while holding an already-taken lock. Close the lock,


@@ -615,6 +616,9 @@ extern int read_index_unmerged(struct index_state *);
  * With `COMMIT_LOCK`, the lock is always committed or rolled back.
  * Without it, the lock is closed, but neither committed nor rolled
  * back.
  * With `COMMIT_LOCK`, the lock is always committed or rolled back.
  * Without it, the lock is closed, but neither committed nor rolled
  * back.

+ *
+ * If `SKIP_IF_UNCHANGED` is given and the index is unchanged, nothing
+ * is written (and the lock is rolled back if `COMMIT_LOCK` is given).

  */
 extern int write_locked_index(struct index_state *, struct lock_file *lock, unsigned flags);
 
  */
 extern int write_locked_index(struct index_state *, struct lock_file *lock, unsigned flags);
 

diff --git a/merge-recursive.c b/merge-recursive.c
index 129577987ba25402a6b9494b103a4643e2b37e68..2f232ad3b40e6a94ea78bd856a4ea2deaaf1d468 100644 (file)
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -2223,10 +2223,9 @@ int merge_recursive_generic(struct merge_options *o,
                return clean;
        }
 
                return clean;
        }
 

-       if (active_cache_changed &&
-           write_locked_index(&the_index, &lock, COMMIT_LOCK))
+       if (write_locked_index(&the_index, &lock,
+                              COMMIT_LOCK | SKIP_IF_UNCHANGED))

                return err(o, _("Unable to write index."));
                return err(o, _("Unable to write index."));

-       rollback_lock_file(&lock);

 
        return clean ? 0 : 1;
 }
 
        return clean ? 0 : 1;
 }

diff --git a/read-cache.c b/read-cache.c
index 28bf0db9d95777659ffe43d994f4970b46a78f56..e2a939a5d2e1d7f16919bce540bceb2727cec4bb 100644 (file)
--- a/read-cache.c
+++ b/read-cache.c
@@ -2538,6 +2538,12 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,
        int new_shared_index, ret;
        struct split_index *si = istate->split_index;
 
        int new_shared_index, ret;
        struct split_index *si = istate->split_index;
 

+       if ((flags & SKIP_IF_UNCHANGED) && !istate->cache_changed) {
+               if (flags & COMMIT_LOCK)
+                       rollback_lock_file(lock);
+               return 0;
+       }
+

        if (istate->fsmonitor_last_update)
                fill_fsmonitor_bitmap(istate);
 
        if (istate->fsmonitor_last_update)
                fill_fsmonitor_bitmap(istate);
 

diff --git a/rerere.c b/rerere.c
index 79203c6c1eae0db5515030b138e9e40e219af5f5..ea24d4c2f47ab6495d97f38245dd420a1ad38391 100644 (file)
--- a/rerere.c
+++ b/rerere.c
@@ -719,11 +719,9 @@ static void update_paths(struct string_list *update)
                        item->string);
        }
 
                        item->string);
        }
 

-       if (active_cache_changed) {
-               if (write_locked_index(&the_index, &index_lock, COMMIT_LOCK))
-                       die("Unable to write new index file");
-       } else
-               rollback_lock_file(&index_lock);
+       if (write_locked_index(&the_index, &index_lock,
+                              COMMIT_LOCK | SKIP_IF_UNCHANGED))
+               die("Unable to write new index file");

 }
 
 static void remove_variant(struct rerere_id *id)
 }
 
 static void remove_variant(struct rerere_id *id)

diff --git a/sequencer.c b/sequencer.c
index 548ad997b50d5dcdfb24eeebcf91398c5d82de81..6ee251849d7a45a989b28a9f87bd9c6ca95c22ba 100644 (file)
--- a/sequencer.c
+++ b/sequencer.c
@@ -517,15 +517,14 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
                return clean;
        }
 
                return clean;
        }
 

-       if (active_cache_changed &&
-           write_locked_index(&the_index, &index_lock, COMMIT_LOCK))
+       if (write_locked_index(&the_index, &index_lock,
+                              COMMIT_LOCK | SKIP_IF_UNCHANGED))

                /*
                 * TRANSLATORS: %s will be "revert", "cherry-pick" or
                 * "rebase -i".
                 */
                return error(_("%s: Unable to write new index file"),
                        _(action_name(opts)));
                /*
                 * TRANSLATORS: %s will be "revert", "cherry-pick" or
                 * "rebase -i".
                 */
                return error(_("%s: Unable to write new index file"),
                        _(action_name(opts)));

-       rollback_lock_file(&index_lock);

 
        if (!clean)
                append_conflicts_hint(msgbuf);
 
        if (!clean)
                append_conflicts_hint(msgbuf);


@@ -1713,13 +1712,13 @@ static int read_and_refresh_cache(struct replay_opts *opts)
                        _(action_name(opts)));
        }
        refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);
                        _(action_name(opts)));
        }
        refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);

-       if (the_index.cache_changed && index_fd >= 0) {
-               if (write_locked_index(&the_index, &index_lock, COMMIT_LOCK)) {
+       if (index_fd >= 0) {
+               if (write_locked_index(&the_index, &index_lock,
+                                      COMMIT_LOCK | SKIP_IF_UNCHANGED)) {

                        return error(_("git %s: failed to refresh the index"),
                                _(action_name(opts)));
                }
        }
                        return error(_("git %s: failed to refresh the index"),
                                _(action_name(opts)));
                }
        }

-       rollback_lock_file(&index_lock);

        return 0;
 }
 
        return 0;
 }