Repository: mbrt/gmailctl
Pull request #421 (2025-01-14 00:42:33)

Add context option for filter diffs
This adds a `--context` option to the `diff` subcommand and a `--diff-context` option to the `apply` and `edit` commands. The option allows the number of lines of diff context to be customized, which is helpful for verifying the correctness of changes to more complex filters.

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/mbrt/gmailctl/pull/421?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Michele+Bertasi) Report
Attention: Patch coverage is `31.81818%` with `15 lines` in your changes missing coverage. Please review.
> Project coverage is 54.52%. Comparing base [(`eb87774`)](https://app.codecov.io/gh/mbrt/gmailctl/commit/eb877748a94f7146310bc64bcc14ba663ef113b7?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Michele+Bertasi) to head [(`f1d5a5e`)](https://app.codecov.io/gh/mbrt/gmailctl/commit/f1d5a5e521568a142d5a26d739fcf32e511df03c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Michele+Bertasi).
> Report is 1 commits behind head on master.

| [Files with missing lines](https://app.codecov.io/gh/mbrt/gmailctl/pull/421?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Michele+Bertasi) | Patch % | Lines |
|---|---|---|
| [cmd/gmailctl/cmd/apply\_cmd.go](https://app.codecov.io/gh/mbrt/gmailctl/pull/421?src=pr&el=tree&filepath=cmd%2Fgmailctl%2Fcmd%2Fapply_cmd.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Michele+Bertasi#diff-Y21kL2dtYWlsY3RsL2NtZC9hcHBseV9jbWQuZ28=) | 0.00% | [5 Missing :warning: ](https://app.codecov.io/gh/mbrt/gmailctl/pull/421?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Michele+Bertasi) |
| [cmd/gmailctl/cmd/diff\_cmd.go](https://app.codecov.io/gh/mbrt/gmailctl/pull/421?src=pr&el=tree&filepath=cmd%2Fgmailctl%2Fcmd%2Fdiff_cmd.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Michele+Bertasi#diff-Y21kL2dtYWlsY3RsL2NtZC9kaWZmX2NtZC5nbw==) | 0.00% | [5 Missing :warning: ](https://app.codecov.io/gh/mbrt/gmailctl/pull/421?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Michele+Bertasi) |
| [cmd/gmailctl/cmd/edit\_cmd.go](https://app.codecov.io/gh/mbrt/gmailctl/pull/421?src=pr&el=tree&filepath=cmd%2Fgmailctl%2Fcmd%2Fedit_cmd.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Michele+Bertasi#diff-Y21kL2dtYWlsY3RsL2NtZC9lZGl0X2NtZC5nbw==) | 0.00% | [5 Missing :warning: ](https://app.codecov.io/gh/mbrt/gmailctl/pull/421?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Michele+Bertasi) |

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           master     #421      +/-   ##
==========================================
- Coverage   54.63%   54.52%   -0.12%     
==========================================
  Files          53       53              
  Lines        4448     4457       +9     
==========================================
  Hits         2430     2430              
- Misses       1877     1886       +9     
  Partials      141      141              
```

</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/mbrt/gmailctl/pull/421?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Michele+Bertasi).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Michele+Bertasi).


Patch:
diff --git a/cmd/gmailctl/cmd/apply_cmd.go b/cmd/gmailctl/cmd/apply_cmd.go
index 7dd38a3..fdd8d8c 100644
--- a/cmd/gmailctl/cmd/apply_cmd.go
+++ b/cmd/gmailctl/cmd/apply_cmd.go
@@ -15,6 +15,7 @@ var (
 	applyRemoveLabels bool
 	applySkipTests    bool
 	applyDebug        bool
+	applyDiffContext  int
 )
 
 const renameLabelWarning = `Warning: You are going to delete labels. This operation is
@@ -52,9 +53,14 @@ func init() {
 	applyCmd.Flags().BoolVarP(&applyRemoveLabels, "remove-labels", "r", false, "allow removing labels")
 	applyCmd.Flags().BoolVarP(&applySkipTests, "yolo", "", false, "skip configuration tests")
 	applyCmd.PersistentFlags().BoolVarP(&applyDebug, "debug", "", false, "print extra debugging information")
+	applyCmd.PersistentFlags().IntVarP(&applyDiffContext, "diff-context", "", papply.DefaultContextLines, "number of lines of filter diff context to show")
 }
 
 func apply(path string, interactive, test bool) error {
+	if applyDiffContext < 0 {
+		return errors.New("--diff-context must be non-negative")
+	}
+
 	parseRes, err := parseConfig(path, "", test)
 	if err != nil {
 		return err
@@ -70,7 +76,7 @@ func apply(path string, interactive, test bool) error {
 		return err
 	}
 
-	diff, err := papply.Diff(parseRes.Res.GmailConfig, upstream, applyDebug)
+	diff, err := papply.Diff(parseRes.Res.GmailConfig, upstream, applyDebug, applyDiffContext)
 	if err != nil {
 		return fmt.Errorf("cannot compare upstream with local config: %w", err)
 	}
diff --git a/cmd/gmailctl/cmd/diff_cmd.go b/cmd/gmailctl/cmd/diff_cmd.go
index fa9d2b0..12b918d 100644
--- a/cmd/gmailctl/cmd/diff_cmd.go
+++ b/cmd/gmailctl/cmd/diff_cmd.go
@@ -6,11 +6,13 @@ import (
 	"github.com/spf13/cobra"
 
 	papply "github.com/mbrt/gmailctl/internal/engine/apply"
+	"github.com/mbrt/gmailctl/internal/errors"
 )
 
 var (
 	diffFilename string
 	diffDebug    bool
+	diffContext  int
 )
 
 // diffCmd represents the diff command
@@ -39,9 +41,14 @@ func init() {
 	// Flags and configuration settings
 	diffCmd.PersistentFlags().StringVarP(&diffFilename, "filename", "f", "", "configuration file")
 	diffCmd.PersistentFlags().BoolVarP(&diffDebug, "debug", "", false, "print extra debugging information")
+	diffCmd.PersistentFlags().IntVarP(&diffContext, "context", "", papply.DefaultContextLines, "number of lines of filter diff context to show")
 }
 
 func diff(path string) error {
+	if diffContext < 0 {
+		return errors.New("--context must be non-negative")
+	}
+
 	parseRes, err := parseConfig(path, "", false)
 	if err != nil {
 		return err
@@ -57,7 +64,7 @@ func diff(path string) error {
 		return err
 	}
 
-	diff, err := papply.Diff(parseRes.Res.GmailConfig, upstream, diffDebug)
+	diff, err := papply.Diff(parseRes.Res.GmailConfig, upstream, diffDebug, diffContext)
 	if err != nil {
 		return fmt.Errorf("cannot compare upstream with local config: %w", err)
 	}
diff --git a/cmd/gmailctl/cmd/edit_cmd.go b/cmd/gmailctl/cmd/edit_cmd.go
index e9e74aa..7d307b1 100644
--- a/cmd/gmailctl/cmd/edit_cmd.go
+++ b/cmd/gmailctl/cmd/edit_cmd.go
@@ -17,9 +17,10 @@ import (
 
 // Parameters
 var (
-	editFilename  string
-	editSkipTests bool
-	editDebug     bool
+	editFilename    string
+	editSkipTests   bool
+	editDebug       bool
+	editDiffContext int
 )
 
 var (
@@ -70,9 +71,14 @@ func init() {
 	editCmd.PersistentFlags().StringVarP(&editFilename, "filename", "f", "", "configuration file")
 	editCmd.Flags().BoolVarP(&editSkipTests, "yolo", "", false, "skip configuration tests")
 	editCmd.PersistentFlags().BoolVarP(&editDebug, "debug", "", false, "print extra debugging information")
+	editCmd.PersistentFlags().IntVarP(&editDiffContext, "diff-context", "", papply.DefaultContextLines, "number of lines of filter diff context to show")
 }
 
 func edit(path string, test bool) error {
+	if editDiffContext < 0 {
+		return errors.New("--diff-context must be non-negative")
+	}
+
 	// First make sure that Gmail can be contacted, so that we don't
 	// waste the user's time editing a config file that cannot be
 	// applied now.
@@ -223,7 +229,7 @@ func applyEdited(path, originalPath string, test bool, gmailapi *api.GmailAPI) e
 		return err
 	}
 
-	diff, err := papply.Diff(parseRes.Res.GmailConfig, upstream, editDebug)
+	diff, err := papply.Diff(parseRes.Res.GmailConfig, upstream, editDebug, editDiffContext)
 	if err != nil {
 		return errors.New("comparing upstream with local config")
 	}
diff --git a/internal/engine/apply/apply.go b/internal/engine/apply/apply.go
index 288b769..bf87ce8 100644
--- a/internal/engine/apply/apply.go
+++ b/internal/engine/apply/apply.go
@@ -11,6 +11,9 @@ import (
 	"github.com/mbrt/gmailctl/internal/engine/parser"
 )
 
+// DefaultContextLines is the default number of lines of context to show in the filter diff.
+const DefaultContextLines = 5
+
 // GmailConfig represents a Gmail configuration.
 type GmailConfig struct {
 	Labels  label.Labels
@@ -113,13 +116,13 @@ func (d ConfigDiff) Validate() error {
 }
 
 // Diff computes the diff between local and upstream configuration.
-func Diff(local, upstream GmailConfig, debugInfo bool) (ConfigDiff, error) {
+func Diff(local, upstream GmailConfig, debugInfo bool, contextLines int) (ConfigDiff, error) {
 	res := ConfigDiff{
 		LocalConfig: local,
 	}
 	var err error
 
-	res.FiltersDiff, err = filter.Diff(upstream.Filters, local.Filters, debugInfo)
+	res.FiltersDiff, err = filter.Diff(upstream.Filters, local.Filters, debugInfo, contextLines)
 	if err != nil {
 		return res, fmt.Errorf("cannot compute filters diff: %w", err)
 	}
diff --git a/internal/engine/filter/diff.go b/internal/engine/filter/diff.go
index 51057dc..2b10245 100644
--- a/internal/engine/filter/diff.go
+++ b/internal/engine/filter/diff.go
@@ -14,12 +14,12 @@ import (
 // Diff computes the diff between two lists of filters.
 //
 // To compute the diff, IDs are ignored, only the contents of the filters are actually considered.
-func Diff(upstream, local Filters, debugInfo bool) (FiltersDiff, error) {
+func Diff(upstream, local Filters, debugInfo bool, contextLines int) (FiltersDiff, error) {
 	// Computing the diff is very expensive, so we have to minimize the number of filters
 	// we have to analyze. To do so, we get rid of the filters that are exactly the same,
 	// by hashing them.
 	added, removed := changedFilters(upstream, local)
-	return NewMinimalFiltersDiff(added, removed, debugInfo), nil
+	return NewMinimalFiltersDiff(added, removed, debugInfo, contextLines), nil
 }
 
 // NewMinimalFiltersDiff creates a new FiltersDiff with reordered filters, where
@@ -28,11 +28,11 @@ func Diff(upstream, local Filters, debugInfo bool) (FiltersDiff, error) {
 // The algorithm used is a quadratic approximation to the otherwise NP-complete
 // travel salesman problem. Hopefully the number of filters is low enough to
 // make this not too slow and the approximation not too bad.
-func NewMinimalFiltersDiff(added, removed Filters, printDebugInfo bool) FiltersDiff {
+func NewMinimalFiltersDiff(added, removed Filters, printDebugInfo bool, contextLines int) FiltersDiff {
 	if len(added) > 0 && len(removed) > 0 {
 		added, removed = reorderWithHungarian(added, removed)
 	}
-	return FiltersDiff{added, removed, printDebugInfo}
+	return FiltersDiff{added, removed, printDebugInfo, contextLines}
 }
 
 // FiltersDiff contains filters that have been added and removed locally with respect to upstream.
@@ -40,6 +40,7 @@ type FiltersDiff struct {
 	Added          Filters
 	Removed        Filters
 	PrintDebugInfo bool
+	ContextLines   int
 }
 
 // Empty returns true if the diff is empty.
@@ -62,7 +63,7 @@ func (f FiltersDiff) String() string {
 		B:        difflib.SplitLines(added),
 		FromFile: "Current",
 		ToFile:   "TO BE APPLIED",
-		Context:  5,
+		Context:  f.ContextLines,
 	})
 	if err != nil {
 		// We can't get a diff apparently, let's make something up here


Test patch:
diff --git a/integration_test.go b/integration_test.go
index 7d5cfa1..8f1a0d8 100644
--- a/integration_test.go
+++ b/integration_test.go
@@ -68,7 +68,7 @@ func TestIntegration(t *testing.T) {
 			require.Nil(t, err)
 
 			// Apply the diff.
-			d, err := apply.Diff(pres.GmailConfig, upres, false)
+			d, err := apply.Diff(pres.GmailConfig, upres, false, apply.DefaultContextLines)
 			require.Nil(t, err)
 			err = apply.Apply(d, gapi, true)
 			require.Nil(t, err)
@@ -139,7 +139,7 @@ func TestIntegrationImportExport(t *testing.T) {
 			require.Nil(t, err)
 
 			// Apply the diff.
-			d, err := apply.Diff(pres.GmailConfig, upres, false)
+			d, err := apply.Diff(pres.GmailConfig, upres, false, apply.DefaultContextLines)
 			require.Nil(t, err)
 			err = apply.Apply(d, gapi, true)
 			require.Nil(t, err)
@@ -174,10 +174,10 @@ func TestIntegrationImportExport(t *testing.T) {
 }
 
 func assertEmptyDiff(t *testing.T, local, remote apply.GmailConfig) {
-	d, err := apply.Diff(local, remote, false)
+	d, err := apply.Diff(local, remote, false, -1)
 	require.Nil(t, err)
-	assert.Empty(t, d.FiltersDiff)
-	assert.Empty(t, d.LabelsDiff)
+	assert.True(t, d.FiltersDiff.Empty())
+	assert.True(t, d.LabelsDiff.Empty())
 }
 
 func mustParseTime(layout, value string) time.Time {
diff --git a/internal/engine/filter/diff_test.go b/internal/engine/filter/diff_test.go
index 94dd858..84f59cd 100644
--- a/internal/engine/filter/diff_test.go
+++ b/internal/engine/filter/diff_test.go
@@ -9,6 +9,8 @@ import (
 	"github.com/mbrt/gmailctl/internal/engine/gmail"
 )
 
+const contextLines = 5
+
 func TestNoDiff(t *testing.T) {
 	prev := Filters{
 		{
@@ -32,7 +34,7 @@ func TestNoDiff(t *testing.T) {
 		},
 	}
 
-	fd, err := Diff(prev, curr, false)
+	fd, err := Diff(prev, curr, false, contextLines)
 	assert.Nil(t, err)
 	// No difference even if the ID is present in only one of them.
 	assert.True(t, fd.Empty())
@@ -65,7 +67,7 @@ func TestDiffOutput(t *testing.T) {
 		},
 	}
 
-	fd, err := Diff(prev, curr, false)
+	fd, err := Diff(prev, curr, false, contextLines)
 	assert.Nil(t, err)
 
 	expected := `
@@ -92,6 +94,57 @@ func TestDiffOutput(t *testing.T) {
 	assert.Equal(t, strings.TrimSpace(fd.String()), strings.TrimSpace(expected))
 }
 
+func TestDiffOutputWithCustomContextLines(t *testing.T) {
+	prev := Filters{
+		{
+			ID: "abcdefg",
+			Criteria: Criteria{
+				From:  "someone@gmail.com",
+				Query: "(a b) subject:(foo bar)",
+			},
+			Action: Actions{
+				MarkRead: true,
+				Category: gmail.CategoryPersonal,
+			},
+		},
+	}
+	curr := Filters{
+		{
+			Criteria: Criteria{
+				From:  "{someone@gmail.com else@gmail.com}",
+				Query: "(a c) subject:(foo baz)",
+			},
+			Action: Actions{
+				MarkRead: true,
+				Category: gmail.CategoryPersonal,
+			},
+		},
+	}
+
+	fd, err := Diff(prev, curr, false, 1 /* contextLines */)
+	assert.Nil(t, err)
+
+	expected := `
+--- Current
++++ TO BE APPLIED
+@@ -1,3 +1,3 @@
+ * Criteria:
+-    from: someone@gmail.com
++    from: {someone@gmail.com else@gmail.com}
+     query: 
+@@ -5,3 +5,3 @@
+         a
+-        b
++        c
+       )
+@@ -9,3 +9,3 @@
+         foo
+-        bar
++        baz
+       )`
+	assert.Equal(t, strings.TrimSpace(fd.String()), strings.TrimSpace(expected))
+}
+
 func TestDiffOutputWithGmailSearchURL(t *testing.T) {
 	prev := Filters{
 		{
@@ -119,7 +172,7 @@ func TestDiffOutputWithGmailSearchURL(t *testing.T) {
 		},
 	}
 
-	fd, err := Diff(prev, curr, true)
+	fd, err := Diff(prev, curr, true, contextLines)
 	assert.Nil(t, err)
 
 	expected := `
@@ -213,10 +266,11 @@ func TestDiffAddRemove(t *testing.T) {
 		},
 	}
 
-	fd, err := Diff(prev, curr, false)
+	fd, err := Diff(prev, curr, false, contextLines)
 	expected := FiltersDiff{
-		Added:   Filters{curr[0]},
-		Removed: Filters{prev[1]},
+		Added:        Filters{curr[0]},
+		Removed:      Filters{prev[1]},
+		ContextLines: contextLines,
 	}
 	assert.Nil(t, err)
 	assert.Equal(t, expected, fd)
@@ -252,7 +306,7 @@ func TestDiffReorder(t *testing.T) {
 		},
 	}
 
-	fd, err := Diff(prev, curr, false)
+	fd, err := Diff(prev, curr, false, contextLines)
 	assert.Nil(t, err)
 	assert.Len(t, fd.Added, 0)
 	assert.Len(t, fd.Removed, 0)
@@ -288,10 +342,11 @@ func TestDiffModify(t *testing.T) {
 		},
 	}
 
-	fd, err := Diff(prev, curr, false)
+	fd, err := Diff(prev, curr, false, contextLines)
 	expected := FiltersDiff{
-		Added:   Filters{curr[1]},
-		Removed: Filters{prev[1]},
+		Added:        Filters{curr[1]},
+		Removed:      Filters{prev[1]},
+		ContextLines: contextLines,
 	}
 	assert.Nil(t, err)
 	assert.Equal(t, expected, fd)
@@ -336,9 +391,10 @@ func TestDiffAdd(t *testing.T) {
 		},
 	}
 
-	fd, err := Diff(prev, curr, false)
+	fd, err := Diff(prev, curr, false, contextLines)
 	expected := FiltersDiff{
-		Added: Filters{curr[2]},
+		Added:        Filters{curr[2]},
+		ContextLines: contextLines,
 	}
 	assert.Nil(t, err)
 	assert.Equal(t, expected, fd)
@@ -358,9 +414,10 @@ func TestDiffRemove(t *testing.T) {
 		},
 	}
 
-	fd, err := Diff(prev, curr, false)
+	fd, err := Diff(prev, curr, false, contextLines)
 	expected := FiltersDiff{
-		Removed: Filters{prev[2], prev[0]},
+		Removed:      Filters{prev[2], prev[0]},
+		ContextLines: contextLines,
 	}
 
 	assert.Nil(t, err)
@@ -388,7 +445,7 @@ func TestDuplicate(t *testing.T) {
 		},
 	}
 
-	fd, err := Diff(prev, curr, false)
+	fd, err := Diff(prev, curr, false, contextLines)
 	assert.Nil(t, err)
 	// Only one of the two identical filters is present
 	assert.Equal(t, curr[1:], fd.Added)