Accessing .git folder through Brackets file system adds it to the index and breaks Brackets

[marcelgerber]

1. Start a new Brackets session (don't do anything important there - it won't be saved!)
2. Do Find In Files
3. Search for anything you like, e.g. Hello
4. Click the first entry given

Result: Brackets hangs, memory consumption changes quickly, Brackets crashes in a bluescreen

It is definitely brackets-git which causes this issue, as I reproduced it with it and only it installed.

[FezVrasta]

it works for me, must be a Brackets bug, I cant find any evidence this bug is related to Brackets Git...

[marcelgerber]

Nope, I can only reproduce it with brackets-git installed/enabled.

Windows 8.1 btw

E: Here's the debug log:

[marcelgerber]

Well, I'm gonna checkout different versions/commits later in order to isolate the problem.

[marcelgerber]

It's even more strange, but e7334d8 is causing this...

[zaggino]

Is something from brackets-git executed in infinite loop? I don't have any access to Windows 8. Maybe ping someone from Brackets team who has?

cc @TomMalbran as find in files author, this bug is quite bad

[FezVrasta]

I've windows 8, no problems noticed here

[zaggino]

@SAplayer can't simulate this problem on Windows 7 so can't really fix until it's isolated.

[marcelgerber]

Investigating now.
At least I got reproducible steps (for me) now:

1. Open folder adobe/brackets (git clone, of course)
2. Find In Files: CREATE
3. Wait
4. Click any entry (but not for the current file)

Result: Brackets becomes unresponsive and blue-screens later, high load (see performance graph below, I/O peaks of 100MB/s!)

Reproduced with Brackets Shell Sprint 38, Brackets at current master, Windows 8.1, brackets-git 0.13.5 downloaded via Extension Manager, Brackets factory reset (deleted AppData/Brackets folder)

[marcelgerber]

Btw, this is the debug log outputted just before the crash:

[brackets-git] Event invoked: brackets.current.document.change - [object Object], [Document C:/Users/***/Documents/GitHub/brackets/samples/root/Getting Started/index.html (clean) (Editable) refs:2], undefined (3 listeners)
[brackets-git] cmd-spawn: C:\Program Files (x86)\Git\bin\git.exe status -u --porcelain
[brackets-git] Event invoked: brackets.file.changed - [object Object], [Directory C:/Users/***/Documents/GitHub/brackets/.git/] (1 listeners)
[brackets-git] cmd-spawn-out (1521ms;ID=10)
[brackets-git] cmd-spawn: C:\Program Files (x86)\Git\bin\git.exe status -u --porcelain
[brackets-git] cmd-spawn-out (1604ms;ID=11)
[brackets-git] cmd-spawn: C:\Program Files (x86)\Git\bin\git.exe diff --no-color -U0 -- samples/root/Getting Started/index.html
[brackets-git] Event invoked: git.status.results (2 listeners)
[brackets-git] cmd-spawn-out (248ms;ID=12)

[marcelgerber]

The failing calls are the two to git status -u --porcelain (both!) GitCli@status() and GitCli@_isFileChanged().
As soon as I enable just one of them, Brackets hangs.

Btw, in case you'd like to take a look yourself (TeamViewer or Chrome Remote Desktop), you're welcome!

[zaggino]

I'll have to look at this but probably later when I'll have more time. I still don't understand why git status (or any Git command executed) call would cause Brackets to hang in any way because they run in completely different process from Brackets.

One of the git status-es is actually a check if a file is staged or not so it should specify a filename - I'm going to fix that now (it will run much faster).

[zaggino]

It probably won't change anything, but just to be sure - can you check this again with f5108aa ?

[marcelgerber]

I've checked it with brackets-git 0.13.8, still the same :(

E: It looks like it's Brackets Helper which has the high load, but that's not better. Will try it without default extensions now.

[zaggino]

I don't understand this. These status commands get executed always when switching files - it doesn't really matter if you're using Find in Files or not... You're saying that this only occurs when Find in Files is open?

[zaggino]

And really ... nothing from e7334d8 is executed while switching files ... I don't understand :(

[marcelgerber]

Well, I don't fully understand this as well, and I doubt it's really e7334d8 which causes this. Btw, notice my edit above.

[marcelgerber]

I were able to capture the file system actions via ProcMon - wow, a 68MB file including 96.000 events.

Here's a quick screen:

It looks like Git writes some data to disk, which Brackets' file watchers are reading then.
Just FYI, CreateFile doesn't write a file to disk, it just opens one.
And the process 8928 is the one started with git.exe status -u --porcelain.

Maybe cc @njx because I know you had to work with some crash dumps.

[zaggino]

Ah, now it makes sense. Brackets file watchers should really ignore contents of .git folder.

Related:
adobe/brackets#6803
adobe/brackets#7332

cc @peterflynn

[zaggino]

From what I understand - git status makes a lot of noise in .git folder and find in files tries to listen to those changes because of the feature that updates the results automatically - https://github.com/adobe/brackets/blob/master/src/search/FindInFiles.js#L201-L207

[marcelgerber]

But there are two questions left:

• Why can't you repro it?
• Why is Git doing such a mess in the .git folder? It's just status, so I guess not much has to be changed, but in fact there a thousands of files and hundreds of megs changed.

But it even makes sense that this happening just with Find In Files - there are watchers set up to search through all the changes happening in the folder.

E: Ok, I just tested git status via console with ProcMon running: 29000 events. It's just the normal ones... I still don't know what they are for.

[marcelgerber]

I just found the probably easiest solution, which is indeed working:

if (entry.fullPath.indexOf("/.git") !== -1) {
    return;
}

(added to FindInFiles:1188-1191)

[zaggino]

Good that we know how to solve it, but this should be handled in FileWatchers directly, not in the FindInFiles. I can't imagine anyone wanting to get thousands of events when hooked on FileSystem.on("change", ...); just because Git is having some fun in its magical folder.

[marcelgerber]

I know, but I just wanted to make sure we are on the right track.

Important info for the Brackets devs:
I don't really know the Windows operation names, but it looks like the file contents are not even changed.

If you need the ProcMon log I can upload it.

[zaggino]

cc @iwehrman

[marcelgerber]

@peterflynn Those files/folders are called through FileIndex.addEntry:

C:/Program Files (x86)/Brackets/dev/.git/HEAD FileIndex.js:78
C:/Program Files (x86)/Brackets/dev/.git/refs/heads/master FileIndex.js:78
C:/Users/Marcel/Documents/GitHub/brackets/.git/ FileIndex.js:78
C:/Users/Marcel/Documents/GitHub/brackets/.git/branches/ FileIndex.js:78
C:/Users/Marcel/Documents/GitHub/brackets/.git/hooks/ FileIndex.js:78
C:/Users/Marcel/Documents/GitHub/brackets/.git/info/ FileIndex.js:78
C:/Users/Marcel/Documents/GitHub/brackets/.git/logs/ FileIndex.js:78
C:/Users/Marcel/Documents/GitHub/brackets/.git/modules/ FileIndex.js:78
C:/Users/Marcel/Documents/GitHub/brackets/.git/objects/ FileIndex.js:78
C:/Users/Marcel/Documents/GitHub/brackets/.git/refs/ FileIndex.js:78
C:/Users/Marcel/Documents/GitHub/brackets/.git/.COMMIT_EDITMSG.swp FileIndex.js:78
C:/Users/Marcel/Documents/GitHub/brackets/.git/COMMIT_EDITMSG FileIndex.js:78
C:/Users/Marcel/Documents/GitHub/brackets/.git/config FileIndex.js:78
C:/Users/Marcel/Documents/GitHub/brackets/.git/description FileIndex.js:78
C:/Users/Marcel/Documents/GitHub/brackets/.git/FETCH_HEAD FileIndex.js:78
C:/Users/Marcel/Documents/GitHub/brackets/.git/gitk.cache FileIndex.js:78
C:/Users/Marcel/Documents/GitHub/brackets/.git/HEAD FileIndex.js:78
C:/Users/Marcel/Documents/GitHub/brackets/.git/index FileIndex.js:78
C:/Users/Marcel/Documents/GitHub/brackets/.git/ORIG_HEAD FileIndex.js:78
C:/Users/Marcel/Documents/GitHub/brackets/.git/packed-refs FileIndex.js:78

The objects folder is certainly the breaking thing, because it's a very big folder with thousands of files. @zaggino Why is a folder added to the index?

@peterflynn The events are mostly on different files, but as noted above, Brackets accesses some files multiple times.

[marcelgerber]

@peterflynn Most of the change events I saw were rename events, where the old and new name is exactly the same.

You can download the ProcMon log here for further research.

[zaggino]

Ah, I know why it's added to the index now ... this check:
https://github.com/zaggino/brackets-git/blob/master/src/Branch.js#L324-L327

It resolves whether the .git folder is present in the project root.

[zaggino]

One thing in my mind that I can move our .git folder checks to be executed in node domain so we don't use Brackets FileSystem at all but if you have better ideas let me know @SAplayer @peterflynn

[marcelgerber]

So it was really e7334d8 which caused this...
@peterflynn Is there maybe a way of checking if a directory exists without watching it?

[zaggino]

This is what I see even without my extension:

BTW this problem is also seen on Windows 7 - the issue why I haven't seen it before is that when I build my own brackets shell - file watchers simply don't work (don't know why, something about version mismatch, will have to investigate later if you guys don't know the solution)

[zaggino]

@SAplayer can you check this with 95a899d

95a899d should solve problem caused by brackets-git but it won't solve the general problem if anyone else tries to access .git folder using Brackets` FileSystem

[zaggino]

@FezVrasta this problem doesn't happen when FileWatchers are down so if you're building your own Brackets shell, make sure you don't have any watching errors in your console.

@peterflynn would it be a good idea to show some kind of warning icon in Brackets when FileWatchers are down?

[marcelgerber]

@zaggino Thanks, works fine now.
Btw, I got problems building a custom shell, too. That's why I use the default one.

[zaggino]

Leave this open for a while... Something about this should be probably also done on Brackets' side.

[marcelgerber]

Ping @iwehrman @peterflynn: We should possibly ignore rename events where oldPath == newPath.