From e9303cec8da78038643e395ac9c49bda53382af1 Mon Sep 17 00:00:00 2001
From: Jarek Kowalski <jaak@jkowalski.net>
Date: Tue, 9 Nov 2021 08:39:47 -0800
Subject: [PATCH] upload: avoid reporting ignored entries twice (#1506)

Previously ignored entries were reported from the estimate goroutine
and from the main upload goroutine doubling each ignored counter in
the UI.
---
 snapshot/snapshotfs/upload.go      | 18 ++++++++-----
 snapshot/snapshotfs/upload_test.go | 41 ++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+), 7 deletions(-)

diff --git a/snapshot/snapshotfs/upload.go b/snapshot/snapshotfs/upload.go
index 6b5fee1bc..34a6376df 100644
--- a/snapshot/snapshotfs/upload.go
+++ b/snapshot/snapshotfs/upload.go
@@ -489,7 +489,7 @@ func (u *Uploader) uploadDirWithCheckpointing(ctx context.Context, rootDir fs.Di
 	}
 
 	if overrideDir != nil {
-		rootDir = u.wrapIgnorefs(uploadLog(ctx), overrideDir, policyTree)
+		rootDir = u.wrapIgnorefs(uploadLog(ctx), overrideDir, policyTree, true)
 	}
 
 	defer u.executeAfterFolderAction(ctx, "after-snapshot-root", policyTree.EffectivePolicy().Actions.AfterSnapshotRoot, localDirPathOrEmpty, &hc)
@@ -1132,7 +1132,7 @@ func uploadDirInternal(
 	defer u.executeAfterFolderAction(ctx, "after-folder", definedActions.AfterFolder, localDirPathOrEmpty, &hc)
 
 	if overrideDir != nil {
-		directory = u.wrapIgnorefs(uploadLog(ctx), overrideDir, policyTree)
+		directory = u.wrapIgnorefs(uploadLog(ctx), overrideDir, policyTree, true)
 	}
 
 	if de, err := uploadShallowDirInternal(ctx, directory, u); de != nil || err != nil {
@@ -1305,14 +1305,14 @@ func (u *Uploader) Upload(
 		go func() {
 			defer scanWG.Done()
 
-			wrapped := u.wrapIgnorefs(estimateLog(ctx), entry, policyTree)
+			wrapped := u.wrapIgnorefs(estimateLog(ctx), entry, policyTree, false /* reportIgnoreStats */)
 
 			ds, _ := u.scanDirectory(scanctx, wrapped, policyTree)
 
 			u.Progress.EstimatedDataSize(ds.numFiles, ds.totalFileSize)
 		}()
 
-		wrapped := u.wrapIgnorefs(uploadLog(ctx), entry, policyTree)
+		wrapped := u.wrapIgnorefs(uploadLog(ctx), entry, policyTree, true /* reportIgnoreStats */)
 
 		s.RootEntry, err = u.uploadDirWithCheckpointing(ctx, wrapped, policyTree, previousDirs, sourceInfo)
 
@@ -1338,7 +1338,7 @@ func (u *Uploader) Upload(
 	return s, nil
 }
 
-func (u *Uploader) wrapIgnorefs(logger logging.Logger, entry fs.Directory, policyTree *policy.Tree) fs.Directory {
+func (u *Uploader) wrapIgnorefs(logger logging.Logger, entry fs.Directory, policyTree *policy.Tree, reportIgnoreStats bool) fs.Directory {
 	if u.DisableIgnoreRules {
 		return entry
 	}
@@ -1350,14 +1350,18 @@ func (u *Uploader) wrapIgnorefs(logger logging.Logger, entry fs.Directory, polic
 				policyTree.EffectivePolicy().LoggingPolicy.Directories.Ignored.OrDefault(policy.LogDetailNone),
 				"ignored directory", fname, nil, nil, timetrack.StartTimer())
 
-			u.Progress.ExcludedDir(fname)
+			if reportIgnoreStats {
+				u.Progress.ExcludedDir(fname)
+			}
 		} else {
 			maybeLogEntryProcessed(
 				logger,
 				policyTree.EffectivePolicy().LoggingPolicy.Entries.Ignored.OrDefault(policy.LogDetailNone),
 				"ignored", fname, nil, nil, timetrack.StartTimer())
 
-			u.Progress.ExcludedFile(fname, md.Size())
+			if reportIgnoreStats {
+				u.Progress.ExcludedFile(fname, md.Size())
+			}
 		}
 
 		u.stats.AddExcluded(md)
diff --git a/snapshot/snapshotfs/upload_test.go b/snapshot/snapshotfs/upload_test.go
index 6dc89937d..ba91cb492 100644
--- a/snapshot/snapshotfs/upload_test.go
+++ b/snapshot/snapshotfs/upload_test.go
@@ -229,6 +229,47 @@ func TestUpload_TopLevelDirectoryReadFailure(t *testing.T) {
 	}
 }
 
+func TestUploadDoesNotReportProgressForIgnoredFilesTwice(t *testing.T) {
+	ctx := testlogging.Context(t)
+	th := newUploadTestHarness(ctx, t)
+
+	defer th.cleanup()
+
+	sourceDir := mockfs.NewDirectory()
+	sourceDir.AddFile("f1", []byte{1, 2, 3}, defaultPermissions)
+	sourceDir.AddFile("f2", []byte{1, 2, 3, 4}, defaultPermissions)
+	sourceDir.AddFile("f3", []byte{1, 2, 3, 4, 5}, defaultPermissions)
+
+	sourceDir.AddDir("d1", defaultPermissions)
+	sourceDir.AddFile("d1/f1", []byte{1, 2, 3}, defaultPermissions)
+
+	sourceDir.AddDir("d2", defaultPermissions)
+	sourceDir.AddFile("d2/f1", []byte{1, 2, 3}, defaultPermissions)
+	sourceDir.AddFile("d2/f2", []byte{1, 2, 3, 4}, defaultPermissions)
+
+	u := NewUploader(th.repo)
+	cup := &CountingUploadProgress{}
+	u.Progress = cup
+	u.OverrideEntryLogDetail = policy.NewLogDetail(10)
+	u.OverrideDirLogDetail = policy.NewLogDetail(10)
+
+	policyTree := policy.BuildTree(map[string]*policy.Policy{
+		".": {
+			FilesPolicy: policy.FilesPolicy{
+				IgnoreRules: []string{"d2", "f2"},
+			},
+		},
+	}, policy.DefaultPolicy)
+
+	_, err := u.Upload(ctx, sourceDir, policyTree, snapshot.SourceInfo{})
+	require.NoError(t, err)
+
+	// make sure ignored counter is only incremented by 1, even though we process each directory twice
+	// - once during estimation and once during upload.
+	require.EqualValues(t, 1, cup.counters.TotalExcludedFiles)
+	require.EqualValues(t, 1, cup.counters.TotalExcludedDirs)
+}
+
 func TestUpload_SubDirectoryReadFailureFailFast(t *testing.T) {
 	ctx := testlogging.Context(t)
 	th := newUploadTestHarness(ctx, t)