git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Date
Sep 16, 2018, 06:31 UTC
Message-ID
<20180916063146.9850-2-pclouds@gmail.com>
In-Reply-To
<20180916063146.9850-1-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
Previous: Nguyễn Thái Ngọc DuyNext: Junio C Hamano
Message 15 of 28 in “commit: new option to abort -a something is already staged”
  1. commit: new option to abort -a something is already stagedNguyễn Thái Ngọc Duy, Aug 20, 2018
  2. Junio C HamanoAug 20, 2018
  3. Eric SunshineAug 20, 2018
  4. Jonathan NiederAug 20, 2018
  5. Duy NguyenAug 21, 2018
  6. Jonathan NiederAug 23, 2018
  7. Jonathan NiederAug 23, 2018
  8. Duy NguyenAug 23, 2018
  9. Junio C HamanoAug 23, 2018
  10. Jacob KellerAug 24, 2018
  11. Duy NguyenAug 24, 2018
  12. Jacob KellerAug 24, 2018
  13. Jacob KellerAug 24, 2018
  14. 0/1 Make 'git commit' not accidentally lose staged contentNguyễn Thái Ngọc Duy, Sep 16, 2018
  15. 1/1 commit: do not clobber the indexNguyễn Thái Ngọc Duy, Sep 16, 2018
  16. Junio C HamanoSep 17, 2018
  17. Duy NguyenSep 17, 2018
  18. Jeff KingSep 17, 2018
  19. Duy NguyenSep 17, 2018
  20. Jeff KingSep 18, 2018
  21. Jacob KellerSep 18, 2018
  22. Jeff KingSep 18, 2018
  23. Duy NguyenSep 19, 2018
  24. Jeff KingSep 19, 2018
  25. Junio C HamanoSep 17, 2018
  26. Jacob KellerSep 18, 2018
  27. Eckhard MaaßSep 18, 2018
  28. Jacob KellerSep 18, 2018

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.