luci-go: Tools for working with BSD style ar archives.

common/archive/ar/errors.go

Index: common/archive/ar/errors.go
diff --git a/common/archive/ar/errors.go b/common/archive/ar/errors.go
new file mode 100644
index 0000000000000000000000000000000000000000..7c7533ba66f9b504438e09ba70240de6a9eb58a5
--- /dev/null
+++ b/common/archive/ar/errors.go
@@ -0,0 +1,46 @@
+// Copyright 2016 The LUCI Authors. All rights reserved.
+// Use of this source code is governed under the Apache License, Version 2.0
+// that can be found in the LICENSE file.
+
+// Write an ar archive file with BSD style filenames.
+
+package ar
+
+import (
+	"fmt"
+)
+
+// Error indicates an error and if it is fatal or not.
+type Error interface {

[mcgreevy, 2016/11/07 00:37:22]

There are a bunch of different error types in this package, some of which serve
to store extra information about the cause of the error, and some which serve to
provide different implementations of this Error interface (e.g.
WriteTooLongError, which always returns false from its Fatal method, and
WriteTooLongFatalError which always returns true).

I'm not sure that all of these different types are necessary, and even if they
are, whether they need to be exported.  For example, will users of this package
ever do type assertions to check whether an error is a WriteTooLongFatalError vs
a WriteTooLongError?  If not, that's a signal that they don't need to be
exported.

The same effect could be achieved with fewer types, e.g.:

// note: these need not be exported.
type fatalError string
func (err fatalError) Fatal() {return true}

type nonFatalError string
func (err nonFatalError) Fatal() {return false}


func WriteTooLongError(got, want int64) Error {
  return nonFatalError(fmt.Sprintf("invalid data length (got %d, want %d)", got,
want))
}

func WriteTooLongFatalError(got, want int64) Error {
  return fatalError(fmt.Sprintf("archive corrupted: invalid data written (got
%d, want %d)", got, want))
}


I suspect that even the distinction between fatal vs non-fatal errors can be
conveyed via some other mechanism, which would mean that this Error interface
could go away, and a standard error type used instead, which would be nice. This
will probably become clearer as the number of these error types is reduced.

+	error
+	Fatal() bool // Is the error fatal and the archive is now corrupted?
+}
+
+// UsageError indicates an error with using the archive/ar API.
+type UsageError struct {

[mcgreevy, 2016/11/07 00:37:22]

This doesn't need to be a struct.

  type UsageError string

would suffice.

You can than create one like so:

error := UsageError("some message")

You can still define methods on this type, like:

func (e UsageError) Error() string {...}

+	msg string
+}
+
+func (e *UsageError) Error() string {
+	return fmt.Sprintf("archive/ar: usage error, %s", e.msg)
+}
+
+// Fatal is always false for Usage.
+func (e *UsageError) Fatal() bool {
+	return false
+}
+
+// IOError indicates an error occurred during IO operations.
+type IOError struct {
+	section string
+	err     error
+}
+
+func (e *IOError) Error() string {
+	return fmt.Sprintf("archive/ar: io error (%s) during %s -- *archive corrupted*", e.err.Error(), e.section)
+}
+
+// Fatal is always true for IOError.
+func (e *IOError) Fatal() bool {
+	return true
+}