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

Do not suppress Chmod on non-existent file #35

Merged
merged 1 commit into from Mar 6, 2024
Merged

Do not suppress Chmod on non-existent file #35

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 6 additions & 6 deletions inotify.go
View file
Open in desktop
Expand Up @@ -317,12 +317,12 @@ func (e *Event) ignoreLinux(mask uint32) bool {
return true
}

// If the event is not a DELETE or RENAME, the file must exist.
// Otherwise the event is ignored.
// *Note*: this was put in place because it was seen that a MODIFY
// event was sent after the DELETE. This ignores that MODIFY and
// assumes a DELETE will come or has come if the file doesn't exist.
if !(e.Op&Remove == Remove || e.Op&Rename == Rename) {
// If the event is Create or Write, the file must exist, or the
// event will be suppressed.
// *Note*: this was put in place because it was seen that a Write
// event was sent after the Remove. This ignores the Write and
// assumes a Remove will come or has come if the file doesn't exist.
if e.Op&Create != 0 || e.Op&Write != 0 {
Comment on lines +320 to +325
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The update to the ignoreLinux function to focus on suppressing Create or Write events for non-existent files aligns well with the PR's objectives. This change ensures that Chmod events, which can indicate a file's removal, are not suppressed, enhancing the library's ability to detect file deletions.

One minor suggestion for improvement is to clarify the comment further by explicitly stating that Chmod events are not suppressed by this logic, making it clear for future maintainers why this specific change was made.

	// If the event is Create or Write, the file must exist, or the
	// event will be suppressed. This logic specifically allows Chmod
	// events, which can indicate a file's removal, to be processed.
	// *Note*: This was put in place because it was observed that a Write
	// event could be sent after the Remove event. This logic ignores the Write
	// and assumes a Remove will come or has come if the file doesn't exist.
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
// If the event is Create or Write, the file must exist, or the
// event will be suppressed.
// *Note*: this was put in place because it was seen that a Write
// event was sent after the Remove. This ignores the Write and
// assumes a Remove will come or has come if the file doesn't exist.
if e.Op&Create != 0 || e.Op&Write != 0 {
// If the event is Create or Write, the file must exist, or the
// event will be suppressed. This logic specifically allows Chmod
// events, which can indicate a file's removal, to be processed.
// *Note*: This was put in place because it was observed that a Write
// event could be sent after the Remove event. This logic ignores the Write
// and assumes a Remove will come or has come if the file doesn't exist.
if e.Op&Create != 0 || e.Op&Write != 0 {

_, statErr := os.Lstat(e.Name)
return os.IsNotExist(statErr)
}
Expand Down
49 changes: 49 additions & 0 deletions inotify_test.go
View file
Open in desktop
Expand Up @@ -496,3 +496,52 @@ func TestInotifyWatchList(t *testing.T) {
}
}
}

func TestInotifyDeleteOpenedFile(t *testing.T) {
testDir := tempMkdir(t)
defer os.RemoveAll(testDir)

testFile := filepath.Join(testDir, "testfile")

// create and open a file
fd, err := os.Create(testFile)
if err != nil {
t.Fatalf("Create failed: %v", err)
}
defer fd.Close()

w, err := NewWatcher()
if err != nil {
t.Fatalf("Failed to create watcher: %v", err)
}
defer w.Close()

err = w.Add(testFile)
if err != nil {
t.Fatalf("Failed to add watch for %s: %v", testFile, err)
}

checkEvent := func(exp Op) {
select {
case event := <-w.Events:
t.Logf("Event received: %s", event.Op)
if event.Op != exp {
t.Fatalf("Event expected: %s, got: %s", exp, event.Op)
}
case <-time.After(100 * time.Millisecond):
t.Fatalf("Expected %s event not received", exp)
}
}

// Remove the (opened) file, check Chmod event (notifying
// about file link count change) is received
err = os.Remove(testFile)
if err != nil {
t.Fatalf("Failed to remove file: %s", err)
}
checkEvent(Chmod)

// Close the file, check Remove event is received
fd.Close()
checkEvent(Remove)
}
Comment on lines +499 to +547
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The TestInotifyDeleteOpenedFile function is well-implemented and effectively tests the delivery of Chmod and Remove events upon file deletion. This aligns with the PR's objectives and enhances the test coverage for the fsnotify library.

One suggestion for improvement is to ensure that the watcher (w) is closed even if the test fails before reaching the defer w.Close() statement. This can be achieved by moving the watcher creation and defer statement to the beginning of the test function.

+	w, err := NewWatcher()
+	if err != nil {
+		t.Fatalf("Failed to create watcher: %v", err)
+	}
+	defer w.Close()
+
	fd, err := os.Create(testFile)
	if err != nil {
		t.Fatalf("Create failed: %v", err)
	}
	defer fd.Close()
-	w, err := NewWatcher()
-	if err != nil {
-		t.Fatalf("Failed to create watcher: %v", err)
-	}
-	defer w.Close()

This change ensures that the watcher is always properly closed, preventing potential resource leaks during test execution.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
func TestInotifyDeleteOpenedFile(t *testing.T) {
testDir := tempMkdir(t)
defer os.RemoveAll(testDir)
testFile := filepath.Join(testDir, "testfile")
// create and open a file
fd, err := os.Create(testFile)
if err != nil {
t.Fatalf("Create failed: %v", err)
}
defer fd.Close()
w, err := NewWatcher()
if err != nil {
t.Fatalf("Failed to create watcher: %v", err)
}
defer w.Close()
err = w.Add(testFile)
if err != nil {
t.Fatalf("Failed to add watch for %s: %v", testFile, err)
}
checkEvent := func(exp Op) {
select {
case event := <-w.Events:
t.Logf("Event received: %s", event.Op)
if event.Op != exp {
t.Fatalf("Event expected: %s, got: %s", exp, event.Op)
}
case <-time.After(100 * time.Millisecond):
t.Fatalf("Expected %s event not received", exp)
}
}
// Remove the (opened) file, check Chmod event (notifying
// about file link count change) is received
err = os.Remove(testFile)
if err != nil {
t.Fatalf("Failed to remove file: %s", err)
}
checkEvent(Chmod)
// Close the file, check Remove event is received
fd.Close()
checkEvent(Remove)
}
func TestInotifyDeleteOpenedFile(t *testing.T) {
testDir := tempMkdir(t)
defer os.RemoveAll(testDir)
testFile := filepath.Join(testDir, "testfile")
w, err := NewWatcher()
if err != nil {
t.Fatalf("Failed to create watcher: %v", err)
}
defer w.Close()
// create and open a file
fd, err := os.Create(testFile)
if err != nil {
t.Fatalf("Create failed: %v", err)
}
defer fd.Close()
err = w.Add(testFile)
if err != nil {
t.Fatalf("Failed to add watch for %s: %v", testFile, err)
}
checkEvent := func(exp Op) {
select {
case event := <-w.Events:
t.Logf("Event received: %s", event.Op)
if event.Op != exp {
t.Fatalf("Event expected: %s, got: %s", exp, event.Op)
}
case <-time.After(100 * time.Millisecond):
t.Fatalf("Expected %s event not received", exp)
}
}
// Remove the (opened) file, check Chmod event (notifying
// about file link count change) is received
err = os.Remove(testFile)
if err != nil {
t.Fatalf("Failed to remove file: %s", err)
}
checkEvent(Chmod)
// Close the file, check Remove event is received
fd.Close()
checkEvent(Remove)
}