From 4fbbec7e05608a4a49ded7414181dff5c28a12f4 Mon Sep 17 00:00:00 2001 From: Simon Let Date: Fri, 16 Dec 2022 00:05:51 +0100 Subject: [PATCH] use record.V1 more, housekeeping spelling error handling improvements logging ci --- .github/workflows/release.yaml | 2 +- cmd/install-utils/main.go | 2 +- cmd/install-utils/migrate.go | 24 +++++-------- internal/cfg/cfg.go | 12 +++---- internal/cfg/migrate.go | 17 ++++----- internal/device/device.go | 1 + internal/futil/futil.go | 11 +++--- internal/histcli/histcli.go | 10 ++++-- internal/histfile/histfile.go | 18 +++++----- internal/histio/file.go | 4 +-- internal/histio/histio.go | 3 +- internal/recio/read.go | 62 +++++++++++++++++---------------- internal/recio/write.go | 11 ------ internal/recordint/flag.go | 9 ----- internal/recordint/indexed.go | 11 ------ internal/recordint/searchapp.go | 41 +++++++++++----------- internal/searchapp/test.go | 4 +-- record/v1.go | 3 ++ 18 files changed, 107 insertions(+), 138 deletions(-) delete mode 100644 internal/recordint/flag.go delete mode 100644 internal/recordint/indexed.go diff --git a/.github/workflows/release.yaml b/.github/workflows/release.yaml index 17e739f..32194f3 100644 --- a/.github/workflows/release.yaml +++ b/.github/workflows/release.yaml @@ -18,7 +18,7 @@ jobs: fetch-depth: 0 - name: Get Go version - run: echo "GO_VERSION=$(grep '^go ' go.mod | cut -d ' ' -f 2)" >> $GITHUB_ENV + run: echo "GO_VERSION=$(grep '^go ' go.mod | cut -d ' ' -f 2)" >> $GITHUB_ENV && cat $GITHUB_ENV - name: Set up Go uses: actions/setup-go@v3 diff --git a/cmd/install-utils/main.go b/cmd/install-utils/main.go index f19b283..69af0df 100644 --- a/cmd/install-utils/main.go +++ b/cmd/install-utils/main.go @@ -44,7 +44,7 @@ func main() { case "rollback": rollback() case "migrate-config": - migrateConfig() + migrateConfig(out) case "migrate-history": migrateHistory(out) case "setup-device": diff --git a/cmd/install-utils/migrate.go b/cmd/install-utils/migrate.go index 0336f98..5154cd8 100644 --- a/cmd/install-utils/migrate.go +++ b/cmd/install-utils/migrate.go @@ -10,22 +10,19 @@ import ( "github.com/curusarn/resh/internal/futil" "github.com/curusarn/resh/internal/output" "github.com/curusarn/resh/internal/recio" - "github.com/curusarn/resh/record" ) -func migrateConfig() { +func migrateConfig(out *output.Output) { err := cfg.Touch() if err != nil { - fmt.Fprintf(os.Stderr, "ERROR: Failed to touch config file: %v\n", err) - os.Exit(1) + out.Fatal("ERROR: Failed to touch config file", err) } changes, err := cfg.Migrate() if err != nil { - fmt.Fprintf(os.Stderr, "ERROR: Failed to update config file: %v\n", err) - os.Exit(1) + out.Fatal("ERROR: Failed to update config file", err) } if changes { - fmt.Printf("RESH config file format has changed since last update - your config was updated to reflect the changes.\n") + out.Info("RESH config file format has changed since last update - your config was updated to reflect the changes.") } } @@ -34,6 +31,8 @@ func migrateHistory(out *output.Output) { migrateHistoryFormat(out) } +// find first existing history and use it +// don't bother with merging of history in multiple locations - it could get messy and it shouldn't be necessary func migrateHistoryLocation(out *output.Output) { dataDir, err := datadir.MakePath() if err != nil { @@ -92,9 +91,9 @@ func migrateHistoryFormat(out *output.Output) { } if !exists { out.ErrorWOErr("There is no history file - this is normal if you are installing RESH for the first time on this device") - err = futil.CreateFile(historyPath) + err = futil.TouchFile(historyPath) if err != nil { - out.Fatal("ERROR: Failed to create history file", err) + out.Fatal("ERROR: Failed to touch history file", err) } os.Exit(0) } @@ -110,12 +109,7 @@ func migrateHistoryFormat(out *output.Output) { if err != nil { out.Fatal("ERROR: Could not load history file", err) } - // TODO: get rid of this conversion - var recsV1 []record.V1 - for _, rec := range recs { - recsV1 = append(recsV1, rec.Rec) - } - err = rio.OverwriteFile(historyPath, recsV1) + err = rio.OverwriteFile(historyPath, recs) if err != nil { out.Error("ERROR: Could not update format of history file", err) diff --git a/internal/cfg/cfg.go b/internal/cfg/cfg.go index 76bd73b..c41affd 100644 --- a/internal/cfg/cfg.go +++ b/internal/cfg/cfg.go @@ -46,8 +46,8 @@ type Config struct { Debug bool // SessionWatchPeriodSeconds is how often should daemon check if terminal // sessions are still alive - // There is not much need to adjust the value both memory overhead of watched sessions - // and the CPU overhead of chacking them are relatively low + // There is not much need to adjust the value because both memory overhead of watched sessions + // and the CPU overhead of checking them are quite low SessionWatchPeriodSeconds uint // ReshHistoryMinSize is how large resh history needs to be for // daemon to ignore standard shell history files @@ -72,11 +72,11 @@ const headerComment = `## ## RESH config (v1) ## ###################### ## Here you can find info about RESH configuration options. -## You can uncomment the options and custimize them. +## You can uncomment the options and customize them. ## Required. ## The config format can change in future versions. -## ConfigVersion helps us seemlessly upgrade to the new formats. +## ConfigVersion helps us seamlessly upgrade to the new formats. # ConfigVersion = "v1" ## Port used by RESH daemon and rest of the components to communicate. @@ -88,7 +88,7 @@ const headerComment = `## ## Options: "debug", "info", "warn", "error", "fatal" # LogLevel = "info" -## When BindControlR is "true" RESH search app is bound to CTRL+R on terminal startuA +## When BindControlR is "true" RESH search app is bound to CTRL+R on terminal startup # BindControlR = true ## When Debug is "true" the RESH search app runs in debug mode. @@ -101,7 +101,7 @@ const headerComment = `## # SessionWatchPeriodSeconds = 600 ## When RESH is first installed there is no RESH history so there is nothing to search. -## As a temporary woraround, RESH daemon parses bash/zsh shell history and searches it. +## As a temporary workaround, RESH daemon parses bash/zsh shell history and searches it. ## Once RESH history is big enough RESH stops using bash/zsh history. ## ReshHistoryMinSize controls how big RESH history needs to be before this happens. ## You can increase this this to e.g. 10000 to get RESH to use bash/zsh history longer. diff --git a/internal/cfg/migrate.go b/internal/cfg/migrate.go index fbbb214..2933e36 100644 --- a/internal/cfg/migrate.go +++ b/internal/cfg/migrate.go @@ -5,21 +5,18 @@ import ( "os" "github.com/BurntSushi/toml" + "github.com/curusarn/resh/internal/futil" ) // Touch config file func Touch() error { - path, err := getConfigPath() + fpath, err := getConfigPath() if err != nil { return fmt.Errorf("could not get config file path: %w", err) } - file, err := os.OpenFile(path, os.O_CREATE|os.O_APPEND|os.O_WRONLY, 0666) + err = futil.TouchFile(fpath) if err != nil { - return fmt.Errorf("could not open/create config file: %w", err) - } - err = file.Close() - if err != nil { - return fmt.Errorf("could not close config file: %w", err) + return fmt.Errorf("could not touch config file: %w", err) } return nil } @@ -27,11 +24,11 @@ func Touch() error { // Migrate old config versions to current config version // returns true if any changes were made to the config func Migrate() (bool, error) { - path, err := getConfigPath() + fpath, err := getConfigPath() if err != nil { return false, fmt.Errorf("could not get config file path: %w", err) } - configF, err := readConfig(path) + configF, err := readConfig(fpath) if err != nil { return false, fmt.Errorf("could not read config: %w", err) } @@ -50,7 +47,7 @@ func Migrate() (bool, error) { if *configF.ConfigVersion != current { return false, fmt.Errorf("unrecognized config version: '%s'", *configF.ConfigVersion) } - err = writeConfig(configF, path) + err = writeConfig(configF, fpath) if err != nil { return true, fmt.Errorf("could not write migrated config: %w", err) } diff --git a/internal/device/device.go b/internal/device/device.go index e1857c1..3bc49bf 100644 --- a/internal/device/device.go +++ b/internal/device/device.go @@ -1,3 +1,4 @@ +// device implements helpers that get/set device config files package device import ( diff --git a/internal/futil/futil.go b/internal/futil/futil.go index 93b542d..c85e98e 100644 --- a/internal/futil/futil.go +++ b/internal/futil/futil.go @@ -1,3 +1,4 @@ +// futil implements common file-related utilities package futil import ( @@ -41,14 +42,14 @@ func FileExists(fpath string) (bool, error) { return false, fmt.Errorf("could not stat file: %w", err) } -func CreateFile(fpath string) error { - ff, err := os.Create(fpath) +func TouchFile(fpath string) error { + file, err := os.OpenFile(fpath, os.O_CREATE|os.O_APPEND|os.O_WRONLY, 0666) if err != nil { - return err + return fmt.Errorf("could not open/create file: %w", err) } - err = ff.Close() + err = file.Close() if err != nil { - return err + return fmt.Errorf("could not close file: %w", err) } return nil } diff --git a/internal/histcli/histcli.go b/internal/histcli/histcli.go index 8069990..0b75dec 100644 --- a/internal/histcli/histcli.go +++ b/internal/histcli/histcli.go @@ -2,22 +2,26 @@ package histcli import ( "github.com/curusarn/resh/internal/recordint" + "github.com/curusarn/resh/record" + "go.uber.org/zap" ) // Histcli is a dump of history preprocessed for resh cli purposes type Histcli struct { // list of records List []recordint.SearchApp + + sugar *zap.SugaredLogger } // New Histcli -func New() Histcli { +func New(sugar *zap.SugaredLogger) Histcli { return Histcli{} } // AddRecord to the histcli -func (h *Histcli) AddRecord(rec *recordint.Indexed) { - cli := recordint.NewSearchApp(rec) +func (h *Histcli) AddRecord(rec *record.V1) { + cli := recordint.NewSearchApp(h.sugar, rec) h.List = append(h.List, cli) } diff --git a/internal/histfile/histfile.go b/internal/histfile/histfile.go index 33a2279..d4d3a7b 100644 --- a/internal/histfile/histfile.go +++ b/internal/histfile/histfile.go @@ -35,7 +35,7 @@ type Histfile struct { rio *recio.RecIO } -// New creates new histfile and runs its gorutines +// New creates new histfile and runs its goroutines func New(sugar *zap.SugaredLogger, input chan recordint.Collect, sessionsToDrop chan string, reshHistoryPath string, bashHistoryPath string, zshHistoryPath string, maxInitHistSize int, minInitHistSizeKB int, @@ -48,7 +48,7 @@ func New(sugar *zap.SugaredLogger, input chan recordint.Collect, sessionsToDrop historyPath: reshHistoryPath, bashCmdLines: histlist.New(sugar), zshCmdLines: histlist.New(sugar), - cliRecords: histcli.New(), + cliRecords: histcli.New(sugar), rio: &rio, } go hf.loadHistory(bashHistoryPath, zshHistoryPath, maxInitHistSize, minInitHistSizeKB) @@ -58,7 +58,7 @@ func New(sugar *zap.SugaredLogger, input chan recordint.Collect, sessionsToDrop } // load records from resh history, reverse, enrich and save -func (h *Histfile) loadCliRecords(recs []recordint.Indexed) { +func (h *Histfile) loadCliRecords(recs []record.V1) { for _, cmdline := range h.bashCmdLines.List { h.cliRecords.AddCmdLine(cmdline) } @@ -218,17 +218,15 @@ func (h *Histfile) mergeAndWriteRecord(sugar *zap.SugaredLogger, part1 recordint return } + recV1 := record.V1(rec) func() { cmdLine := rec.CmdLine h.bashCmdLines.AddCmdLine(cmdLine) h.zshCmdLines.AddCmdLine(cmdLine) - h.cliRecords.AddRecord(&recordint.Indexed{ - // TODO: is this what we want? - Rec: rec, - }) + h.cliRecords.AddRecord(&recV1) }() - h.rio.AppendToFile(h.historyPath, []record.V1{rec}) + h.rio.AppendToFile(h.historyPath, []record.V1{recV1}) } // TODO: use errors in RecIO @@ -261,13 +259,13 @@ func (h *Histfile) DumpCliRecords() histcli.Histcli { return h.cliRecords } -func loadCmdLines(sugar *zap.SugaredLogger, recs []recordint.Indexed) histlist.Histlist { +func loadCmdLines(sugar *zap.SugaredLogger, recs []record.V1) histlist.Histlist { hl := histlist.New(sugar) // go from bottom and deduplicate var cmdLines []string cmdLinesSet := map[string]bool{} for i := len(recs) - 1; i >= 0; i-- { - cmdLine := recs[i].Rec.CmdLine + cmdLine := recs[i].CmdLine if cmdLinesSet[cmdLine] { continue } diff --git a/internal/histio/file.go b/internal/histio/file.go index 5233717..40f2544 100644 --- a/internal/histio/file.go +++ b/internal/histio/file.go @@ -6,7 +6,7 @@ import ( "sync" "github.com/curusarn/resh/internal/recio" - "github.com/curusarn/resh/internal/recordint" + "github.com/curusarn/resh/record" "go.uber.org/zap" ) @@ -16,7 +16,7 @@ type histfile struct { path string mu sync.RWMutex - data []recordint.Indexed + data []record.V1 fileinfo os.FileInfo } diff --git a/internal/histio/histio.go b/internal/histio/histio.go index d541f9d..143727e 100644 --- a/internal/histio/histio.go +++ b/internal/histio/histio.go @@ -3,7 +3,6 @@ package histio import ( "path" - "github.com/curusarn/resh/internal/recordint" "github.com/curusarn/resh/record" "go.uber.org/zap" ) @@ -18,7 +17,7 @@ type Histio struct { // moreHistories map[string]*histfile recordsToAppend chan record.V1 - recordsToFlag chan recordint.Flag + // recordsToFlag chan recordint.Flag } func New(sugar *zap.SugaredLogger, dataDir, deviceID string) *Histio { diff --git a/internal/recio/read.go b/internal/recio/read.go index 6e03f8c..032b7f9 100644 --- a/internal/recio/read.go +++ b/internal/recio/read.go @@ -10,86 +10,88 @@ import ( "github.com/curusarn/resh/internal/futil" "github.com/curusarn/resh/internal/recconv" - "github.com/curusarn/resh/internal/recordint" "github.com/curusarn/resh/record" "go.uber.org/zap" ) -func (r *RecIO) ReadAndFixFile(fpath string, maxErrors int) ([]recordint.Indexed, error) { - recs, numErrs, err := r.ReadFile(fpath) +func (r *RecIO) ReadAndFixFile(fpath string, maxErrors int) ([]record.V1, error) { + recs, err, decodeErrs := r.ReadFile(fpath) if err != nil { return nil, err } + numErrs := len(decodeErrs) if numErrs > maxErrors { + r.sugar.Errorw("Encountered too many decoding errors", + "corruptedRecords", numErrs, + ) return nil, fmt.Errorf("encountered too many decoding errors") } if numErrs == 0 { return recs, nil } - // TODO: check there error messages + // TODO: check the error messages r.sugar.Warnw("Some history records could not be decoded - fixing resh history file by dropping them", "corruptedRecords", numErrs, ) fpathBak := fpath + ".bak" r.sugar.Infow("Backing up current corrupted history file", - "backupFilename", fpathBak, + "historyFileBackup", fpathBak, ) - // TODO: maybe use upstream copy function err = futil.CopyFile(fpath, fpathBak) if err != nil { r.sugar.Errorw("Failed to create a backup history file - aborting fixing history file", - "backupFilename", fpathBak, + "historyFileBackup", fpathBak, zap.Error(err), ) return recs, nil } r.sugar.Info("Writing resh history file without errors ...") - var recsV1 []record.V1 - for _, rec := range recs { - recsV1 = append(recsV1, rec.Rec) - } - err = r.OverwriteFile(fpath, recsV1) + err = r.OverwriteFile(fpath, recs) if err != nil { - r.sugar.Errorw("Failed write fixed history file - aborting fixing history file", - "filename", fpath, + r.sugar.Errorw("Failed write fixed history file - restoring history file from backup", + "historyFile", fpath, zap.Error(err), ) + + err = futil.CopyFile(fpathBak, fpath) + if err != nil { + r.sugar.Errorw("Failed restore history file from backup", + "historyFile", fpath, + "HistoryFileBackup", fpathBak, + zap.Error(err), + ) + } } return recs, nil } -func (r *RecIO) ReadFile(fpath string) ([]recordint.Indexed, int, error) { - var recs []recordint.Indexed +func (r *RecIO) ReadFile(fpath string) ([]record.V1, error, []error) { + var recs []record.V1 file, err := os.Open(fpath) if err != nil { - return nil, 0, fmt.Errorf("failed to open history file: %w", err) + return nil, fmt.Errorf("failed to open history file: %w", err), nil } defer file.Close() reader := bufio.NewReader(file) - numErrs := 0 - var idx int + decodeErrs := []error{} for { var line string line, err = reader.ReadString('\n') if err != nil { break } - idx++ rec, err := r.decodeLine(line) if err != nil { - numErrs++ + r.sugar.Errorw("Error while decoding line", zap.Error(err), + "filePath", fpath, + "line", line, + ) + decodeErrs = append(decodeErrs, err) continue } - recidx := recordint.Indexed{ - Rec: *rec, - // TODO: Is line index actually enough? - // Don't we want to count bytes because we will scan by number of bytes? - // hint: https://benjamincongdon.me/blog/2018/04/10/Counting-Scanned-Bytes-in-Go/ - Idx: idx, - } - recs = append(recs, recidx) + recs = append(recs, *rec) } if err != io.EOF { r.sugar.Error("Error while loading file", zap.Error(err)) @@ -97,7 +99,7 @@ func (r *RecIO) ReadFile(fpath string) ([]recordint.Indexed, int, error) { r.sugar.Infow("Loaded resh history records", "recordCount", len(recs), ) - return recs, numErrs, nil + return recs, nil, decodeErrs } func (r *RecIO) decodeLine(line string) (*record.V1, error) { diff --git a/internal/recio/write.go b/internal/recio/write.go index f3caa37..cf35877 100644 --- a/internal/recio/write.go +++ b/internal/recio/write.go @@ -5,7 +5,6 @@ import ( "fmt" "os" - "github.com/curusarn/resh/internal/recordint" "github.com/curusarn/resh/record" ) @@ -29,16 +28,6 @@ func (r *RecIO) AppendToFile(fpath string, recs []record.V1) error { return writeRecords(file, recs) } -// TODO: better errors -// TODO: rethink this -func (r *RecIO) EditRecordFlagsInFile(fpath string, idx int, rec recordint.Flag) error { - // FIXME: implement - // open file "not as append" - // scan to the correct line - r.sugar.Error("not implemented yet (FIXME)") - return nil -} - func writeRecords(file *os.File, recs []record.V1) error { for _, rec := range recs { jsn, err := encodeV1Record(rec) diff --git a/internal/recordint/flag.go b/internal/recordint/flag.go deleted file mode 100644 index 2eaff6a..0000000 --- a/internal/recordint/flag.go +++ /dev/null @@ -1,9 +0,0 @@ -package recordint - -type Flag struct { - deviceID string - recordID string - - flagDeleted bool - flagFavourite bool -} diff --git a/internal/recordint/indexed.go b/internal/recordint/indexed.go deleted file mode 100644 index 8e527a7..0000000 --- a/internal/recordint/indexed.go +++ /dev/null @@ -1,11 +0,0 @@ -package recordint - -import "github.com/curusarn/resh/record" - -// TODO: rethink this - -// Indexed record allows us to find records in history file in order to edit them -type Indexed struct { - Rec record.V1 - Idx int -} diff --git a/internal/recordint/searchapp.go b/internal/recordint/searchapp.go index 1b5f26d..da85e28 100644 --- a/internal/recordint/searchapp.go +++ b/internal/recordint/searchapp.go @@ -5,7 +5,9 @@ import ( "strconv" "strings" + "github.com/curusarn/resh/record" giturls "github.com/whilp/git-urls" + "go.uber.org/zap" ) // SearchApp record used for sending records to RESH-CLI @@ -27,7 +29,6 @@ type SearchApp struct { Idx int } -// NewCliRecordFromCmdLine func NewSearchAppFromCmdLine(cmdLine string) SearchApp { return SearchApp{ IsRaw: true, @@ -35,36 +36,36 @@ func NewSearchAppFromCmdLine(cmdLine string) SearchApp { } } -// NewCliRecord from EnrichedRecord -func NewSearchApp(r *Indexed) SearchApp { - // TODO: we used to validate records with recutil.Validate() - // TODO: handle this error - time, _ := strconv.ParseFloat(r.Rec.Time, 64) +// The error handling here could be better +func NewSearchApp(sugar *zap.SugaredLogger, r *record.V1) SearchApp { + time, err := strconv.ParseFloat(r.Time, 64) + if err != nil { + sugar.Errorw("Error while parsing time as float", zap.Error(err), + "time", time) + } return SearchApp{ IsRaw: false, - SessionID: r.Rec.SessionID, - CmdLine: r.Rec.CmdLine, - Host: r.Rec.Device, - Pwd: r.Rec.Pwd, - Home: r.Rec.Home, + SessionID: r.SessionID, + CmdLine: r.CmdLine, + Host: r.Device, + Pwd: r.Pwd, + Home: r.Home, // TODO: is this the right place to normalize the git remote - GitOriginRemote: normalizeGitRemote(r.Rec.GitOriginRemote), - ExitCode: r.Rec.ExitCode, + GitOriginRemote: normalizeGitRemote(sugar, r.GitOriginRemote), + ExitCode: r.ExitCode, Time: time, - - Idx: r.Idx, } } // TODO: maybe move this to a more appropriate place // normalizeGitRemote helper -func normalizeGitRemote(gitRemote string) string { - if strings.HasSuffix(gitRemote, ".git") { - gitRemote = gitRemote[:len(gitRemote)-4] - } +func normalizeGitRemote(sugar *zap.SugaredLogger, gitRemote string) string { + gitRemote = strings.TrimSuffix(gitRemote, ".git") parsedURL, err := giturls.Parse(gitRemote) if err != nil { - // TODO: log this error + sugar.Errorw("Failed to parse git remote", zap.Error(err), + "gitRemote", gitRemote, + ) return gitRemote } if parsedURL.User == nil || parsedURL.User.Username() == "" { diff --git a/internal/searchapp/test.go b/internal/searchapp/test.go index 9a2d284..52a0982 100644 --- a/internal/searchapp/test.go +++ b/internal/searchapp/test.go @@ -12,12 +12,12 @@ func LoadHistoryFromFile(sugar *zap.SugaredLogger, historyPath string, numLines rio := recio.New(sugar) recs, _, err := rio.ReadFile(historyPath) if err != nil { - sugar.Panicf("failed to read hisotry file: %w", err) + sugar.Panicf("failed to read history file: %w", err) } if numLines != 0 && numLines < len(recs) { recs = recs[:numLines] } - cliRecords := histcli.New() + cliRecords := histcli.New(sugar) for i := len(recs) - 1; i >= 0; i-- { rec := recs[i] cliRecords.AddRecord(&rec) diff --git a/record/v1.go b/record/v1.go index 6585051..47397af 100644 --- a/record/v1.go +++ b/record/v1.go @@ -54,6 +54,9 @@ type V1 struct { // these look like internal stuff + // TODO: rethink + // I don't really like this :/ + // Maybe just one field 'NotMerged' with 'partOne' and 'partTwo' as values and empty string for merged // records come in two parts (collect and postcollect) PartOne bool `json:"partOne,omitempty"` // false => part two PartsNotMerged bool `json:"partsNotMerged,omitempty"`