OokTech/TW5-Bob

Git's "discard change" create file with _1 suffix

Closed this issue · 9 comments

linonetwo commented

I am running (check any that apply, put an x inside the [ ] to check a box, like this: [x]):

  • Windows
  • OSX
  • Linux
  • Other

and using

  • The nodejs version
  • The single file executable

Before posting I read issue guidelines and:

  • I am using the newest version
  • The answer to my question isn't listed in the documentation or this isn't
    a question
  • This is not a duplicate issue
  • I have not done anything that required me to set acceptance to
    I Will Not Get Tech Support For This

Watch my screenshot video gif below:

gitdiscardchange

When Git fastly deletes and recreate the file, seems Bob will create an Index_1.tid, and if I delete that file, then Bob will show Index.tid is deleted. But actually that file is still on the disk. I have to restart Bob to get it back to the browser.

This won't happen all the time. Sometimes when I discard change in the GitHub Desktop, Bob just works fine. But one out of two times this but will happen.

Maybe this is related to #134 ?

inmysocks commented

If you use some other form of syncing you will have this problem. You can turn off the file system monitor or not use other syncing software or git while Bob is running.

joshuafontany commented

I have been wanting to use Bob to control Git for my wikis backup/push etc. I think it would be work figuring out a way to allow Bob to turn off the File System Monitor internally before running tasks that might invoke outside processes, then back on afterwards. I wonder if this issue has come up with other projects that use git...

linonetwo commented

Hi @inmysocks , actually I'm using Bob because I'm developing an App that auto-sync tw data to Github, so we can use private repo of Github to build cross-platform privacy-first TW app.

So I really need Bob to work with git operations...

Can I turn off file system monitor on the fly and reopen it after git operation so it knows what happened correctly? So I don't need to restart server after git operation just like before I meet Bob.

linonetwo commented

@joshuafontany I don't think we should turn off the File System Monitor, because if so then Bob won't know files changed by git.

linonetwo commented

When Github Desktop discards a change, it moves the file to trash and re-check-out that file from the previous commit. This happens very quick.

Why would this cause this bug in Bob?

Maybe

  1. move file to trash
  2. this change is monitored by Bob, so it delete the tiddler in the browser (it disappeared for half a second)
  3. git check out Index.tid
  4. Somehow bob added that file back to fs, so there is Index_1.tid

@inmysocks do you have any comment on this?

linonetwo commented

Maybe add a debounce to fs.watch can solve this?

I'm using

const frequentlyChangedFileThatShouldBeIgnoredFromWatch = new Set(['output', '$__StoryList.tid']);
const topLevelFoldersToIgnored = ['node_modules', '.git'];

function watchFolder(wikiRepoPath, wikiFolderPath, githubRepoUrl, userInfo) {
  fs.watch(
    wikiFolderPath,
    { recursive: true },
    debounce((_, fileName) => {
      if (topLevelFoldersToIgnored.some(name => fileName.startsWith(name))) return;
      if (frequentlyChangedFileThatShouldBeIgnoredFromWatch.has(fileName)) return;
      console.log(`${fileName} change`);

      debounceCommitAndSync(wikiRepoPath, githubRepoUrl, userInfo);
    }, 500),
  );
  console.log(`wiki watch ${wikiFolderPath} now`);
}

In my sync script. What's your opinion @inmysocks ?

linonetwo commented

I dive into this, add some console.log to trace what actually happened, found sometimes when this error occurs, it runs into this if branch:

TW5-Bob/FileSystem/FileSystemMonitor.js

Lines 49 to 62 in 03e2ea4

if(err.code === 'ENOENT') {
// The item doesn't exist, so it was removed
// If the file doesn't exist anymore remove it from the wiki
if(['.tid', '.meta'].indexOf(fileExtension) !== -1) {
$tw.Bob.DeleteTiddler(folder, filename, prefix);
} else {
$tw.Bob.logger.log('non-tiddler file deleted:', filename, {level: 3})
}
} else if(err.code === 'EACCES') {
// Permissions error
} else {
// Some other error
}
} else {

This is because:

  1. git delete file on fs
  2. fs.watch detect this change
  3. if git checkout is fast enough, then there is no trouble; but it is a bit slow...
  4. fs.stat found this file 'ENOENT', it thinks the file has been removed
  5. so Bob deletes the tiddler in the browser using $tw.Bob.DeleteTiddler

then git check out the file after 20ms...this time fs.stat say we got the file, so Bob thinks we are creating a new file, so it goes to this else branch, where newTitle is still Index same as old one.

TW5-Bob/FileSystem/FileSystemMonitor.js

Lines 62 to 120 in 03e2ea4

} else {
// Item exists
// If it is a new folder than watch that folder too
if(fileStats.isDirectory()) {
$tw.Bob.WatchFolder(itemPath, prefix)
} else if(fileStats.isFile()) {
const tiddlerName = Object.keys($tw.Bob.Files[prefix]).filter(function (item) {
// This is to handle some edge cases I ran into while making
// it.
if(typeof item === 'string') {
return ($tw.Bob.Files[prefix][item].filepath === itemPath)
} else {
return false;
}
})[0];
if(['.tid', '.meta'].indexOf(fileExtension) !== -1) {
let tiddlerObject = {tiddlers:[{}]}
// This try block catches an annoying race condition problem
// when the filesystem adaptor deletes a file the file watcher
// starts acting before the deleting is completely finished.
// This means that it sees the file as still existing and tries // to open it, but it is deleted so there is an error.
try {
// Load tiddler data from the file
tiddlerObject = $tw.loadTiddlersFromFile(itemPath);
} catch (e) {
if(e.code !== 'ENOENT') {
$tw.Bob.logger.error(e, {level: 3})
}
return
}
// Make sure that it at least has a title
if(tiddlerObject.tiddlers[0]['title']) {
// Test to see if the filename matches what the wiki says it
// should be. If not rename the file to match the rules set by
// the wiki.
// This is the title based on the current .tid file
let newTitle = $tw.syncadaptor.generateTiddlerBaseFilepath(tiddlerObject.tiddlers[0].title, prefix);
const existingTiddler = $tw.Bob.Wikis[prefix].wiki.getTiddler(tiddlerObject.tiddlers[0].title);
// Load the tiddler from the wiki, check if they are different (non-existent is changed)
if($tw.Bob.Shared.TiddlerHasChanged(existingTiddler, {fields: tiddlerObject.tiddlers[0]})) {
// Rename the file
// If $:/config/FileSystemPaths is used than the folder and
// newTitle may overlap.
// This determines if any of the title has an overlap in the path
if(newTitle.replace('\\','/').indexOf('/') !== -1) {
const pieces = newTitle.replace('\\','/').split('/')
let pathBits = pieces.slice(0,-1);
while (pathBits.length > 0) {
if(folder.endsWith(pathBits.join(path.sep))) {
break;
}
pathBits = pathBits.slice(0,-1);
}
if(pathBits.length > 0) {
newTitle = pieces.slice(pathBits.length).join(path.sep);
}
}
// translate tiddler title into filepath
const theFilepath = path.join(folder, newTitle + fileExtension);

But somehow it wants to create a new tiddler

So, it will use the old tiddler object, to create tiddler here↓

And when creating with same name as an existed tiddler, it will automatically append _1 to the end of title

TW5-Bob/FileSystem/FileSystemMonitor.js

Lines 155 to 160 in 03e2ea4

} else {
// Create the new tiddler
const newTiddler = $tw.Bob.Shared.normalizeTiddler({fields: tiddlerObject.tiddlers[0]});
// Save the new file
$tw.syncadaptor.saveTiddler(newTiddler, prefix);
}

Then, when I discard this Index_1, it go into 'ENOENT' branch, so

  1. it thinks the file has been removed
  2. so Bob deletes the tiddler in the browser using $tw.Bob.DeleteTiddler
linonetwo commented

If git cause error:

----
eventType "rename" Index.tid
err.code "ENOENT"
Deleted tiddler Index
----
eventType "rename" Index.tid
fileStats.isFile() true

if it works fine

----
eventType "rename" Index.tid
----
eventType "rename" Index.tid
fileStats.isFile() true
fileStats.isFile() true

a typical race condition bug.

linonetwo commented

Oh, I know what's exactly going on!

  1. If fs.watch say the file is modified, Bob sync from fs to browser, $tw.syncadaptor.saveTiddler will use fs.writeFile, because it detects there is a tiddler in the browser, so it believes there is a corresponding file in the fs, so it should just modify the file in the fs
  2. If fs.watch say the file is created or deleted ( eventType === 'rename' ), and fs.stat confirm that we are creating a file, then Bob try to sync from fs to browser use $tw.syncadaptor.saveTiddler same as above

But!

$tw.syncadaptor.saveTiddler found no such tiddler in the browser, so it guess there is no such file in the fs too.

So it will try to create a new tiddler in the fs. And there is already a tiddler with that name in the fs, so it append _1 to the name.

So the fix is very simple, we can just delete the file in the fs first, then use $tw.syncadaptor.saveTiddler, so it will create a tiddler with a correct file name.