From 0cee5e3a9450fc2bcd93b5498622d757356314dc Mon Sep 17 00:00:00 2001 From: Ayman Bagabas Date: Fri, 27 Oct 2023 09:59:02 -0400 Subject: [PATCH] Revert "feat: add logger interface (#17)" This reverts commit bb5f31e52272f4e171baeecef2a7aee40354cb9e. --- .editorconfig | 5 --- cmd.go => cmd/cmd.go | 26 +++++++-------- cmd_other.go => cmd/cmd_other.go | 2 +- cmd_unix.go => cmd/cmd_unix.go | 2 +- internal/local/backend.go | 7 +++- internal/local/backend_unix.go | 1 + main.go | 26 +++------------ transfer/args.go | 1 + transfer/log.go | 24 +++++++++----- transfer/oid.go | 2 +- transfer/pktline.go | 37 +++++++++------------ transfer/processor.go | 56 +++++++++++++++----------------- 12 files changed, 85 insertions(+), 104 deletions(-) delete mode 100644 .editorconfig rename cmd.go => cmd/cmd.go (77%) rename cmd_other.go => cmd/cmd_other.go (95%) rename cmd_unix.go => cmd/cmd_unix.go (98%) diff --git a/.editorconfig b/.editorconfig deleted file mode 100644 index 0e88400..0000000 --- a/.editorconfig +++ /dev/null @@ -1,5 +0,0 @@ -root = true - -[*.go] -indent_style = tab -indent_size = 4 diff --git a/cmd.go b/cmd/cmd.go similarity index 77% rename from cmd.go rename to cmd/cmd.go index e8e02ac..b227a32 100644 --- a/cmd.go +++ b/cmd/cmd.go @@ -1,4 +1,4 @@ -package main +package cmd import ( "fmt" @@ -31,7 +31,7 @@ func ensureDirs(path string) error { return nil } -func run(r io.Reader, w io.Writer, args ...string) error { +func run(r io.Reader, w io.Writer, args []string) error { if len(args) != 2 { return fmt.Errorf("expected 2 arguments, got %d", len(args)) } @@ -50,20 +50,20 @@ func run(r io.Reader, w io.Writer, args ...string) error { return err } umask := setPermissions(gitdir) - handler := transfer.NewPktline(r, w, logger) + handler := transfer.NewPktline(r, w) for _, cap := range capabilities { if err := handler.WritePacketText(cap); err != nil { - logger.Log("error sending capability", "cap", cap, "err", err) + transfer.Logf("error sending capability: %s: %v", cap, err) } } if err := handler.WriteFlush(); err != nil { - logger.Log("error flushing capabilities", "err", err) + transfer.Logf("error flushing capabilities: %v", err) } now := time.Now() - logger.Log("umask", "umask", umask) + transfer.Logf("umask %o", umask) backend := local.New(lfsPath, umask, &now) - p := transfer.NewProcessor(handler, backend, logger) - defer logger.Log("done processing commands") + p := transfer.NewProcessor(handler, backend) + defer transfer.Log("done processing commands") switch op { case "upload": return p.ProcessCommands(transfer.UploadOperation) @@ -94,17 +94,17 @@ func Command(stdin io.Reader, stdout io.Writer, stderr io.Writer, args ...string errc := make(chan error, 1) setup(done) - logger.Log("git-lfs-transfer", "version", "v1") - defer logger.Log("git-lfs-transfer completed") + transfer.Logf("git-lfs-transfer %s", "v1") + defer transfer.Log("git-lfs-transfer completed") go func() { - errc <- run(stdin, stdout, args...) + errc <- run(stdin, stdout, args) }() select { case s := <-done: - logger.Log("signal received", "signal", s) + transfer.Logf("signal %q received", s) case err := <-errc: - logger.Log("done running") + transfer.Log("done running") fmt.Fprintln(stderr, Usage()) fmt.Fprintln(stderr, err) if err != nil { diff --git a/cmd_other.go b/cmd/cmd_other.go similarity index 95% rename from cmd_other.go rename to cmd/cmd_other.go index e80536c..ef1586f 100644 --- a/cmd_other.go +++ b/cmd/cmd_other.go @@ -1,7 +1,7 @@ //go:build !(darwin || dragonfly || freebsd || linux || netbsd || openbsd || solaris) // +build !darwin,!dragonfly,!freebsd,!linux,!netbsd,!openbsd,!solaris -package main +package cmd import ( "os" diff --git a/cmd_unix.go b/cmd/cmd_unix.go similarity index 98% rename from cmd_unix.go rename to cmd/cmd_unix.go index f0444c3..ffea638 100644 --- a/cmd_unix.go +++ b/cmd/cmd_unix.go @@ -1,7 +1,7 @@ //go:build darwin || dragonfly || freebsd || linux || netbsd || openbsd || solaris // +build darwin dragonfly freebsd linux netbsd openbsd solaris -package main +package cmd import ( "os" diff --git a/internal/local/backend.go b/internal/local/backend.go index 6407d3d..0c4599c 100644 --- a/internal/local/backend.go +++ b/internal/local/backend.go @@ -72,6 +72,7 @@ func (l *LocalBackend) FinishUpload(state interface{}, _ transfer.Args) error { case *UploadState: destPath := oidExpectedPath(l.lfsPath, state.Oid) parent := filepath.Dir(destPath) + transfer.Logf("finishing upload of %s at %s", destPath, parent) if err := os.MkdirAll(parent, 0777); err != nil { return err } @@ -105,6 +106,7 @@ func (l *LocalBackend) StartUpload(oid string, r io.Reader, _ transfer.Args) (in if r == nil { return nil, fmt.Errorf("%w: received null data", transfer.ErrMissingData) } + transfer.Logf("start uploading %s", oid) tempDir := filepath.Join(l.lfsPath, "incomplete") randBytes := make([]byte, 12) if _, err := rand.Read(randBytes); err != nil { @@ -140,6 +142,7 @@ func (l *LocalBackend) Verify(oid string, args transfer.Args) (transfer.Status, return nil, err } if stat.Size() != int64(expectedSize) { + transfer.Logf("size mismatch, expected %d, got %d", expectedSize, stat.Size()) return transfer.NewFailureStatus(transfer.StatusConflict, "size mismatch"), nil } return transfer.SuccessStatus(), nil @@ -166,7 +169,7 @@ func (l *localLockBackend) Timestamp() *time.Time { } // Create implements main.LockBackend. -func (l *localLockBackend) Create(path, _ string) (transfer.Lock, error) { +func (l *localLockBackend) Create(path, refname string) (transfer.Lock, error) { id := localBackendLock{}.HashFor(path) var b bytes.Buffer b.WriteString(fmt.Sprintf("%s:%d:", LocalBackendLockVersion, l.Timestamp().Unix())) @@ -239,10 +242,12 @@ func (l *localLockBackend) Range(_ string, _ int, f func(l transfer.Lock) error) if err != nil { return "", err } + transfer.Logf("found %d locks", len(data)) sort.Slice(data, func(i, j int) bool { return data[i].Name() < data[j].Name() }) for _, lf := range data { + transfer.Logf("found lock %s", lf.Name()) var lock transfer.Lock lock, err = l.FromID(lf.Name()) if err != nil { diff --git a/internal/local/backend_unix.go b/internal/local/backend_unix.go index 17fdf35..729c946 100644 --- a/internal/local/backend_unix.go +++ b/internal/local/backend_unix.go @@ -19,6 +19,7 @@ func (l *LocalBackend) FixPermissions(path string) (transfer.Status, error) { if err != nil { return nil, err } + transfer.Logf("settings permissions of %s to %o", path, 0777&^l.umask) if err := os.Chmod(path, 0777&^l.umask); err != nil { return nil, err } diff --git a/main.go b/main.go index 989429d..6c3ab84 100644 --- a/main.go +++ b/main.go @@ -5,37 +5,19 @@ import ( "fmt" "os" + "github.com/charmbracelet/git-lfs-transfer/cmd" "github.com/charmbracelet/git-lfs-transfer/transfer" - "github.com/rubyist/tracerx" ) -type tracerxLogger struct{} - -// Log logs the given arguments if Debug is true. -func (*tracerxLogger) Log(msg string, kv ...interface{}) { - format := msg - for i := 0; i < len(kv); i += 2 { - format += " %s=%v" - } - tracerx.Printf(format, kv...) -} - -var logger = new(tracerxLogger) - -func init() { - tracerx.DefaultKey = "GIT" - tracerx.Prefix = "trace git-lfs-transfer: " -} - func main() { args := os.Args if len(args) < 2 { - fmt.Fprintln(os.Stderr, Usage()) + fmt.Fprintln(os.Stderr, cmd.Usage()) fmt.Fprintf(os.Stderr, "expected 2 arguments, got %d\n", len(args)-1) os.Exit(1) } - if err := Command(os.Stdin, os.Stdout, os.Stderr, args[1:]...); err != nil { - fmt.Fprintf(os.Stderr, Usage()) + if err := cmd.Command(os.Stdin, os.Stdout, os.Stderr, args[1:]...); err != nil { + fmt.Fprintf(os.Stderr, cmd.Usage()) fmt.Fprintln(os.Stderr) fmt.Fprintln(os.Stderr, err) switch { diff --git a/transfer/args.go b/transfer/args.go index 3f7ae4c..992c476 100644 --- a/transfer/args.go +++ b/transfer/args.go @@ -29,6 +29,7 @@ func ParseArgs(parts []string) (Args, error) { key, value := parts[0], parts[1] args[key] = value } + Logf("args: %d %v", len(args), args) return args, nil } diff --git a/transfer/log.go b/transfer/log.go index 937402d..a3ed11f 100644 --- a/transfer/log.go +++ b/transfer/log.go @@ -1,14 +1,20 @@ package transfer -// Logger is a logging interface. -type Logger interface { - // Log logs the given message and structured arguments. - Log(msg string, kv ...interface{}) -} +import ( + "github.com/rubyist/tracerx" +) -type noopLogger struct{} +var ( + // Debug is the debug flag. + Debug = false +) -var _ Logger = (*noopLogger)(nil) +// Log logs the given arguments if Debug is true. +func Log(v ...interface{}) { + tracerx.Printf("%v", v...) +} -// Log implements Logger. -func (*noopLogger) Log(string, ...interface{}) {} +// Logf logs the given arguments if Debug is true. +func Logf(format string, v ...interface{}) { + tracerx.Printf(format, v...) +} diff --git a/transfer/oid.go b/transfer/oid.go index 42ac533..ccd689a 100644 --- a/transfer/oid.go +++ b/transfer/oid.go @@ -34,7 +34,7 @@ func (p Pointer) IsValid() bool { return true } -// RelativePath returns the relative storage path of the pointer. +// RelativePath returns the relative storage path of the pointer func (p Pointer) RelativePath() string { if len(p.Oid) < 5 { return p.Oid diff --git a/transfer/pktline.go b/transfer/pktline.go index fa6cbb9..85583e2 100644 --- a/transfer/pktline.go +++ b/transfer/pktline.go @@ -30,75 +30,70 @@ const ( // PktLine is a Git packet line handler. type Pktline struct { *pktline.Pktline - r io.Reader - w io.Writer - logger Logger + r io.Reader + w io.Writer } // NewPktline creates a new Git packet line handler. -func NewPktline(r io.Reader, w io.Writer, logger Logger) *Pktline { - if logger == nil { - logger = new(noopLogger) - } +func NewPktline(r io.Reader, w io.Writer) *Pktline { return &Pktline{ Pktline: pktline.NewPktline(r, w), r: r, w: w, - logger: logger, } } // SendError sends an error msg. func (p *Pktline) SendError(status uint32, message string) error { - p.logger.Log("sending error status", "code", status, "msg", message) + Logf("sending error: %d %s", status, message) if err := p.WritePacketText(fmt.Sprintf("status %03d", status)); err != nil { - p.logger.Log("failed to write packet", "err", err) + Logf("error writing status: %s", err) } if err := p.WriteDelim(); err != nil { - p.logger.Log("failed to write delimiter", "err", err) + Logf("error writing delim: %s", err) } if err := p.WritePacketText(message); err != nil { - p.logger.Log("failed to write message", "err", err) + Logf("error writing message: %s", err) } return p.WriteFlush() } // SendStatus sends a status message. func (p *Pktline) SendStatus(status Status) error { - p.logger.Log("sending status", "code", status) + Logf("sending status: %s", status) if err := p.WritePacketText(fmt.Sprintf("status %03d", status.Code())); err != nil { - p.logger.Log("failed to write status", "err", err) + Logf("error writing status: %s", err) } if args := status.Args(); len(args) > 0 { for _, arg := range args { if err := p.WritePacketText(arg); err != nil { - p.logger.Log("failed to write argument", "arg", arg, "err", err) + Logf("error writing arg: %s", err) } } } if msgs := status.Messages(); len(msgs) > 0 { if err := p.WriteDelim(); err != nil { - p.logger.Log("failed to write delimiter", "err", err) + Logf("error writing delim: %s", err) } for _, msg := range msgs { if err := p.WritePacketText(msg); err != nil { - p.logger.Log("failed to write message", "err", err) + Logf("error writing msg: %s", err) } } } else if r := status.Reader(); r != nil { - p.logger.Log("sending reader") + Logf("sending reader") // Close reader if it implements io.Closer. if c, ok := r.(io.Closer); ok { defer c.Close() // nolint: errcheck } if err := p.WriteDelim(); err != nil { - p.logger.Log("failed to write delimiter", "err", err) + Logf("error writing delim: %v", err) } w := p.Writer() if _, err := io.Copy(w, r); err != nil { - p.logger.Log("failed to copy reader", "err", err) + Logf("error copying reader: %v", err) } - defer p.logger.Log("done copying") + defer Logf("done copying") return w.Flush() } return p.WriteFlush() diff --git a/transfer/processor.go b/transfer/processor.go index 47de002..493da00 100644 --- a/transfer/processor.go +++ b/transfer/processor.go @@ -15,18 +15,13 @@ import ( type Processor struct { handler *Pktline backend Backend - logger Logger } // NewProcessor creates a new transfer processor. -func NewProcessor(line *Pktline, backend Backend, logger Logger) *Processor { - if logger == nil { - logger = new(noopLogger) - } +func NewProcessor(line *Pktline, backend Backend) *Processor { return &Processor{ handler: line, backend: backend, - logger: logger, } } @@ -34,7 +29,7 @@ func NewProcessor(line *Pktline, backend Backend, logger Logger) *Processor { func (p *Processor) Version() (Status, error) { _, err := p.handler.ReadPacketListToFlush() if err != nil { - p.logger.Log("invalid version", "err", err) + Logf("version error: %s", err) } return NewSuccessStatus([]string{}), nil } @@ -56,7 +51,8 @@ func (p *Processor) ReadBatch(op string, args Args) ([]BatchItem, error) { default: return nil, fmt.Errorf("%w: %s", ErrNotAllowed, fmt.Sprintf("unsupported hash algorithm: %s", hashAlgo)) } - p.logger.Log("read batch", "operation", op, "args-len", len(args), "args", args, "data-len", len(data), "data", data) + Logf("data: %d %v", len(data), data) + Logf("batch: %s args: %d %v data: %d %v", op, len(args), args, len(data), data) items := make([]BatchItem, 0) for _, line := range data { if line == "" { @@ -86,12 +82,12 @@ func (p *Processor) ReadBatch(op string, args Args) ([]BatchItem, error) { } items = append(items, item) } - p.logger.Log("batch items", "items", items) + Logf("items %v", items) its, err := p.backend.Batch(op, items, args) if err != nil { return nil, err } - p.logger.Log("batch items", "items", items) + Logf("batch items: %v", its) return its, nil } @@ -243,12 +239,12 @@ func (p *Processor) Lock() (Status, error) { for { lock, err := lockBackend.Create(path, refname) if errors.Is(err, ErrConflict) { - p.logger.Log("lock conflict") + Logf("lock conflict") lock, err = lockBackend.FromPath(path) if err != nil { - p.logger.Log("lock conflict, but no lock found") + Logf("lock conflict, but no lock found") if retried { - p.logger.Log("lock conflict, but no lock found, and retried") + Logf("lock conflict, but no lock found, and retried") return nil, err } retried = true @@ -257,10 +253,10 @@ func (p *Processor) Lock() (Status, error) { return NewFailureStatusWithArgs(StatusConflict, "conflict", lock.AsArguments()...), nil } if err != nil { - p.logger.Log("failed to create lock", "err", err) + Logf("lock error: %v", err) return nil, err } - p.logger.Log("lock success", "lock", lock) + Logf("lock success: %v", lock) return NewSuccessStatusWithCode(StatusCreated, lock.AsArguments()...), nil } // unreachable @@ -319,7 +315,7 @@ func (p *Processor) ListLocks(useOwnerID bool) (Status, error) { // skip nil locks return nil } - p.logger.Log("adding lock", "path", lock.Path(), "id", lock.ID()) + Logf("adding lock %s %s", lock.Path(), lock.ID()) locks = append(locks, lock) return nil }) @@ -378,7 +374,7 @@ func (p *Processor) Unlock(id string) (Status, error) { // ProcessCommands processes commands from the transfer protocol. func (p *Processor) ProcessCommands(op string) error { - p.logger.Log("processing commands") + Log("processing commands") for { pkt, err := p.handler.ReadPacketText() if errors.Is(err, io.EOF) { @@ -387,21 +383,21 @@ func (p *Processor) ProcessCommands(op string) error { if err != nil { return err } - p.logger.Log("received packet", "packet", pkt) + Logf("received packet: %s", pkt) if pkt == "" { if err := p.handler.SendError(StatusBadRequest, "unknown command"); err != nil { - p.logger.Log("failed to send pktline", "err", err) + Logf("error pktline sending error: %v", err) } continue } msgs := strings.Split(pkt, " ") if len(msgs) < 1 { if err := p.handler.SendError(StatusBadRequest, "no command provided"); err != nil { - p.logger.Log("failed to send pktline", "err", err) + Logf("error pktline sending error: %v", err) } continue } - p.logger.Log("received command", "command", msgs[0], "messages", msgs[1:]) + Logf("received command: %s %v", msgs[0], msgs[1:]) var status Status switch msgs[0] { case versionCommand: @@ -413,10 +409,10 @@ func (p *Processor) ProcessCommands(op string) error { case batchCommand: switch op { case UploadOperation: - p.logger.Log("upload batch command received") + Logf("upload batch command received") status, err = p.UploadBatch() case DownloadOperation: - p.logger.Log("download batch command received") + Logf("download batch command received") status, err = p.DownloadBatch() default: err = p.handler.SendError(StatusBadRequest, "unknown operation") @@ -448,7 +444,7 @@ func (p *Processor) ProcessCommands(op string) error { case DownloadOperation: status, err = p.ListLocks(false) } - p.logger.Log("list lock command", "status", status, "err", err) + Logf("list lock status: %v %v", status, err) case unlockCommand: if len(msgs) > 1 { status, err = p.Unlock(msgs[1]) @@ -457,7 +453,7 @@ func (p *Processor) ProcessCommands(op string) error { } case quitCommand: if err := p.handler.SendStatus(SuccessStatus()); err != nil { - p.logger.Log("failed to send pktline", "err", err) + Logf("error pktline sending status: %v", err) } return nil default: @@ -470,20 +466,20 @@ func (p *Processor) ProcessCommands(op string) error { errors.Is(err, ErrInvalidPacket), errors.Is(err, ErrCorruptData): if err := p.handler.SendError(StatusBadRequest, fmt.Errorf("error: %w", err).Error()); err != nil { - p.logger.Log("failed to send pktline", "err", err) + Logf("error pktline sending error: %v", err) } default: - p.logger.Log("failed to process command", "err", err) + Logf("error processing command: %v", err) if err := p.handler.SendError(StatusInternalServerError, "internal error"); err != nil { - p.logger.Log("failed to send pktline", "err", err) + Logf("error pktline sending error: %v", err) } } } if status != nil { if err := p.handler.SendStatus(status); err != nil { - p.logger.Log("failed to send pktline", "err", err) + Logf("error pktline sending status: %v", err) } } - p.logger.Log("processed command") + Log("processed command") } }