Open files are deleted by checkout

[orgads]

• 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.17.0.windows.1.26.g82f1b61609
cpu: x86_64
built from commit: 82f1b6160988ab0a61edebef6ed1c3048b66a3a4
sizeof-long: 4

• 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 6.1.7601]

• What options did you set as part of the installation? Or did you choose the
  defaults?

# One of the following:
> type "C:\Program Files\Git\etc\install-options.txt"
> type "C:\Program Files (x86)\Git\etc\install-options.txt"
> type "%USERPROFILE%\AppData\Local\Programs\Git\etc\install-options.txt"
$ cat /etc/install-options.txt

Editor Option: VIM
Path Option: Cmd
SSH Option: OpenSSH
CURL Option: WinSSL
CRLF Option: CRLFAlways
Bash Terminal Option: MinTTY
Performance Tweaks FSCache: Enabled
Use Credential Manager: Enabled
Enable Symlinks: Enabled

Details

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

Git Bash

• What commands did you run to trigger this issue? If you can provide a
  Minimal, Complete, and Verifiable example
  this will help us understand the issue.

git init
echo first > foo
git add foo
git commit -m 'Initial commit'
git checkout -b other
echo second > foo
git commit -a -m 'Second'
(
  sleep 1
  git checkout master
  echo 'Press Ctrl-D now'
) &
cat >> foo
git status

• What did you expect to occur after running these commands?

I expect that either checkout will fail, or it will wait until the file is released (with some reasonable timeout), then replace it.

• What actually happened instead?

The file is deleted!

This issue happens all the time when working with Qt Creator, which parses header files constantly, so they're open for short periods. This was reported here. To workaround the issue, Qt Creator avoids using mmap in the clang library, but this did not completely solved the issue, only improved it. This is actually a Git (or MSYS?) bug, which should be solved.

[orgads]

Can you give me some pointers for investigating this? I don't know where to start :)

[drizzd]

The file is deleted in checkout_entry:

        /*
         * We unlink the old file, to get the new one with the
         * right permissions (including umask, which is nasty
         * to emulate by hand - much easier to let the system
         * just do the right thing)
         */
[...]
        } else if (unlink(path.buf))
            return error_errno("unable to unlink old '%s'", path.buf);

We subsequently open the file for writing in write_entry. At this point we get a permission denied error. This is easy to reproduce without Git:

// unlinkfoo.c
#include <unistd.h>
#include <fcntl.h>
#include <stdio.h>
int main(void)
{
        if (unlink("foo")) {
                perror("unlink");
                return 1;
        }
        int fd = open("foo", O_WRONLY | O_CREAT | O_EXCL, 0666);
        if (fd < 0) {
                perror("open");
                return 1;
        }
        close(fd);
        return 0;
}

gcc unlinkfoo.c -ounlinkfoo
echo second >foo
cat >>foo &
proc=$!
./unlinkfoo
kill $proc

Running ./unlinkfoo above fails with open: Permission denied. It is interesting that I do not get the same behavior with plain shell commands:

echo second >foo
cat >>foo &
proc=$!
rm foo
echo first >foo
cat foo
kill $proc

The above script succeeds and the content of foo is first.

[orgads]

Thank you. I'll try to investigate further.

By the way, this also happens if the file is open just for reading:

git init
echo first > foo
git add foo
git commit -m 'Initial commit'
git checkout -b other
echo second > foo
git commit -a -m 'Second'
exec 3<foo
git checkout master
exec 3>&-
git status

[orgads]

Found a solution that works for me. #1665 .

FWIW, I tried to use _wremove and DeleteFileW instead of _unlink, but neither made any difference.

I think Closing and Deleting Files has the relevant hint:

The DeleteFile function can be used to delete a file on close. A file cannot be deleted until all handles to it are closed. If a file cannot be deleted, its name cannot be reused. To reuse a file name immediately, rename the existing file.

Here is a proof of concept fix using rename and unlink:

diff --git a/compat/mingw.c b/compat/mingw.c
index 502c1961ac..ba5e67b5f9 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -386,14 +386,17 @@ static wchar_t *normalize_ntpath(wchar_t *wbuf)
 int mingw_unlink(const char *pathname)
 {
        int tries = 0;
-       wchar_t wpathname[MAX_LONG_PATH];
+       wchar_t wpathname[MAX_LONG_PATH], wtmpname[MAX_LONG_PATH+4];
        if (xutftowcs_long_path(wpathname, pathname) < 0)
                return -1;
+       wcscpy(wtmpname, wpathname);
+       wcscat(wtmpname, L".tmp");

        do {
                /* read-only files cannot be removed */
                _wchmod(wpathname, 0666);
-               if (!_wunlink(wpathname))
+               if (MoveFileExW(wpathname, wtmpname, MOVEFILE_REPLACE_EXISTING | MOVEFILE_COPY_ALLOWED) &&
+                   !_wunlink(wtmpname))
                        return 0;
                if (!is_file_in_use_error(GetLastError()))
                        break;

TODO

• Fix temporary file name conflicts (*.tmp could exist already)
• Rework _wrmdir(wpathname) below.

[orgads]

I'm sorry, but no matter what I do, I can't get it to work properly. My previous PR #1665 might be "unclean", but I consider it "good enough", as it works for the important cases.

If you have a decent alternative I'm open for suggestions, but there's no reason to reject it just because MS doesn't recommend using this function, without having a solution that works.

I was inspired for this change by this patch which was superseded by using ReplaceFile, which is not applicable in our case (without rewriting checkout_entry).

@dscho: What do you say?

[drizzd]

Your solution may be good enough to cover your use case. But we do not understand at all why the KTM Api make a difference here. It is also unclear what other undesirable effects this may have.

In particular, keep in mind that Git is optimized for performance. Even without Microsoft's warning, I would suspect that transactional operations come with a performance cost. We would have to check that.

The rename based solution needs some work. But I think it is clear what needs to be done. We even have precedence for writing to temporary files in Git before moving them into place. Maybe the code can be reused.

I'd rather we spend some time to get it right than go for a hack with unforseeable consequences. If we do not have the time, then maybe the issue is not important enough to fix.

[orgads]

@drizzd: Thanks for insisting. I now have a good working solution that doesn't use deprecated APIs. Please review #1666.

It might have been useful to have mktemp for wchar_t, to avoid conversion back and forth. Do you think it's worth implementing it?

[drizzd]

Since the performance hit of #1666 is not so nice, another idea is to avoid unlink'ing entirely. According to the comment above, it's only there to let the system handle the file mode for newly created files depending on umask. On Windows, that's not so interesting, since there is only read/write/execute permissions, not differentiating for user/group/others like on Linux. We already know the file mode from the lstat above, so we do not have to do anything if the file mode is correct, and we can use chmod otherwise.

diff --git a/entry.c b/entry.c
index 31c00816dc..db6edfa783 100644
--- a/entry.c
+++ b/entry.c
@@ -79,7 +79,7 @@ static void remove_subtree(struct strbuf *path)
 static int create_file(const char *path, unsigned int mode)
 {
        mode = (mode & 0100) ? 0777 : 0666;
-       return open(path, O_WRONLY | O_CREAT | O_EXCL, mode);
+       return open(path, O_WRONLY | O_CREAT, mode);
 }

 static void *read_blob_entry(const struct cache_entry *ce, unsigned long *size)
@@ -470,8 +470,10 @@ int checkout_entry(struct cache_entry *ce,
                        if (!state->force)
                                return error("%s is a directory", path.buf);
                        remove_subtree(&path);
-               } else if (unlink(path.buf))
-                       return error_errno("unable to unlink old '%s'", path.buf);
+               } else if ((st.st_mode & 0100) != (ce->ce_mode & 0100)) {
+                       if (chmod(path.buf, (ce->ce_mode & 0100) ? 0777 : 0666))
+                               return error_errno("unable to change mode of '%s'", path.buf);
+               }
        } else if (state->not_new)
                return 0;

TODO

• If we error out between chmod and open, then we changed the executable mode but leave the old content. Is that a problem?
• Behavior on Linux should not change, although it would be interesting to check if this could improe performance on Linux as well.
• We check and modify only the executable permission. Do we need to check read/write permissions too? Should we consider umask as well?
• Check other callers of create_file. They probably should not be affected.
• Test with file mode changes.
• Test performance.
• Run regression tests.
• Add regression tests.

Edit: fix check of chmod return value

[drizzd]

Some observations from my tests:

• I expected a performance improvement because we do not call unlink any more. But performance does not improve. Nor does it degrade.
• Checkout fails for read-only work tree files (chmod -w file). Previously read-only files were overwritten. My guess is that the behavior should not change. Git will only overwrite modified files if the user asks. This could be fixed by setting the write permissions on such files.
• By default, Git bash mounts ntfs with the noacl option. Therefore, changing the executable permissions has no effect. We should probably use chmod only if core.fileMode is not false. Do we care about behavior with acl enabled?

[orgads]

Something is broken with your change.

I executed git reset --hard, and it did not modify existing files.

My change also has strange problems with symbolic links. It leaves behind a temporary file when a symbolic link is renamed.