Skip to content

Commit

Permalink
commit -a -m: allow the top-level tree to become empty again
Browse files Browse the repository at this point in the history
In 03267e8 (commit: discard partial cache before (re-)reading it,
2022-11-08), a memory leak was plugged by discarding any partial index
before re-reading it.

The problem with this memory leak fix is that it was based on an
incomplete understanding of the logic introduced in 7168624 (Do not
generate full commit log message if it is not going to be used,
2007-11-28).

That logic was introduced to add a shortcut when committing without
editing the commit message interactively. A part of that logic was to
ensure that the index was read into memory:

	if (!active_nr && read_cache() < 0)
		die(...)

Translation to English: If the index has not yet been read, read it, and
if that fails, error out.

That logic was incorrect, though: It used `!active_nr` as an indicator
that the index was not yet read. Usually this is not a problem because
in the vast majority of instances, the index contains at least one
entry.

However, when the index does not contain any entry (which is quite
common in Git's test suite), the index is read unnecessarily, and as
e.g. the `cache_tree` in that index could have been initialized,
pointing to allocated memory, this could lead to a memory leak.

The correct fix for that memory leak is to adjust the condition so that
it does not mistake `active_nr == 0` to mean that the index has not yet
been read.

Using the `initialized` flag instead, we avoid that mistake, and as a
bonus we can fix a bug at the same time that was introduced by the
memory leak fix: When deleting all tracked files and then asking `git
commit -a -m ...` to commit the result, Git would internally update the
index, then discard and re-read the index undoing the update, and fail
to commit anything.

This fixes git-for-windows#4462

Signed-off-by: Johannes Schindelin <[email protected]>
  • Loading branch information
dscho committed Jun 28, 2023
1 parent a9e066f commit 687b47e
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 5 deletions.
7 changes: 2 additions & 5 deletions builtin/commit.c
Original file line number Diff line number Diff line change
Expand Up @@ -998,11 +998,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
struct object_id oid;
const char *parent = "HEAD";

if (!the_index.cache_nr) {
discard_index(&the_index);
if (repo_read_index(the_repository) < 0)
die(_("Cannot read index"));
}
if (!the_index.initialized && repo_read_index(the_repository) < 0)
die(_("Cannot read index"));

if (amend)
parent = "HEAD^1";
Expand Down
11 changes: 11 additions & 0 deletions t/t2200-add-update.sh
Original file line number Diff line number Diff line change
Expand Up @@ -197,4 +197,15 @@ test_expect_success '"add -u non-existent" should fail' '
! grep "non-existent" actual
'

test_expect_success '"commit -a" implies "add -u" if index becomes empty' '
git rm -rf \* &&
git commit -m clean-slate &&
test_commit file1 &&
rm file1.t &&
test_tick &&
git commit -a -m remove &&
git ls-tree HEAD: >out &&
test_must_be_empty out
'

test_done

0 comments on commit 687b47e

Please sign in to comment.