Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

commit operation fails when used with both -a and -m #4462

Closed
1 task done
baudronp opened this issue Jun 8, 2023 · 6 comments
Closed
1 task done

commit operation fails when used with both -a and -m #4462

baudronp opened this issue Jun 8, 2023 · 6 comments
Assignees
@dscho

Comments

@baudronp
Copy link

baudronp commented Jun 8, 2023

  • I was not able to find an open or closed issue matching what I'm seeing

Setup

  • Which version of Git for Windows are you using? Is it 32-bit or 64-bit?
$ git --version --build-options
git version 2.40.1.windows.1
cpu: x86_64
built from commit: ceee26d5cac05a3437097b43d034c4ad2e99d571
sizeof-long: 4
sizeof-size_t: 8
shell-path: /bin/sh
feature: fsmonitor--daemon
  • Which version of Windows are you running? Vista, 7, 8, 10? Is it 32-bit or 64-bit?
$ cmd.exe /c ver
Microsoft Windows [Version 10.0.19044.2965]
  • What options did you set as part of the installation? Or did you choose the
    defaults?
> type "C:\Program Files\Git\etc\install-options.txt"
Editor Option: VisualStudioCode
Custom Editor Path:
Default Branch Option: main
Path Option: Cmd
SSH Option: ExternalOpenSSH
Tortoise Option: false
CURL Option: WinSSL
CRLF Option: CRLFAlways
Bash Terminal Option: MinTTY
Git Pull Behavior Option: Rebase
Use Credential Manager: Enabled
Performance Tweaks FSCache: Enabled
Enable Symlinks: Disabled
Enable Pseudo Console Support: Disabled
Enable FSMonitor: Disabled
  • Any other interesting things about your environment that might be related
    to the issue you're seeing?

No

Details

  • Which terminal/shell are you running Git from? e.g Bash/CMD/PowerShell/other

CMD

mkdir test_repo
cd test_repo
git init
echo test > file1.txt
echo test > file2.txt
git add --all
git commit -m "first commit"
del file1.txt
del file2.txt
git commit -m "remove files" -a
  • What did you expect to occur after running these commands?

Both deleted files (file1.txt and file2.txt) are automatically staged (-a) and the commit is created with the description remove files.

  • What actually happened instead?

Starting with version 2.40.0, Git report that nothing is present in the staging area:

>git commit -m "remove files" -a
On branch main
Changes not staged for commit:
  (use "git add/rm <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
        deleted:    file1.txt
        deleted:    file2.txt

no changes added to commit (use "git add" and/or "git commit -a")

With an older version like 2.39.2 it works as expected:

>2.39.2\cmd\git.exe commit -m "remove files" -a
[detached HEAD d555039] remove files
 2 files changed, 2 deletions(-)
 delete mode 100644 file1.txt
 delete mode 100644 file2.txt

If we remove the -m option it works.
If we only have one delete file it works.

  • If the problem was occurring with a specific repository, can you provide the
    URL to that repository to help us with testing?

It can be reproduced on a locally initialized repository.
@baudronp
Copy link
Author

Hi,
Is it the good place to raise this issue?
If not, don't hesitate to point me to the right one 🙂

@dscho
Copy link
Member

dscho commented Jun 27, 2023

2.39.2\cmd\git.exe commit -m "remove files" -a

Could you check if putting -a before the -m "remove files" argument make things work?

@baudronp
Copy link
Author

baudronp commented Jun 28, 2023 •
edited
Loading

The same thing happen:

>git commit -a -m "remove files"
On branch main
Changes not staged for commit:
  (use "git add/rm <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
        deleted:    file1.txt
        deleted:    file2.txt

no changes added to commit (use "git add" and/or "git commit -a")

@dscho
Copy link
Member

dscho commented Jun 28, 2023

Aha! I implemented this as a new test case for Git's test suite and at first could not get it to fail. The crucial part to make it fail was to ensure that no tracked file is left after staging the deletions. And I can confirm that it is a Git upstream issue, it's not specific to Git for Windows.

Will keep you posted.

@dscho dscho self-assigned this Jun 28, 2023
@dscho dscho mentioned this issue Jun 28, 2023
Closed
dscho added a commit to dscho/git that referenced this issue Jun 28, 2023
@dscho
commit -a -m: allow the top-level tree to become empty again
687b47e 
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 <johannes.schindelin@gmx.de>
dscho added a commit to dscho/git that referenced this issue Jun 28, 2023
@dscho
commit -a -m: allow the top-level tree to become empty again
6b8929a 
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 <johannes.schindelin@gmx.de>
dscho added a commit to dscho/git that referenced this issue Jun 29, 2023
@dscho
commit -a -m: allow the top-level tree to become empty again
d3391e3 
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.

And it was natural to do it this way because at the time that condition
was introduced, the `index_state` structure had no explicit flag to
indicate that it was initialized: This flag was only introduced in
913e0e9 (unpack_trees(): protect the handcrafted in-core index from
read_cache(), 2008-08-23), but that commit did not adjust the code path
where no index file was found and a new, pristine index was initialized.

Now, when the index does not contain any entry (which is quite
common in Git's test suite because it starts quite a many repositories
from scratch), subsequent calls to `do_read_index()` will mistake the
index not to be initialized, and read it again unnecessarily.

This is a problem because after initializing the empty index e.g. the
`cache_tree` in that index could have been initialized before a
subsequent call to `do_read_index()` wants to ensure an initialized
index. And if that subsequent call mistakes the index not to have been
initialized, it would lead to leaked memory.

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 <johannes.schindelin@gmx.de>
dscho added a commit to dscho/git that referenced this issue Jun 29, 2023
@dscho
commit -a -m: allow the top-level tree to become empty again
08c50b6 
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.

And it was natural to do it this way because at the time that condition
was introduced, the `index_state` structure had no explicit flag to
indicate that it was initialized: This flag was only introduced in
913e0e9 (unpack_trees(): protect the handcrafted in-core index from
read_cache(), 2008-08-23), but that commit did not adjust the code path
where no index file was found and a new, pristine index was initialized.

Now, when the index does not contain any entry (which is quite
common in Git's test suite because it starts quite a many repositories
from scratch), subsequent calls to `do_read_index()` will mistake the
index not to be initialized, and read it again unnecessarily.

This is a problem because after initializing the empty index e.g. the
`cache_tree` in that index could have been initialized before a
subsequent call to `do_read_index()` wants to ensure an initialized
index. And if that subsequent call mistakes the index not to have been
initialized, it would lead to leaked memory.

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 <johannes.schindelin@gmx.de>
@dscho
Copy link
Member

dscho commented Jun 30, 2023

gitgitgadget#1554 seems to be cruising along. Once that's accepted upstream, we can either backport it into Git for Windows or get it via the next upstream Git version.

@baudronp
Copy link
Author

baudronp commented Jun 30, 2023 •
edited
Loading

Thanks for the fix and the update!
On my side there is no urgency it can wait for the next upstream version 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dscho dscho

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@dscho @baudronp