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.

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

[Bug] Rename event is misinterpreted as two add events on macOS fsevent when changing case only #181

Closed
xyhuang7215 opened this issue Aug 27, 2024 · 4 comments
Assignees
Labels
bug Something isn't working

Comments

@xyhuang7215
Copy link
Contributor

xyhuang7215 commented Aug 27, 2024

Description:
I encountered an issue where a case-only rename event on macOS generates two Add events instead of a Rename event.
For instance, when renaming a.txt to A.txt, the system generates one Add a.txt event and one Add A.txt event.


Root Cause:

The root cause lies in the WatcherFSEvents::handleActions function:

if ( event.Flags & efswFSEventStreamEventFlagItemRenamed ) {
    if ( ( i + 1 < esize ) &&
    ( events[i + 1].Flags & efswFSEventStreamEventFlagItemRenamed ) &&
    ( events[i + 1].Id == event.Id + 1 ) ) {
            FSEvent& nEvent = events[i + 1];
            std::string newDir( FileSystem::pathRemoveFileName( nEvent.Path ) );
            std::string newFilepath( FileSystem::fileNameFromPath( nEvent.Path ) );
            // ...
    }
}

In my investigation, when a case-only rename occurs, the ID difference between the two rename events is 2.
The current implementation only checks for an ID difference of 1, causing the condition to fail, which ultimately leads to the rename events being misinterpreted as two Add events.
I tried to modify the condition to allow for an ID difference of 2, which allowed the rename event to be classified correctly.

However, this led to another issue where the source and destination paths were reversed (e.g., a.txt -> A.txt would be reported as Move A.txt to a.txt).
The root cause is that the event.Path still exists since only the case was changed:

if ( !FileInfo::exists( event.Path ) ) {
	sendFileAction( ID, dirPath, newFilepath, Actions::Moved,
					filePath );
} else {
	sendFileAction( ID, dirPath, filePath, Actions::Moved,
					newFilepath );
}

void WatcherFSEvents::sendFileAction( WatchID watchid, const std::string& dir,
	const std::string& filename, Action action,
        std::string oldFilename );

Proposed Solution:

To resolve the misclassification issue, I suggest modifying the condition to allow for an ID difference of 2.
For the reversed paths issue, we could add an additional case-insensitive comparison or just skip the FileInfo::exists check. (I'm unsure which specific case the below event corresponds to—it may be related to a previous workaround?).

@SpartanJ SpartanJ self-assigned this Aug 28, 2024
@SpartanJ SpartanJ added the bug Something isn't working label Aug 28, 2024
@SpartanJ
Copy link
Owner

SpartanJ commented Sep 3, 2024

Thanks for reporting it, i'll investigate a little further and try to find the best solution.

SpartanJ added a commit that referenced this issue Sep 3, 2024
@SpartanJ
Copy link
Owner

SpartanJ commented Sep 3, 2024

FSStream does provide information of the affected inode since macOS 10.13, I enabled that extended information and now it compares the inode with the subsequent events and seems to work fine (before it was using event ids which was unreliable and incorrect).

@SpartanJ SpartanJ closed this as completed Sep 3, 2024
xyhuang7215 added a commit to xyhuang7215/efsw that referenced this issue Sep 3, 2024
@xyhuang7215
Copy link
Contributor Author

Thanks for the quick response.
Leveraging inodes does seem like a better approach to determine whether subsequent events are related.

However, the issue where the source and destination paths are reversed is still unresolved. The case-insensitive filesystem ignores the case difference and considers the file to still exist when queried with the old path.

To address this, I implemented a simple fix to use strcasecmp. You can review the changes in this commit: xyhuang7215@92a9023.

Please let me know if you have any further suggestions.

SpartanJ added a commit that referenced this issue Sep 3, 2024
@SpartanJ
Copy link
Owner

SpartanJ commented Sep 3, 2024

Oh, thanks for reporting it, I was a little sleepy last night and missed the order. I applied your patch for the moment, I can't think of a better solution.

xyhuang7215 added a commit to xyhuang7215/efsw that referenced this issue Sep 4, 2024
xyhuang7215 added a commit to xyhuang7215/efsw that referenced this issue Sep 4, 2024
SpartanJ added a commit that referenced this issue Sep 4, 2024
Fix issue #181 (should validate cfInode)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants