Fix file mode handling (#39)

* Use proper replace logic

* Ensure generate is handled without unnecessary temp file

* Remove old TODO

* Add basic duplicate handling

* Correct error message

* Add duplicate error test case

* Suppress usage error when error is within processing

internal/cli/generate.go

@@ -33,14 +33,18 @@ func init() {
 
 func executeGenerate(cmd *cobra.Command, args []string) error {
 	// TODO: add some util func to hande all common error cases
+
 	if len(args) < 1 {
-		return errors.New("error: incorrect argument, you need to pass in an argument")
+		return errors.New("incorrect argument, you need to pass in an argument")
 	}
 
+	// Suppress usage message after this point
+	cmd.SilenceUsage = true
+
 	arg := args[0]
 	out := generateTargetFile
 	if err := generate(arg, out); err != nil {
-		return fmt.Errorf("error: handling generate, %v", err)
+		return fmt.Errorf("handling generate, %v", err)
 	}
 
 	return nil

internal/cli/preview.go

@@ -22,7 +22,6 @@ var (
 This allows you to find what the file looks like after ` + "`update`" + ` or ` + "`purge`" + `.
 `,
 		RunE: executePreview,
-		// TODO: Add flags to see only specific preview (e.g. `importer preview file --update` for update only view)
 		// TODO: Add support for diff preview
 	}
 	previewPurge   bool
@@ -38,10 +37,14 @@ func init() {
 
 func executePreview(cmd *cobra.Command, args []string) error {
 	// TODO: add some util func to hande all common error cases
+
 	if len(args) != 1 {
 		return errors.New("error: incorrect argument, you can only pass in 1 argument")
 	}
 
+	// Suppress usage message after this point
+	cmd.SilenceUsage = true
+
 	arg := args[0]
 	if err := preview(arg); err != nil {
 		return fmt.Errorf("error: handling preview, %v", err)

internal/cli/purge.go

@@ -35,6 +35,9 @@ func executePurge(cmd *cobra.Command, args []string) error {
 		return errors.New("missing file input")
 	}
 
+	// Suppress usage message after this point
+	cmd.SilenceUsage = true
+
 	for _, file := range args {
 		if err := purge(file); err != nil {
 			fmt.Printf("Warning: failed to purge for '%s', %v", file, err)

internal/cli/update.go

@@ -36,6 +36,9 @@ func executeUpdate(cmd *cobra.Command, args []string) error {
 		return errors.New("missing file input")
 	}
 
+	// Suppress usage message after this point
+	cmd.SilenceUsage = true
+
 	for _, file := range args {
 		if err := update(file); err != nil {
 			fmt.Printf("Warning: failed to generate for '%s', %v", file, err)

internal/file/replace.go

@@ -26,71 +26,42 @@ func WithForce() ReplaceOption {
 }
 
 // ReplaceWithAfter replaces the original file content with the processed
-// content. This is done by creating a temp file first, and replacing it.
-//
-// TODO: Ensure file mode is kept, or clarify in the comment.
+// content.
 func (f *File) ReplaceWithAfter(options ...ReplaceOption) error {
 	mode := &replaceMode{}
 	for _, opt := range options {
 		opt(mode)
 	}
 
-	file, err := os.CreateTemp("/tmp/", "importer_replace_*")
-	if err != nil {
-		return err // TODO: test coverage
-	}
-	defer file.Close()
-
-	_, err = file.Write(f.ContentAfter)
-	if err != nil {
-		return err // TODO: test coverage
-	}
-
-	// Exit early if dry-run
-	if mode.isDryRun {
-		return nil
-	}
-
-	err = os.Rename(file.Name(), f.FileName)
-	if err != nil {
-		return err // TODO: test coverage
-	}
-
-	return nil
+	return replace(f.FileName, f.ContentAfter, mode)
 }
 
 // ReplaceWithAfter replaces the original file content with the processed
-// content. This is done by creating a temp file first, and replacing it.
-//
-// TODO: Ensure file mode is kept, or clarify in the comment.
+// content.
 func (f *File) ReplaceWithPurged(options ...ReplaceOption) error {
 	mode := &replaceMode{}
 	for _, opt := range options {
 		opt(mode)
 	}
 
-	file, err := os.CreateTemp("/tmp/", "importer_replace_*")
-	if err != nil {
-		return err // TODO: test coverage
-	}
-	defer file.Close()
-
 	data := strings.Join(f.ContentPurged, "\n")
 	data = data + "\n" // Make sure to add new line at the end of the file
-	_, err = file.WriteString(data)
-	if err != nil {
-		return err // TODO: test coverage
-	}
 
+	return replace(f.FileName, []byte(data), mode)
+}
+
+func replace(fileName string, content []byte, mode *replaceMode) error {
 	// Exit early if dry-run
 	if mode.isDryRun {
 		return nil
 	}
 
-	err = os.Rename(file.Name(), f.FileName)
+	file, err := os.OpenFile(fileName, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0644)
 	if err != nil {
-		return err // TODO: test coverage
+		return err
 	}
+	defer file.Close()
 
-	return nil
+	_, err = file.Write(content)
+	return err
 }

internal/file/replace_test.go

@@ -12,7 +12,8 @@ import (
 
 func TestReplaceWithAfter(t *testing.T) {
 	cases := map[string]struct {
-		input *File
+		input   *File
+		options []ReplaceOption
 
 		// Output
 		want    string
@@ -29,18 +30,37 @@ func TestReplaceWithAfter(t *testing.T) {
 			},
 			want: "Completely different data",
 		},
+		"dry-run": {
+			input: &File{
+				FileName: "tmpfile.txt",
+				ContentBefore: []string{
+					"Some data",
+					"and more",
+				},
+				ContentAfter: []byte(`Completely different data`),
+			},
+			options: []ReplaceOption{
+				WithDryRun(),
+			},
+			want: "", // not written
+		},
 	}
 
 	for name, tc := range cases {
 		t.Run(name, func(t *testing.T) {
-			err := tc.input.ReplaceWithAfter()
+			_, err := os.Create(tc.input.FileName)
+			if err != nil {
+				t.Fatal(err)
+			}
+			defer os.Remove(tc.input.FileName)
+
+			err = tc.input.ReplaceWithAfter(tc.options...)
 			if err != nil {
 				if !errors.Is(err, tc.wantErr) {
 					t.Errorf("error did not match:\n    want: %v\n    got:  %v", tc.wantErr, err)
 				}
 				return
 			}
-			defer os.Remove(tc.input.FileName)
 
 			result := golden.FileAsString(t, tc.input.FileName)
 			if diff := cmp.Diff(tc.want, result); diff != "" {
@@ -52,7 +72,8 @@ func TestReplaceWithAfter(t *testing.T) {
 
 func TestReplaceWithPurged(t *testing.T) {
 	cases := map[string]struct {
-		input *File
+		input   *File
+		options []ReplaceOption
 
 		// Output
 		want    string
@@ -72,18 +93,41 @@ func TestReplaceWithPurged(t *testing.T) {
 			},
 			want: "Some purged data\n",
 		},
+
+		"dry-run": {
+			input: &File{
+				FileName: "tmpfile.txt",
+				ContentBefore: []string{
+					"Some data",
+					"and more",
+				},
+				ContentPurged: []string{
+					"Some purged data",
+				},
+				ContentAfter: []byte(`Completely different data`),
+			},
+			options: []ReplaceOption{
+				WithDryRun(),
+			},
+			want: "", // not written
+		},
 	}
 
 	for name, tc := range cases {
 		t.Run(name, func(t *testing.T) {
-			err := tc.input.ReplaceWithPurged()
+			_, err := os.Create(tc.input.FileName)
+			if err != nil {
+				t.Fatal(err)
+			}
+			defer os.Remove(tc.input.FileName)
+
+			err = tc.input.ReplaceWithPurged(tc.options...)
 			if err != nil {
 				if !errors.Is(err, tc.wantErr) {
 					t.Errorf("error did not match:\n    want: %v\n    got:  %v", tc.wantErr, err)
 				}
 				return
 			}
-			defer os.Remove(tc.input.FileName)
 
 			result := golden.FileAsString(t, tc.input.FileName)
 			if diff := cmp.Diff(tc.want, result); diff != "" {
@@ -92,3 +136,21 @@ func TestReplaceWithPurged(t *testing.T) {
 		})
 	}
 }
+
+func TestReplaceFail(t *testing.T) {
+	tmp := "test_file_for_replace_fail"
+
+	f, err := os.Create(tmp)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer os.Remove(tmp)
+
+	// Not writeable
+	f.Chmod(0444)
+
+	err = replace(tmp, []byte(`some data`), &replaceMode{})
+	if err == nil {
+		t.Fatal("should be permission error, but got no error")
+	}
+}

internal/file/write.go

@@ -2,25 +2,14 @@ package file
 
 import "os"
 
-// WriteAfterTo takes the processed content and copies into provided filepath.
-//
-// TODO: Ensure file mode is kept, or clarify in the comment.
+// WriteAfterTo writes the processed content to the provided filepath.
 func (f *File) WriteAfterTo(filepath string) error {
-	file, err := os.CreateTemp("/tmp/", "importer_replace_*")
+	file, err := os.OpenFile(filepath, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0644)
 	if err != nil {
-		return err // TODO: test coverage
+		return err
 	}
 	defer file.Close()
 
 	_, err = file.Write(f.ContentAfter)
-	if err != nil {
-		return err // TODO: test coverage
-	}
-
-	err = os.Rename(file.Name(), filepath)
-	if err != nil {
-		return err // TODO: test coverage
-	}
-
-	return nil
+	return err
 }

internal/parse/parse.go

@@ -42,6 +42,10 @@ type File struct {
 	Markers map[int]*marker.Marker
 }
 
+var (
+	ErrDuplicatedMarker = errors.New("duplicated marker within a single file")
+)
+
 // Parse reads filename and input, and parses data in the file.
 //
 // The steps are as follows:
@@ -149,7 +153,11 @@ func parse(markerRegex string, fileName string, input io.Reader) (*file.File, er
 		// track of already found match.
 		matchData := &marker.RawMarker{Name: subgroupName}
 		if data, found := rawMarkers[subgroupName]; found {
-			// TODO: Handle case where the same subgroup name gets used multiple times.
+			// If a marker with the same name has a pair already, return as an error.
+			// TODO: multiple 'begin' marker may still be a problem
+			if data.IsBeginFound && data.IsEndFound {
+				return nil, fmt.Errorf("%w, marker '%s' has been already processed", ErrDuplicatedMarker, subgroupName)
+			}
 			matchData = data
 		}
 

internal/parse/parse_error_test.go

@@ -5,6 +5,8 @@ import (
 	"io"
 	"strings"
 	"testing"
+
+	"github.com/upsidr/importer/internal/testingutil/golden"
 )
 
 func TestParseFail(t *testing.T) {
@@ -26,6 +28,11 @@ func TestParseFail(t *testing.T) {
 			input:    nil,
 			wantErr:  ErrNoInput,
 		},
+		"invalid marker setup": {
+			fileName: "dummy.md",
+			input:    strings.NewReader(golden.FileAsString(t, "./testdata/other/duplicated-marker.md")),
+			wantErr:  ErrDuplicatedMarker,
+		},
 	}
 
 	for name, tc := range cases {

internal/parse/testdata/other/duplicated-marker.md

@@ -0,0 +1,13 @@
+# Test Markdown
+
+<!-- == imptr: some_importer / begin from: ../../testdata/markdown/simple-before-importer.md#1~2 == -->
+
+some data between an annotation pair, which gets purged.
+
+<!-- == imptr: some_importer / end == -->
+
+<!-- == imptr: some_importer / begin from: ../../testdata/markdown/simple-before-importer.md#3~4 == -->
+
+another data
+
+<!-- == imptr: some_importer / end == -->