transport/internal: error handling fixes and clean up (#160)

* protocol/packp: remove redundant isFlush check on AdvRefs.
* protocol/packp: improve AdvRefs documentation.
* transport: improve error handling for non-existing repos.
* protocol/packp: AdvRefs Decode now returns different errors for
  empty, but syntactically correct, AdvRefs message (ErrEmptyAdvRefs)
  and empty input (ErrEmptyInput).
* transport/internal/common: read stderr only when needed (ErrEmptyInput).
  Close the client gracefully.
* transport/internal/common: missing stderr on non existing repository
  does not block.
* transport/internal/common: buffer error messages.
* transport/file: fix changing binary name, add tests.
* transport/file: support changing git-upload-pack and git-receive-pack
  binary names.
* transport/file: add tests for misbehaving servers.
* transport/internal/common: remove Stderr field.
* transport/internal/common: do not close twice.

3 files changed, 47 insertions, 37 deletions

diff --git a/plumbing/protocol/packp/advrefs.go b/plumbing/protocol/packp/advrefs.go
index a0587ab..4e031fb 100644
--- a/plumbing/protocol/packp/advrefs.go
+++ b/plumbing/protocol/packp/advrefs.go
@@ -13,25 +13,32 @@ import (
 // AdvRefs values represent the information transmitted on an
 // advertised-refs message.  Values from this type are not zero-value
 // safe, use the New function instead.
-//
-// When using this messages over (smart) HTTP, you have to add a pktline
-// before the whole thing with the following payload:
-//
-// '# service=$servicename" LF
-//
-// Moreover, some (all) git HTTP smart servers will send a flush-pkt
-// just after the first pkt-line.
-//
-// To accomodate both situations, the Prefix field allow you to store
-// any data you want to send before the actual pktlines.  It will also
-// be filled up with whatever is found on the line.
 type AdvRefs struct {
-	Prefix       [][]byte // payloads of the prefix
-	Head         *plumbing.Hash
+	// Prefix stores prefix payloads.
+	//
+	// When using this message over (smart) HTTP, you have to add a pktline
+	// before the whole thing with the following payload:
+	//
+	// '# service=$servicename" LF
+	//
+	// Moreover, some (all) git HTTP smart servers will send a flush-pkt
+	// just after the first pkt-line.
+	//
+	// To accomodate both situations, the Prefix field allow you to store
+	// any data you want to send before the actual pktlines.  It will also
+	// be filled up with whatever is found on the line.
+	Prefix [][]byte
+	// Head stores the resolved HEAD reference if present.
+	// This can be present with git-upload-pack, not with git-receive-pack.
+	Head *plumbing.Hash
+	// Capabilities are the capabilities.
 	Capabilities *capability.List
-	References   map[string]plumbing.Hash
-	Peeled       map[string]plumbing.Hash
-	Shallows     []plumbing.Hash
+	// References are the hash references.
+	References map[string]plumbing.Hash
+	// Peeled are the peeled hash references.
+	Peeled map[string]plumbing.Hash
+	// Shallows are the shallow object ids.
+	Shallows []plumbing.Hash
 }
 
 // NewAdvRefs returns a pointer to a new AdvRefs value, ready to be used.
diff --git a/plumbing/protocol/packp/advrefs_decode.go b/plumbing/protocol/packp/advrefs_decode.go
index 5926645..964e3eb 100644
--- a/plumbing/protocol/packp/advrefs_decode.go
+++ b/plumbing/protocol/packp/advrefs_decode.go
@@ -27,8 +27,13 @@ type advRefsDecoder struct {
 	data  *AdvRefs         // parsed data is stored here
 }
 
-// ErrEmptyAdvRefs is returned by Decode when there was no advertised-message at all
-var ErrEmptyAdvRefs = errors.New("empty advertised-ref message")
+var (
+	// ErrEmptyAdvRefs is returned by Decode if it gets an empty advertised
+	// references message.
+	ErrEmptyAdvRefs = errors.New("empty advertised-ref message")
+	// ErrEmptyInput is returned by Decode if the input is empty.
+	ErrEmptyInput = errors.New("empty input")
+)
 
 func newAdvRefsDecoder(r io.Reader) *advRefsDecoder {
 	return &advRefsDecoder{
@@ -67,7 +72,7 @@ func (d *advRefsDecoder) nextLine() bool {
 		}
 
 		if d.nLine == 1 {
-			d.err = ErrEmptyAdvRefs
+			d.err = ErrEmptyInput
 			return false
 		}
 
@@ -87,33 +92,31 @@ func decodePrefix(d *advRefsDecoder) decoderStateFn {
 		return nil
 	}
 
-	// If the repository is empty, we receive a flush here (SSH).
-	if isFlush(d.line) {
-		d.err = ErrEmptyAdvRefs
+	if !isPrefix(d.line) {
+		return decodeFirstHash
+	}
+
+	tmp := make([]byte, len(d.line))
+	copy(tmp, d.line)
+	d.data.Prefix = append(d.data.Prefix, tmp)
+	if ok := d.nextLine(); !ok {
 		return nil
 	}
 
-	if isPrefix(d.line) {
-		tmp := make([]byte, len(d.line))
-		copy(tmp, d.line)
-		d.data.Prefix = append(d.data.Prefix, tmp)
-		if ok := d.nextLine(); !ok {
-			return nil
-		}
+	if !isFlush(d.line) {
+		return decodeFirstHash
 	}
 
-	if isFlush(d.line) {
-		d.data.Prefix = append(d.data.Prefix, pktline.Flush)
-		if ok := d.nextLine(); !ok {
-			return nil
-		}
+	d.data.Prefix = append(d.data.Prefix, pktline.Flush)
+	if ok := d.nextLine(); !ok {
+		return nil
 	}
 
 	return decodeFirstHash
 }
 
 func isPrefix(payload []byte) bool {
-	return payload[0] == '#'
+	return len(payload) > 0 && payload[0] == '#'
 }
 
 func isFlush(payload []byte) bool {
diff --git a/plumbing/protocol/packp/advrefs_decode_test.go b/plumbing/protocol/packp/advrefs_decode_test.go
index f807f15..2cc2568 100644
--- a/plumbing/protocol/packp/advrefs_decode_test.go
+++ b/plumbing/protocol/packp/advrefs_decode_test.go
@@ -19,7 +19,7 @@ var _ = Suite(&AdvRefsDecodeSuite{})
 func (s *AdvRefsDecodeSuite) TestEmpty(c *C) {
 	var buf bytes.Buffer
 	ar := NewAdvRefs()
-	c.Assert(ar.Decode(&buf), Equals, ErrEmptyAdvRefs)
+	c.Assert(ar.Decode(&buf), Equals, ErrEmptyInput)
 }
 
 func (s *AdvRefsDecodeSuite) TestEmptyFlush(c *C) {