[PATCH v2 1/1] commit: do not clobber the index

["Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>]

"git commit" can be used in two different workflows:

 - the user never bothers with "git add" and uses "git commit" for
   both adding content to the index and committing it

 - the user uses "git add" to carefully prepare what they want to
   commit, and "git commit" creates a new commit out of the current
   index. In this case "git commit" does not modify the index at all.

The user could switch between the two workflows of course, and
mistakes can happen that lead to content loss. Imagine the user has
prepared the index with 'git add -p' (second workflow), which makes
the index content different from both HEAD and worktree. If they
accidentally do "git commit -a" (first workflow) after that, all the
preparation is gone because "git commit" clobbers the index.

Prevent this by checking the staged content when runnning "commit -a"
or "commit <path>". If the staged content is different from both HEAD
and the worktree version, abort the commit. The user can still
override this by giving "--clobber-index" option.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/git-commit.txt     |  11 +++-
 builtin/commit.c                 | 105 ++++++++++++++++++++++++++++---
 cache.h                          |   1 +
 read-cache.c                     |  11 ++++
 t/t2201-add-update-typechange.sh |   2 +-
 t/t4015-diff-whitespace.sh       |   2 +-
 t/t7102-reset.sh                 |   2 +-
 t/t7500-commit.sh                |   2 +-
 t/t7502-commit.sh                |   4 +-
 t/t9350-fast-export.sh           |   2 +-
 10 files changed, 126 insertions(+), 16 deletions(-)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index f970a43422..2d128527ec 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -31,8 +31,9 @@ The content to be added can be specified in several ways:
 
 3. by listing files as arguments to the 'commit' command
    (without --interactive or --patch switch), in which
-   case the commit will ignore changes staged in the index, and instead
-   record the current content of the listed files (which must already
+   case the commit will ignore changes staged in the index if
+   --clobber-index is specified, and instead record the current
+   content of the listed files (which must already
    be known to Git);
 
 4. by using the -a switch with the 'commit' command to automatically
@@ -274,6 +275,12 @@ FROM UPSTREAM REBASE" section in linkgit:git-rebase[1].)
 	already been staged. If used together with `--allow-empty`
 	paths are also not required, and an empty commit will be created.
 
+--clobber-index::
+	By default 'git commit' will not update the index if
+	non-conflicted staged content of a path is different from both
+	HEAD and the working tree. This option skips this check and
+	makes 'git commit' ignore content changes in the index.
+
 -u[<mode>]::
 --untracked-files[=<mode>]::
 	Show untracked files.
diff --git a/builtin/commit.c b/builtin/commit.c
index 0d9828e29e..a179b8a9e7 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -101,6 +101,7 @@ static int all, also, interactive, patch_interactive, only, amend, signoff;
 static int edit_flag = -1; /* unspecified */
 static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;
 static int config_commit_verbose = -1; /* unspecified */
+static int clobber_index;
 static int no_post_rewrite, allow_empty_message;
 static char *untracked_files_arg, *force_date, *ignore_submodule_arg, *ignored_arg;
 static char *sign_commit;
@@ -263,11 +264,78 @@ static int list_paths(struct string_list *list, const char *with_tree,
 	return ret;
 }
 
-static void add_remove_files(struct string_list *list)
+static void mark_staged_paths_cb(struct diff_queue_struct *q,
+				 struct diff_options *options,
+				 void *data)
+{
+	struct index_state *istate = data;
+	int i;
+
+	for (i = 0; i < q->nr; i++) {
+		struct diff_filepair *p;
+		int pos;
+
+		p = q->queue[i];
+
+		switch (p->status) {
+		case DIFF_STATUS_ADDED:
+			break;
+
+		case DIFF_STATUS_MODIFIED:
+			/*
+			 * _MODIFIED could be either because of content or
+			 * mode change. Ignore mode change.
+			 */
+			if (oidcmp(&p->one->oid, &p->two->oid))
+				break;
+			continue;
+
+		default:
+			continue;
+		}
+
+		pos = index_name_pos(istate, p->two->path,
+				     strlen(p->two->path));
+		if (pos < 0)
+			BUG("entry '%s' => '%s' status '%c' not found in the index",
+			    p->one->path ? p->one->path : "(empty)",
+			    p->two->path ? p->two->path : "(empty)",
+			    p->status);
+		istate->cache[pos]->ce_flags |= CE_MATCHED;
+	}
+}
+
+static void mark_staged_paths(struct index_state *istate,
+			      const char *prefix,
+			      struct pathspec *pathspec)
+{
+	struct rev_info rev;
+	struct setup_revision_opt opt;
+	int i;
+
+	init_revisions(&rev, NULL);
+	memset(&opt, 0, sizeof(opt));
+	opt.def = "HEAD";
+	setup_revisions(0, NULL, &rev, &opt);
+
+	rev.diffopt.ita_invisible_in_index = 1;
+	rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
+	rev.diffopt.format_callback = mark_staged_paths_cb;
+	rev.diffopt.format_callback_data = istate;
+	copy_pathspec(&rev.prune_data, pathspec);
+
+	for (i = 0; i < istate->cache_nr; i++)
+		istate->cache[i]->ce_flags &= ~CE_MATCHED;
+
+	run_diff_index(&rev, 1);
+}
+
+static void add_remove_files(struct string_list *list, int flags)
 {
 	int i;
 	for (i = 0; i < list->nr; i++) {
 		struct stat st;
+		int pos;
 		struct string_list_item *p = &(list->items[i]);
 
 		/* p->util is skip-worktree */
@@ -275,10 +343,18 @@ static void add_remove_files(struct string_list *list)
 			continue;
 
 		if (!lstat(p->string, &st)) {
-			if (add_to_cache(p->string, &st, 0))
+			if (add_to_cache(p->string, &st, flags))
 				die(_("updating files failed"));
-		} else
-			remove_file_from_cache(p->string);
+			continue;
+		}
+
+		if (flags & ADD_CACHE_KEEP_CE_MATCHED &&
+		    (pos = index_name_pos(&the_index, p->string,
+					  strlen(p->string))) >= 0 &&
+		    the_index.cache[pos]->ce_flags & CE_MATCHED)
+			die(_("clobbering '%s' is not allowed"),
+			    p->string);
+		remove_file_from_cache(p->string);
 	}
 }
 
@@ -326,6 +402,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
 	struct string_list partial = STRING_LIST_INIT_DUP;
 	struct pathspec pathspec;
 	int refresh_flags = REFRESH_QUIET;
+	unsigned int add_flags = 0;
 	const char *ret;
 
 	if (is_status)
@@ -372,6 +449,9 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
 		goto out;
 	}
 
+	if (!clobber_index && !current_head)
+		clobber_index = 1;
+
 	/*
 	 * Non partial, non as-is commit.
 	 *
@@ -385,8 +465,14 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
 	 * (B) on failure, rollback the real index.
 	 */
 	if (all || (also && pathspec.nr)) {
+		const char *add_prefix = also ? prefix : NULL;
+
 		hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);
-		add_files_to_cache(also ? prefix : NULL, &pathspec, 0);
+		if (!clobber_index) {
+			mark_staged_paths(&the_index, add_prefix, &pathspec);
+			add_flags |= ADD_CACHE_KEEP_CE_MATCHED;
+		}
+		add_files_to_cache(add_prefix, &pathspec, add_flags);
 		refresh_cache_or_die(refresh_flags);
 		update_main_cache_tree(WRITE_TREE_SILENT);
 		if (write_locked_index(&the_index, &index_lock, 0))
@@ -455,7 +541,11 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
 		die(_("cannot read the index"));
 
 	hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);
-	add_remove_files(&partial);
+	if (!clobber_index) {
+		mark_staged_paths(&the_index, NULL, &pathspec);
+		add_flags |= ADD_CACHE_KEEP_CE_MATCHED;
+	}
+	add_remove_files(&partial, add_flags);
 	refresh_cache(REFRESH_QUIET);
 	update_main_cache_tree(WRITE_TREE_SILENT);
 	if (write_locked_index(&the_index, &index_lock, 0))
@@ -467,7 +557,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
 				  LOCK_DIE_ON_ERROR);
 
 	create_base_index(current_head);
-	add_remove_files(&partial);
+	add_remove_files(&partial, 0);
 	refresh_cache(REFRESH_QUIET);
 
 	if (write_locked_index(&the_index, &false_lock, 0))
@@ -1477,6 +1567,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		OPT_BOOL('o', "only", &only, N_("commit only specified files")),
 		OPT_BOOL('n', "no-verify", &no_verify, N_("bypass pre-commit and commit-msg hooks")),
 		OPT_BOOL(0, "dry-run", &dry_run, N_("show what would be committed")),
+		OPT_BOOL(0, "clobber-index", &clobber_index, N_("clobber the index")),
 		OPT_SET_INT(0, "short", &status_format, N_("show status concisely"),
 			    STATUS_FORMAT_SHORT),
 		OPT_BOOL(0, "branch", &s.show_branch, N_("show branch information")),
diff --git a/cache.h b/cache.h
index b1fd3d58ab..8718cc636b 100644
--- a/cache.h
+++ b/cache.h
@@ -738,6 +738,7 @@ extern int index_name_pos(const struct index_state *, const char *name, int name
 #define ADD_CACHE_JUST_APPEND 8		/* Append only; tree.c::read_tree() */
 #define ADD_CACHE_NEW_ONLY 16		/* Do not replace existing ones */
 #define ADD_CACHE_KEEP_CACHE_TREE 32	/* Do not invalidate cache-tree */
+#define ADD_CACHE_KEEP_CE_MATCHED 64	/* Do not change CE_MATCHED files */
 extern int add_index_entry(struct index_state *, struct cache_entry *ce, int option);
 extern void rename_index_entry_at(struct index_state *, int pos, const char *new_name);
 
diff --git a/read-cache.c b/read-cache.c
index 7b1354d759..6b912225d5 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -757,6 +757,17 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
 			discard_cache_entry(ce);
 			return error("unable to index file %s", path);
 		}
+		if (flags & ADD_CACHE_KEEP_CE_MATCHED) {
+			struct cache_entry *ent;
+			int pos = index_name_pos(istate, path, namelen);
+
+			ent = (0 <= pos) ? istate->cache[pos] : NULL;
+			if (ent && ent->ce_flags & CE_MATCHED &&
+			    oidcmp(&ent->oid, &ce->oid)) {
+				discard_cache_entry(ce);
+				return error(_("clobbering '%s' is not allowed"), path);
+			}
+		}
 	} else
 		set_object_name_for_intent_to_add_entry(ce);
 
diff --git a/t/t2201-add-update-typechange.sh b/t/t2201-add-update-typechange.sh
index a4eec0a346..99bf3264bf 100755
--- a/t/t2201-add-update-typechange.sh
+++ b/t/t2201-add-update-typechange.sh
@@ -136,7 +136,7 @@ test_expect_success 'commit -a' '
 		rm -f ".git/index" &&
 		mv ".git/saved-index" ".git/index"
 	fi &&
-	git commit -m "second" -a &&
+	git commit --clobber-index -m "second" -a &&
 	git ls-files -s >actual &&
 	test_cmp expect-final actual &&
 	rm -f .git/index &&
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 35fc8b5c2a..e85789b95a 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -616,7 +616,7 @@ test_expect_success 'check staged with space before tab in indent (diff-index)'
 
 test_expect_success 'check with no whitespace errors (diff-tree)' '
 	echo "foo();" >x &&
-	git commit -m "new commit" x &&
+	git commit --clobber-index -m "new commit" x &&
 	git diff-tree --check HEAD^ HEAD
 '
 
diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
index 97be0d968d..dcb773edf4 100755
--- a/t/t7102-reset.sh
+++ b/t/t7102-reset.sh
@@ -223,7 +223,7 @@ EOF
 test_expect_success \
 	'changing files and redo the last commit should succeed' '
 	echo "3rd line 2nd file" >>secondfile &&
-	git commit -a -C ORIG_HEAD &&
+	git commit --clobber-index -a -C ORIG_HEAD &&
 	head4=$(git rev-parse --verify HEAD) &&
 	check_changes $head4 &&
 	test "$(git rev-parse ORIG_HEAD)" = \
diff --git a/t/t7500-commit.sh b/t/t7500-commit.sh
index 170b4810e0..6864c396d3 100755
--- a/t/t7500-commit.sh
+++ b/t/t7500-commit.sh
@@ -174,7 +174,7 @@ EOF
 
 test_expect_success '--signoff' '
 	echo "yet another content *narf*" >> foo &&
-	echo "zort" | git commit -s -F - foo &&
+	echo "zort" | git commit --clobber-index -s -F - foo &&
 	git cat-file commit HEAD | sed "1,/^\$/d" > output &&
 	test_cmp expect output
 '
diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index ca4a740da0..c4f2423ec3 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -93,7 +93,7 @@ test_expect_success 'the basics' '
 	echo update added "commit is" file >"commit is" &&
 	echo also update another >not/forbid &&
 	test_tick &&
-	git commit -a -m "initial with -a" &&
+	git commit --clobber-index -a -m "initial with -a" &&
 
 	git cat-file blob HEAD:"commit is" >current.1 &&
 	git cat-file blob HEAD:not/forbid >current.2 &&
@@ -188,7 +188,7 @@ test_expect_success 'prepare file with comment line and trailing newlines'  '
 test_expect_success 'cleanup commit messages (verbatim option,-t)' '
 
 	echo >>negative &&
-	git commit --cleanup=verbatim --no-status -t expect -a &&
+	git commit --cleanup=verbatim --no-status -t expect -a --clobber-index &&
 	git cat-file -p HEAD |sed -e "1,/^\$/d" >actual &&
 	test_cmp expect actual
 
diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
index 6a392e87bc..c7aeccec96 100755
--- a/t/t9350-fast-export.sh
+++ b/t/t9350-fast-export.sh
@@ -480,7 +480,7 @@ test_expect_success 'avoid uninteresting refs' '
 	git tag v1.0 &&
 	git branch uninteresting &&
 	echo bump > file &&
-	git commit -a -m bump &&
+	git commit --clobber-index -a -m bump &&
 	git fast-export --import-marks=tmp-marks \
 		--export-marks=tmp-marks ^uninteresting ^v1.0 master > actual &&
 	test_cmp expected actual
-- 
2.19.0.rc0.337.ge906d732e7