Drover's back, baby!

Drover's back, baby!

Implements drover for git!

Assumes some things about the git repo though:
  * uses cr-rev to disambiguate commit-to-land
    * includes automatic discovery of git repo
  * uses gitiles to fetch the minimal subset of objects necessary for the
    specified action.
  * knows about codereview.settings and the pending ref stuff.

Features:
  * Colors
  * Progress bars
  * It's really really fast
  * Performs the action in a real on-disk environment with real (minimal) git
    commits. No history is available, but if there's a conflict, you can
    resolve it in the normal way.
    * Pops you into a shell in the directory if it detects a conflict.
  * Currently stops short of the push, but I've verified that the git client
    can correctly synthesize the packfile and perform the push.
  * Intermediate progress is cached locally... if something goes sideways in
    the process you can re-do the command and it will only fetch objects it
    doesn't have yet.
  * Probably works on windows (untested)

R=agable@chromium.org,maruel@chromium.org
BUG=404755

[iannucci, 2014-10-17 23:29:07]

Needs tests, and needs a couple packages mirrored (for progress bar and colors),
but totally works (tm)!

[iannucci, 2014-10-17 23:36:58]

And please suggest API refactoring... I posted this all in one chunk, but I
think I'd be happy to split this into git/gitiles/drover CLs as I add tests as
appropriate. If you could do a quick overview, that would be great.

There's some gross code in drover especially... I've got writers block though
and I can't really look at it. If you spot some obvious things to clean up,
please post them too.

[M-A Ruel, 2014-10-18 00:47:07]

Also a package level comment on each non-main package would be nice.

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/blob.go
File go/src/infra/libs/git/blob.go (right):

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/blob.g...
go/src/infra/libs/git/blob.go:1: package git
Chromium Copyright.

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/commit.go
File go/src/infra/libs/git/commit.go (right):

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/commit...
go/src/infra/libs/git/commit.go:3: import "bytes"
import (
  "bytes"
  "fmt"
..
)

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/commit...
go/src/infra/libs/git/commit.go:27: return fmt.Sprintf("%s <%s> %d %s", u.Name,
u.Email, u.Time.Unix(),
No word wrap, use "gomt -w -s *.go" and "goimports -w ."

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/commit...
go/src/infra/libs/git/commit.go:28: u.Time.Format("-0700"))
PDT, really?

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/commit...
go/src/infra/libs/git/commit.go:34: type Commit struct {
Is it implementing an interface? Otherwise why all these getters?

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/commit...
go/src/infra/libs/git/commit.go:37: // TODO(riannucci): Make these real Object's
iannucci

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/commit...
go/src/infra/libs/git/commit.go:47: messageRawLines *[]string
Why *[]string and not []string?

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/commit...
go/src/infra/libs/git/commit.go:50: footers         *map[string][]string
why pointer?

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/commit...
go/src/infra/libs/git/commit.go:54: func (c *Commit) Type() string   { return
"commit" }
You should:

type ObjectType string
const (
  CommitType = "commit"
  BlobType = "blob"
...
)

that would catch typo, enforce strictness, etc.

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/commit...
go/src/infra/libs/git/commit.go:61: func (c *Commit) ExtraHeaders() (r []string)
{ return append(r, c.extraHeaders...) }
You meant:
return c.extraHeaders[:]

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/commit...
go/src/infra/libs/git/commit.go:62: func (c *Commit) MessageRaw() string        
{ return c.messageRaw }
Note that golang string is unicode, but it's possible that git commit messages
to not be in utf-8.

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/commit...
go/src/infra/libs/git/commit.go:240: var UserRegex = regexp.MustCompile(
it's not private if it is Upper case

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/commit...
go/src/infra/libs/git/commit.go:249: if r := recover(); r != nil {
what would panic that you'd want to catch this way?

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/diff.go
File go/src/infra/libs/git/diff.go (right):

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/diff.g...
go/src/infra/libs/git/diff.go:6: func (r *Repo) GetTextDiff(left, right string)
[]string {
Why []string instead of string?

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/diff.g...
go/src/infra/libs/git/diff.go:6: func (r *Repo) GetTextDiff(left, right string)
[]string {
Why not implement in repo.go or where ever you define Repo?

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/diff.g...
go/src/infra/libs/git/diff.go:9: panic(fmt.Errorf("cannot diff(%s, %s): %s",
left, right, rslt))
No, change the signature to return ([]string, error)
then
return nil, fmt.Errorf(...

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/diff.g...
go/src/infra/libs/git/diff.go:11: return strings.Split(rslt, "\n")
return strings.Split(rslt, "\n"), nil

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/diff_t...
File go/src/infra/libs/git/diff_tree.go (right):

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/diff_t...
go/src/infra/libs/git/diff_tree.go:34: func (t TreeDiffEntryHalf) String()
string {
Use a pointer, otherwise is copy the struct

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/diff_t...
go/src/infra/libs/git/diff_tree.go:67: panic("git.DiffTree: desynchronized
parsing error")
[BTW I know it's usual to use panic() all around while implementing, just noting
for completeness]

return an error instead

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/get_ob...
File go/src/infra/libs/git/get_object.go (right):

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/get_ob...
go/src/infra/libs/git/get_object.go:79: FullTree  fullTree = true
move constants and simple types at the top

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/get_ob...
go/src/infra/libs/git/get_object.go:103: p := p
Oops?

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/get_ob...
go/src/infra/libs/git/get_object.go:151: func (r *Repo) ensureCatFileServer(ch
**chan<- catFileRequest, checkOnly bool) {
Double pointer is unnecessary and you shouldn't normally need unsafe at all.
Please refactor.

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/get_ob...
go/src/infra/libs/git/get_object.go:183: panic(err)
Send error back in channel

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/intern.go
File go/src/infra/libs/git/intern.go (right):

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/intern...
go/src/infra/libs/git/intern.go:6: // InternResult contians either the ID of the
intern'd object, or an error.
contains

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/object.go
File go/src/infra/libs/git/object.go (right):

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/object...
go/src/infra/libs/git/object.go:24: copy(ret.raw[:], raw)
Why not ret.raw = raw?

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/object...
go/src/infra/libs/git/object.go:43: return hex.EncodeToString(o.raw[:])
Why raw[:]?

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/tree.go
File go/src/infra/libs/git/tree.go (right):

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/tree.g...
go/src/infra/libs/git/tree.go:20: cachedRawString *string
You could use a string (non pointer) and just do t.cachedRawString != "" as the
check in Complete.

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/tree.g...
go/src/infra/libs/git/tree.go:262: }
Spacing

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/tree.g...
go/src/infra/libs/git/tree.go:265: func (t *Tree) intern(r *Repo, allowMissing
bool) {
Can you add a short comment about what it means?

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/tree.g...
go/src/infra/libs/git/tree.go:271: c := c
?

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/types.go
File go/src/infra/libs/git/types.go (right):

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/types....
go/src/infra/libs/git/types.go:25: type Repo struct {
Please move all the Repo member function into the same file.

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/types....
go/src/infra/libs/git/types.go:44: type Object interface {
Could you note in the comment of each struct that "It implements interface
Object."

Most of them also implement fmt.Stringer

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/types....
go/src/infra/libs/git/types.go:78: raw [20]byte
Ah, that's why you copy around. I think a slice would be fine. I don't mind.

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/gitiles/gi...
File go/src/infra/libs/gitiles/gitiles.go (right):

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/gitiles/gi...
go/src/infra/libs/gitiles/gitiles.go:43: func (s StatusError) Bad() bool     {
return !(s >= 200 && s < 300) }
return s < 200 || s >= 300
is actually shorter and IMHO more readable

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/gitiles/gi...
go/src/infra/libs/gitiles/gitiles.go:57: type JSONResult struct {
Personally I had used specialized handlers, e.g. line 54
https://github.com/maruel/gddo/commit/b6c003e8c8e3dfc2d4c3800b79a003e389b5a9d2

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/gitiles/gi...
go/src/infra/libs/gitiles/gitiles.go:161: // Lanched as a goroutine to avoid
blocking the request processor.
Launched

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/gitiles/gi...
go/src/infra/libs/gitiles/gitiles.go:183: err = &e
why not?
err = e

https://codereview.chromium.org/662113003/diff/1/go/src/infra/tools/drover/de...
File go/src/infra/tools/drover/default_shell_darwin.go (right):

https://codereview.chromium.org/662113003/diff/1/go/src/infra/tools/drover/de...
go/src/infra/tools/drover/default_shell_darwin.go:2: 
I assume you meant

//+ build darwin


E.g. http://golang.org/pkg/go/build/ "Build Constraints"

https://codereview.chromium.org/662113003/diff/1/go/src/infra/tools/drover/di...
File go/src/infra/tools/drover/disambiguate.go (right):

https://codereview.chromium.org/662113003/diff/1/go/src/infra/tools/drover/di...
go/src/infra/tools/drover/disambiguate.go:107: var candidates
sortableRefCandidates = make([]refCandidate, 0, 10)
candidates := make(sortableRefCandidates, 0, 10)

https://codereview.chromium.org/662113003/diff/1/go/src/infra/tools/drover/ma...
File go/src/infra/tools/drover/main.go (right):

https://codereview.chromium.org/662113003/diff/1/go/src/infra/tools/drover/ma...
go/src/infra/tools/drover/main.go:16: var action = flag.String("action", "",
"The action to take. Must specify `cherry-pick|revert`.")
Why not two boolean flags instead?

https://codereview.chromium.org/662113003/diff/1/go/src/infra/tools/drover/ma...
go/src/infra/tools/drover/main.go:20: func init() {
No need for init in package main, just put it at the start of main. It's more
readable IMHO.

https://codereview.chromium.org/662113003/diff/1/go/src/infra/tools/drover/ma...
go/src/infra/tools/drover/main.go:153: os.Exit(1)
I generally prefer to have something that looks like:

func main() {
  os.Exit(mainImpl())
}

func mainImpl() int{
  ...
  return 1
}

The reason is that defer's work.

https://codereview.chromium.org/662113003/diff/1/go/src/infra/tools/drover/ut...
File go/src/infra/tools/drover/util.go (right):

https://codereview.chromium.org/662113003/diff/1/go/src/infra/tools/drover/ut...
go/src/infra/tools/drover/util.go:11: os.Exit(1)
That's a bit severe and hard to test.

[iannucci, 2014-10-20 21:11:55]

iannucci@chromium.org changed reviewers:
+ vadimsh@chromium.org

[iannucci, 2014-10-20 21:11:58]

+vadimsh

lots of fixes, PTAL

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/blob.go
File go/src/infra/libs/git/blob.go (right):

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/blob.g...
go/src/infra/libs/git/blob.go:1: package git
On 2014/10/18 00:47:04, M-A Ruel wrote:
> Chromium Copyright.

Done.

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/commit.go
File go/src/infra/libs/git/commit.go (right):

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/commit...
go/src/infra/libs/git/commit.go:3: import "bytes"
On 2014/10/18 00:47:05, M-A Ruel wrote:
> import (
>   "bytes"
>   "fmt"
> ..
> )

Done.

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/commit...
go/src/infra/libs/git/commit.go:27: return fmt.Sprintf("%s <%s> %d %s", u.Name,
u.Email, u.Time.Unix(),
On 2014/10/18 00:47:04, M-A Ruel wrote:
> No word wrap, use "gomt -w -s *.go" and "goimports -w ."

I did... it doesn't care about wrap. Are we abandoning 80 cols? I'm OK with
this, but I think it's a bigger discussion, no?

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/commit...
go/src/infra/libs/git/commit.go:28: u.Time.Format("-0700"))
On 2014/10/18 00:47:04, M-A Ruel wrote:
> PDT, really?

It's the example format, not a hard-coded string... it's a weird syntax but
check out the time package.

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/commit...
go/src/infra/libs/git/commit.go:34: type Commit struct {
On 2014/10/18 00:47:05, M-A Ruel wrote:
> Is it implementing an interface? Otherwise why all these getters?

Discussed offline. This is to make the object immutable.

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/commit...
go/src/infra/libs/git/commit.go:37: // TODO(riannucci): Make these real Object's
On 2014/10/18 00:47:04, M-A Ruel wrote:
> iannucci

Oops. Bad text macro. Done.

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/commit...
go/src/infra/libs/git/commit.go:47: messageRawLines *[]string
On 2014/10/18 00:47:05, M-A Ruel wrote:
> Why *[]string and not []string?

because I want to distinguish between set-and-empty and just empty, and I forgot
that slices can be nil :p. Done.

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/commit...
go/src/infra/libs/git/commit.go:50: footers         *map[string][]string
On 2014/10/18 00:47:05, M-A Ruel wrote:
> why pointer?

Same as above.

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/commit...
go/src/infra/libs/git/commit.go:54: func (c *Commit) Type() string   { return
"commit" }
On 2014/10/18 00:47:04, M-A Ruel wrote:
> You should:
> 
> type ObjectType string
> const (
>   CommitType = "commit"
>   BlobType = "blob"
> ...
> )
> 
> that would catch typo, enforce strictness, etc.

Better, made it a uint8 implementing String().

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/commit...
go/src/infra/libs/git/commit.go:61: func (c *Commit) ExtraHeaders() (r []string)
{ return append(r, c.extraHeaders...) }
On 2014/10/18 00:47:05, M-A Ruel wrote:
> You meant:
> return c.extraHeaders[:]

This is to actually copy the headers, since this object is supposed to be
immutable.

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/commit...
go/src/infra/libs/git/commit.go:62: func (c *Commit) MessageRaw() string        
{ return c.messageRaw }
On 2014/10/18 00:47:05, M-A Ruel wrote:
> Note that golang string is unicode, but it's possible that git commit messages
> to not be in utf-8.

Hm... I thought that go strings were actually just byte sequences:
http://blog.golang.org/strings

"""In Go, a string is in effect a read-only slice of bytes... It's important to
state right up front that a string holds arbitrary bytes. It is not required to
hold Unicode text, UTF-8 text, or any other predefined format."""

Is this documentation incorrect?

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/commit...
go/src/infra/libs/git/commit.go:240: var UserRegex = regexp.MustCompile(
On 2014/10/18 00:47:05, M-A Ruel wrote:
> it's not private if it is Upper case

Oops. Done.

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/commit...
go/src/infra/libs/git/commit.go:249: if r := recover(); r != nil {
On 2014/10/18 00:47:05, M-A Ruel wrote:
> what would panic that you'd want to catch this way?

Leftover code :). Removed.

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/diff.go
File go/src/infra/libs/git/diff.go (right):

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/diff.g...
go/src/infra/libs/git/diff.go:6: func (r *Repo) GetTextDiff(left, right string)
[]string {
On 2014/10/18 00:47:05, M-A Ruel wrote:
> Why not implement in repo.go or where ever you define Repo?

Done.

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/diff.g...
go/src/infra/libs/git/diff.go:6: func (r *Repo) GetTextDiff(left, right string)
[]string {
On 2014/10/18 00:47:05, M-A Ruel wrote:
> Why []string instead of string?

Fair enough.

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/diff.g...
go/src/infra/libs/git/diff.go:9: panic(fmt.Errorf("cannot diff(%s, %s): %s",
left, right, rslt))
On 2014/10/18 00:47:05, M-A Ruel wrote:
> No, change the signature to return ([]string, error)
> then
> return nil, fmt.Errorf(...

Done.

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/diff.g...
go/src/infra/libs/git/diff.go:11: return strings.Split(rslt, "\n")
On 2014/10/18 00:47:05, M-A Ruel wrote:
> return strings.Split(rslt, "\n"), nil

Done.

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/diff_t...
File go/src/infra/libs/git/diff_tree.go (right):

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/diff_t...
go/src/infra/libs/git/diff_tree.go:34: func (t TreeDiffEntryHalf) String()
string {
On 2014/10/18 00:47:05, M-A Ruel wrote:
> Use a pointer, otherwise is copy the struct

Done.

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/get_ob...
File go/src/infra/libs/git/get_object.go (right):

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/get_ob...
go/src/infra/libs/git/get_object.go:79: FullTree  fullTree = true
On 2014/10/18 00:47:05, M-A Ruel wrote:
> move constants and simple types at the top

Done.

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/get_ob...
go/src/infra/libs/git/get_object.go:103: p := p
On 2014/10/18 00:47:05, M-A Ruel wrote:
> Oops?

Nope, to avoid aliasing p in the func()

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/get_ob...
go/src/infra/libs/git/get_object.go:151: func (r *Repo) ensureCatFileServer(ch
**chan<- catFileRequest, checkOnly bool) {
On 2014/10/18 00:47:05, M-A Ruel wrote:
> Double pointer is unnecessary and you shouldn't normally need unsafe at all.
> Please refactor.

Come again? How do I safely start the service lazily then?

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/get_ob...
go/src/infra/libs/git/get_object.go:183: panic(err)
On 2014/10/18 00:47:05, M-A Ruel wrote:
> Send error back in channel

Nowhere to send it to, this condition should never happen. There's no reasonable
recovery here.

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/intern.go
File go/src/infra/libs/git/intern.go (right):

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/intern...
go/src/infra/libs/git/intern.go:6: // InternResult contians either the ID of the
intern'd object, or an error.
On 2014/10/18 00:47:05, M-A Ruel wrote:
> contains

Done (removed this struct entirely)

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/object.go
File go/src/infra/libs/git/object.go (right):

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/object...
go/src/infra/libs/git/object.go:24: copy(ret.raw[:], raw)
On 2014/10/18 00:47:06, M-A Ruel wrote:
> Why not ret.raw = raw?

because I can't trust that the caller won't modify raw after the call, and raw
is a [20]byte, not a []byte

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/object...
go/src/infra/libs/git/object.go:43: return hex.EncodeToString(o.raw[:])
On 2014/10/18 00:47:06, M-A Ruel wrote:
> Why raw[:]?

Because raw is a [20]byte and not a []byte.

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/tree.go
File go/src/infra/libs/git/tree.go (right):

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/tree.g...
go/src/infra/libs/git/tree.go:20: cachedRawString *string
On 2014/10/18 00:47:06, M-A Ruel wrote:
> You could use a string (non pointer) and just do t.cachedRawString != "" as
the
> check in Complete.

It's possible to have an empty tree. I need to distinguish this case.

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/tree.g...
go/src/infra/libs/git/tree.go:262: }
On 2014/10/18 00:47:06, M-A Ruel wrote:
> Spacing

It's what gofmt does. What did you have in mind?

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/tree.g...
go/src/infra/libs/git/tree.go:265: func (t *Tree) intern(r *Repo, allowMissing
bool) {
On 2014/10/18 00:47:06, M-A Ruel wrote:
> Can you add a short comment about what it means?

Done.

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/tree.g...
go/src/infra/libs/git/tree.go:271: c := c
On 2014/10/18 00:47:06, M-A Ruel wrote:
> ?

Again, avoids loop variable aliasing. See bottom of
http://golang.org/doc/faq#closures_and_goroutines

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/types.go
File go/src/infra/libs/git/types.go (right):

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/types....
go/src/infra/libs/git/types.go:25: type Repo struct {
On 2014/10/18 00:47:06, M-A Ruel wrote:
> Please move all the Repo member function into the same file.

Done.

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/types....
go/src/infra/libs/git/types.go:44: type Object interface {
On 2014/10/18 00:47:06, M-A Ruel wrote:
> Could you note in the comment of each struct that "It implements interface
> Object."
> 
> Most of them also implement fmt.Stringer

Is that really necessary to note? I thought godoc could infer implementations of
interfaces.

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/types....
go/src/infra/libs/git/types.go:78: raw [20]byte
On 2014/10/18 00:47:06, M-A Ruel wrote:
> Ah, that's why you copy around. I think a slice would be fine. I don't mind.

It' has a fixed size... this also means the empty value is correctly the
"no-hash" value of 20 zero-bytes (which git also uses internally as "not a real
value").

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/gitiles/gi...
File go/src/infra/libs/gitiles/gitiles.go (right):

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/gitiles/gi...
go/src/infra/libs/gitiles/gitiles.go:43: func (s StatusError) Bad() bool     {
return !(s >= 200 && s < 300) }
On 2014/10/18 00:47:06, M-A Ruel wrote:
> return s < 200 || s >= 300
> is actually shorter and IMHO more readable

Done.

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/gitiles/gi...
go/src/infra/libs/gitiles/gitiles.go:57: type JSONResult struct {
On 2014/10/18 00:47:06, M-A Ruel wrote:
> Personally I had used specialized handlers, e.g. line 54
> https://github.com/maruel/gddo/commit/b6c003e8c8e3dfc2d4c3800b79a003e389b5a9d2

Made this struct private. no reason to expose to caller.

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/gitiles/gi...
go/src/infra/libs/gitiles/gitiles.go:161: // Lanched as a goroutine to avoid
blocking the request processor.
On 2014/10/18 00:47:06, M-A Ruel wrote:
> Launched

Done.

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/gitiles/gi...
go/src/infra/libs/gitiles/gitiles.go:183: err = &e
On 2014/10/18 00:47:06, M-A Ruel wrote:
> why not?
> err = e

Oops, done.

https://codereview.chromium.org/662113003/diff/1/go/src/infra/tools/drover/de...
File go/src/infra/tools/drover/default_shell_darwin.go (right):

https://codereview.chromium.org/662113003/diff/1/go/src/infra/tools/drover/de...
go/src/infra/tools/drover/default_shell_darwin.go:2: 
On 2014/10/18 00:47:06, M-A Ruel wrote:
> I assume you meant
> 
> //+ build darwin
> 
> 
> E.g. http://golang.org/pkg/go/build/ "Build Constraints"

Don't need to, it's implied by the file name. Keep reading the link you pasted
:)

https://codereview.chromium.org/662113003/diff/1/go/src/infra/tools/drover/di...
File go/src/infra/tools/drover/disambiguate.go (right):

https://codereview.chromium.org/662113003/diff/1/go/src/infra/tools/drover/di...
go/src/infra/tools/drover/disambiguate.go:107: var candidates
sortableRefCandidates = make([]refCandidate, 0, 10)
On 2014/10/18 00:47:06, M-A Ruel wrote:
> candidates := make(sortableRefCandidates, 0, 10)

Der... dunno what I was thinking.

https://codereview.chromium.org/662113003/diff/1/go/src/infra/tools/drover/ma...
File go/src/infra/tools/drover/main.go (right):

https://codereview.chromium.org/662113003/diff/1/go/src/infra/tools/drover/ma...
go/src/infra/tools/drover/main.go:16: var action = flag.String("action", "",
"The action to take. Must specify `cherry-pick|revert`.")
On 2014/10/18 00:47:06, M-A Ruel wrote:
> Why not two boolean flags instead?

They're mutually exclusive.

https://codereview.chromium.org/662113003/diff/1/go/src/infra/tools/drover/ma...
go/src/infra/tools/drover/main.go:20: func init() {
On 2014/10/18 00:47:06, M-A Ruel wrote:
> No need for init in package main, just put it at the start of main. It's more
> readable IMHO.

Done.

[M-A Ruel, 2014-10-21 00:55:55]

I think it's enough comments for now. :)

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/commit.go
File go/src/infra/libs/git/commit.go (right):

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/commit...
go/src/infra/libs/git/commit.go:27: return fmt.Sprintf("%s <%s> %d %s", u.Name,
u.Email, u.Time.Unix(),
On 2014/10/20 21:11:56, iannucci wrote:
> On 2014/10/18 00:47:04, M-A Ruel wrote:
> > No word wrap, use "gomt -w -s *.go" and "goimports -w ."
> 
> I did... it doesn't care about wrap. Are we abandoning 80 cols? I'm OK with
> this, but I think it's a bigger discussion, no?

Personally, I wrap comments at 80 cols but not function calls. I agree this can
be discussed.

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/commit...
go/src/infra/libs/git/commit.go:62: func (c *Commit) MessageRaw() string        
{ return c.messageRaw }
On 2014/10/20 21:11:56, iannucci wrote:
> On 2014/10/18 00:47:05, M-A Ruel wrote:
> > Note that golang string is unicode, but it's possible that git commit
messages
> > to not be in utf-8.
> 
> Hm... I thought that go strings were actually just byte sequences:
> http://blog.golang.org/strings
> 
> """In Go, a string is in effect a read-only slice of bytes... It's important
to
> state right up front that a string holds arbitrary bytes. It is not required
to
> hold Unicode text, UTF-8 text, or any other predefined format."""
> 
> Is this documentation incorrect?

The very last sentence ends with:
".. although they may contain arbitrary bytes, UTF-8 is a central part of their
design."

When string's are caried around, they are assumed to be utf-8. At the moment
it's fine, just keep in mind that there's a encoding flag in git that could be
used and it'd be good to add a comment near messageRaw that it is assumed to be
in utf-8.

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/get_ob...
File go/src/infra/libs/git/get_object.go (right):

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/get_ob...
go/src/infra/libs/git/get_object.go:103: p := p
On 2014/10/20 21:11:57, iannucci wrote:
> On 2014/10/18 00:47:05, M-A Ruel wrote:
> > Oops?
> 
> Nope, to avoid aliasing p in the func()

It's because you need to call with p as a parameter;

go func(p foo, c bar) {
   ...
}(p, c)

I prefer this to explicit aliasing. Better is likely to create a named function
outside this scope. It's less risky.

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/tree.go
File go/src/infra/libs/git/tree.go (right):

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/tree.g...
go/src/infra/libs/git/tree.go:262: }
On 2014/10/20 21:11:57, iannucci wrote:
> On 2014/10/18 00:47:06, M-A Ruel wrote:
> > Spacing
> 
> It's what gofmt does. What did you have in mind?

gofmt allows both no empty line and one empty line between file level symbols.

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/types.go
File go/src/infra/libs/git/types.go (right):

https://codereview.chromium.org/662113003/diff/1/go/src/infra/libs/git/types....
go/src/infra/libs/git/types.go:44: type Object interface {
On 2014/10/20 21:11:58, iannucci wrote:
> On 2014/10/18 00:47:06, M-A Ruel wrote:
> > Could you note in the comment of each struct that "It implements interface
> > Object."
> > 
> > Most of them also implement fmt.Stringer
> 
> Is that really necessary to note? I thought godoc could infer implementations
of
> interfaces.

It's not necessary but nice to document.

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/bl...
File go/src/infra/libs/git/blob.go (right):

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/bl...
go/src/infra/libs/git/blob.go:4: package git
you need to keep an empty line, otherwise it is considered package
documentation. You can confirm with godoc -http=:6060

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/bl...
go/src/infra/libs/git/blob.go:9: data string
So you really use string here as []byte. I think I'd prefer []byte. String has
implication that it is somehow human readable.

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/bl...
go/src/infra/libs/git/blob.go:22: // BlobFromRawWithID creates a new *Blob,
trusting |id|. There is no verification
Generally I personally ensure these docstrings are 80 cols.

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/co...
File go/src/infra/libs/git/commit.go (right):

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/co...
go/src/infra/libs/git/commit.go:33: u.Time.Format("-0700"))
Either use the local time or UTC, but do not hardcode the time zone.

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/co...
go/src/infra/libs/git/commit.go:86: func (c *Commit) SetTree(t ObjectID) (ret
*Commit) {
That still means that SetTree() modifies the caller if it was not set. I think
it's unexpected behavior.

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/co...
go/src/infra/libs/git/commit.go:211: // Parses the message for footer_pairs,
footers, message_lines
parseMessage parses ...

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/co...
go/src/infra/libs/git/commit.go:214: i := -1
i := len(allLines)

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/co...
go/src/infra/libs/git/commit.go:215: for i = len(allLines) - 1; i >= 0; i-- {
for ; i>= 0; i-- {

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/co...
go/src/infra/libs/git/commit.go:234: key, value := bits[0], bits[1]
This will crash if there was no ": "

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/co...
go/src/infra/libs/git/commit.go:266: t, err := time.Parse("-0700", fields[5])
I'm not a big fan of named return value. This line is a good example, where it's
ambiguous if err is being aliased or not. I'd recommend against.

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/ob...
File go/src/infra/libs/git/object.go (right):

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/ob...
go/src/infra/libs/git/object.go:12: type ObjectType uint8
If it's exported, it's worth documenting. :)

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/ob...
go/src/infra/libs/git/object.go:18: TreeType
Y U NO TAG? Even if you explicitly do not support it, it's worth stating this
explicitly.

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/ob...
go/src/infra/libs/git/object.go:69: type ObjectID struct {
It's fine and shorter to use 
type ObjectID [20] byte

iif you want. I don't mind.

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/ob...
go/src/infra/libs/git/object.go:98: panic("MakeObjectID() must be invoked with a
40 byte hex string")
In general, packages should not panic. It's really painful to handle otherwise,
even for trivial things like this.

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/re...
File go/src/infra/libs/git/repo.go (right):

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/re...
go/src/infra/libs/git/repo.go:24: NoBlobs           = false
If the type is not exported, the consts should not be exported either.

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/re...
go/src/infra/libs/git/repo.go:39: catFile      *chan<- catFileRequest
I still don't understand why it's a pointer. Create one instance on object
creation and it's fine.

Also you can consider the option of not exporting the structure itself (e.g.
repo instead of Repo) and add a function NewRepo() Repo where Repo is an
interface.

It depends if you think it's valuable, I'm not sure.

Basically, you'd do:

func NewRepo() {
  r := repo{catFile: make(chan catFileRequest), ...}
  go catServer()
  return r
}

and add

func (r*repo) Close() error {
  r.catFile <- catFileRequest{diiie}
  // wait
  return nil
}

so you implement io.Closer.

More importantly, this removes the need for unsafe.

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/re...
go/src/infra/libs/git/repo.go:50: // Actino is one of "ACDMRTUX"
Action

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/re...
go/src/infra/libs/git/repo.go:82: RawString() string
As with the other part, I think []byte is a bit more appropriate, which would
mean renaming the function. :)

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/re...
go/src/infra/libs/git/repo.go:87: func (r *Repo) RunInput(input string, cmd
...string) (string, bool) {
That's an example of a function that could accept a Repo as a parameter.

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/re...
go/src/infra/libs/git/repo.go:161: // HasObject will return true iff the Repo
contains the objectish
objectish or ref?

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/re...
go/src/infra/libs/git/repo.go:172: func (r *Repo) HasObjectID(id ObjectID) bool
{
What about just this function and not have HasObject()? It's much clearer IMHO.

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/re...
go/src/infra/libs/git/repo.go:185: func (r *Repo) GetFullTree(treeish string, b
blobOpt, f fullTree) *Tree {
This function likely doesn't need to be a method.

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/re...
go/src/infra/libs/git/repo.go:226: func (r *Repo) GetFullTreeID(tree ObjectID, b
blobOpt, f fullTree) *Tree {
Keep this one, remove GetFullTree()

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/re...
go/src/infra/libs/git/repo.go:231: rslt, ok := r.Run("diff", left, right)
At the very least, you want --no-ext-diff

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/re...
go/src/infra/libs/git/repo.go:245: atoi := func(s string, base int) int {
Another external function.

I think it should accept ObjectID.

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/re...
go/src/infra/libs/git/repo.go:301: func (r *Repo) Intern(obj InternableObject)
(ObjectID, error) {
I find the name surprising.

I would have thought about AppendObjectToRepo or something like that but I don't
mind that much.

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/re...
go/src/infra/libs/git/repo.go:340: reply     chan<- *catFileReply
I think it's fine to reply instances, the object is not big.

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/re...
go/src/infra/libs/git/repo.go:363: fmt.Println("recovering panick'd
catFileServer", err)
Why recover()?

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/tr...
File go/src/infra/libs/git/tree.go (right):

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/tr...
go/src/infra/libs/git/tree.go:18: type Mode int
Mode is maybe a tad too generic. But I see that it also applies to commit
objects, so I don't know.

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/tr...
go/src/infra/libs/git/tree.go:30: panic(fmt.Sprintf("unknown type for mode %d",
m))
fmt.Errorf()

but I don't like that input data can crash a service.

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/tr...
go/src/infra/libs/git/tree.go:65: func (t *Tree) ID() ObjectID { return t.id }
Why not call Rehash automatically?

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/tr...
go/src/infra/libs/git/tree.go:126: // TreeFromText returns a Complete() Tree for
a `git ls-tree`-formatted
Note that there could be cases where it doesn't fit in memory.

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/tr...
go/src/infra/libs/git/tree.go:192: panic(fmt.Errorf("intermediate child %s
doesn't exist", dir))
return an error, it's not panic worthy.

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/tr...
go/src/infra/libs/git/tree.go:258: if t.cachedRawString == nil {
It's not necessary that it is a pointer, because it can't be "" and still be
valid, so you can do instead:

if t.cachedRawString == "" {
  ...

and not use a pointer.

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/tr...
go/src/infra/libs/git/tree.go:282: /// Private
assuming is a file block section, add an empty line.

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/tr...
go/src/infra/libs/git/tree.go:307: // Interns the Tree recursively. If
allowMissing is true, this will not panic
// intern interns the ..

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/tr...
go/src/infra/libs/git/tree.go:341: r.Intern(t)
What about a directory containing a directory that is empty?

[Vadim Sh., 2014-10-21 15:27:00]

I don't know go and don't know git internals, so do not mind my review too much
:) (and it's unfinished, will continue later).

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/hello_pkg/h...
File go/src/infra/hello_pkg/hello_pkg.go (right):

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/hello_pkg/h...
go/src/infra/hello_pkg/hello_pkg.go:1: // Copyright 2014 The Chromium Authors.
All rights reserved.
You can probably remove hello_pkg and other temporary stuff there, now that we
have a real Go code.

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/co...
File go/src/infra/libs/git/commit.go (right):

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/co...
go/src/infra/libs/git/commit.go:23: // User represents an author/committer line
in a Commit
nit: end sentence with dot, same in other places.

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/ob...
File go/src/infra/libs/git/object.go (right):

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/ob...
go/src/infra/libs/git/object.go:30: return "UNKNOWN"
panic too?

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/ob...
go/src/infra/libs/git/object.go:91: return
is this 'return' required here?

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/ob...
go/src/infra/libs/git/object.go:98: panic("MakeObjectID() must be invoked with a
40 byte hex string")
On 2014/10/21 00:55:54, M-A Ruel wrote:
> In general, packages should not panic. It's really painful to handle
otherwise,
> even for trivial things like this.

IMHO, panic is like assert. If caller doesn't fullfil the contract, it's
caller's fault.

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/re...
File go/src/infra/libs/git/repo.go (right):

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/re...
go/src/infra/libs/git/repo.go:97: panic(err)
when this can happen?

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/re...
go/src/infra/libs/git/repo.go:122: // return a channel for the result. If the
object is missing, GetObject will
I don't see it returning a channel... Also it's blocking. Probably stale doc
string.

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/re...
go/src/infra/libs/git/repo.go:127: rchan := make(chan *catFileReply, 1)
Does go have thread pool implementation?

IMHO, it's conceptually simpler to push callbacks to a generic thread pool,
rather then specially crafted structs to some specialized goroutine.

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/re...
go/src/infra/libs/git/repo.go:226: func (r *Repo) GetFullTreeID(tree ObjectID, b
blobOpt, f fullTree) *Tree {
At this point you gave up writing comments? :)

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/re...
go/src/infra/libs/git/repo.go:346: swapped := atomic.CompareAndSwapPointer(
fancy

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/re...
go/src/infra/libs/git/repo.go:392: for req := range rchan {
who closes rchan? When the goroutine die?

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/gitile...
File go/src/infra/libs/gitiles/gitiles.go (right):

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/gitile...
go/src/infra/libs/gitiles/gitiles.go:30: func NewGitiles(url string,
maxConnections int) *Gitiles {
there's some idiomatic Go way to support HTTP midleware (like OAuth
authentication), some set of interfaces... e.g.. instead of 'url' you can accept
Transport or HttpClient or something like that (I do not remember).

tl;dr this stuff doesn't work with non-public repos?

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/gitile...
go/src/infra/libs/gitiles/gitiles.go:38: for i := 0; i < maxConnections; i++ {
Isn't there built in connection pool? :(

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/infra_...
File go/src/infra/libs/infra_util/buf.go (right):

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/infra_...
go/src/infra/libs/infra_util/buf.go:28: func Yoink(buf io.Reader) func(int)
[]byte {
"
An exclamation that, when uttered in conjunction with taking an object,
immediately transfers ownership from the original owner to the person using the
word regardless of previous property rights.
"

o_O

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/tools/drove...
File go/src/infra/tools/drover/main.go (right):

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/tools/drove...
go/src/infra/tools/drover/main.go:14: "github.com/daviddengcn/go-colortext"
did you manage to add this to Goop file?

[agable, 2014-10-21 17:01:35]

Didn't make it through all of this (sick today :() but here are some initial
shallow comments. Doubt I'll have much of substance once maruel@ and vadmish@
are done with your Go :)

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/co...
File go/src/infra/libs/git/commit.go (right):

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/co...
go/src/infra/libs/git/commit.go:42: // TODO(iannucci): Make these real Object's
nit: Objects

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/co...
go/src/infra/libs/git/commit.go:89: return
Is the explicit return statement necessary for the named output to work, even
though it is the last line of the function?

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/co...
go/src/infra/libs/git/commit.go:119: c.id = MakeObjectIDForData(CommitType,
buf.Bytes())
Creating a raw string representation of a commit shouldn't modify the commit
struct as a side effect.

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/co...
go/src/infra/libs/git/commit.go:146: //   ([^:]*):\s(.*)
Doesn't seem quite right. Empty keys and values are allowed? And only one
character of whitespace after the colon? Should be
([^:]+):\s+(.+)
or something. Also, the parsing code actually requires a space, not just a
character of whitespace.

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/co...
go/src/infra/libs/git/commit.go:165: // CommitFromRaw returns a Commit parsed
from the hash-object compatible commit
nit: I like the earlier "...`git hash-object` compatible..." phrasing better.

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/co...
go/src/infra/libs/git/commit.go:177: // origin of |data|.
...and of |id|.

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/co...
go/src/infra/libs/git/commit.go:266: t, err := time.Parse("-0700", fields[5])
On 2014/10/21 00:55:54, M-A Ruel wrote:
> I'm not a big fan of named return value. This line is a good example, where
it's
> ambiguous if err is being aliased or not. I'd recommend against.

I'm fine with this usage; it's one of the first things pointed out as being
'canonical go' in the Go intro I was reading just a couple weeks ago. I'm not
sure how err could be 'aliased' here -- there's only one scope it could be in.

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/ob...
File go/src/infra/libs/git/object.go (right):

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/ob...
go/src/infra/libs/git/object.go:57: // Note that Tree's are never EmptyObject's.
nit: apostrophes

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/ob...
go/src/infra/libs/git/object.go:69: type ObjectID struct {
On 2014/10/21 00:55:54, M-A Ruel wrote:
> It's fine and shorter to use 
> type ObjectID [20] byte
> 
> iif you want. I don't mind.

Was going to say the same thing. Not sure why this is a struct.

[Vadim Sh., 2015-06-03 18:45:24]

vadimsh@chromium.org changed reviewers:
- vadimsh@chromium.org

[iannucci, 2016-05-23 21:53:43]

sending comments for historical purposes; this CL is dead.

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/bl...
File go/src/infra/libs/git/blob.go (right):

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/bl...
go/src/infra/libs/git/blob.go:4: package git
On 2014/10/21 00:55:53, M-A Ruel wrote:
> you need to keep an empty line, otherwise it is considered package
> documentation. You can confirm with godoc -http=:6060

oh, yikes, ok

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/bl...
go/src/infra/libs/git/blob.go:9: data string
On 2014/10/21 00:55:53, M-A Ruel wrote:
> So you really use string here as []byte. I think I'd prefer []byte. String has
> implication that it is somehow human readable.

Well... most blobs /are/ human readable (usually text files). The other
advantage is that string is immutable but []byte is not, which means one less
copy.

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/bl...
go/src/infra/libs/git/blob.go:22: // BlobFromRawWithID creates a new *Blob,
trusting |id|. There is no verification
On 2014/10/21 00:55:53, M-A Ruel wrote:
> Generally I personally ensure these docstrings are 80 cols.

Done.

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/co...
File go/src/infra/libs/git/commit.go (right):

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/co...
go/src/infra/libs/git/commit.go:33: u.Time.Format("-0700"))
On 2014/10/21 00:55:53, M-A Ruel wrote:
> Either use the local time or UTC, but do not hardcode the time zone.

Again, it's not hard coded :) Read Time.Format docs:
http://golang.org/pkg/time/#Time.Format

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/co...
go/src/infra/libs/git/commit.go:86: func (c *Commit) SetTree(t ObjectID) (ret
*Commit) {
On 2014/10/21 00:55:53, M-A Ruel wrote:
> That still means that SetTree() modifies the caller if it was not set. I think
> it's unexpected behavior.

It modifies the caller only if the caller started with a partial commit (e.g.
wasn't hashed, so id == NoID).

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/co...
go/src/infra/libs/git/commit.go:211: // Parses the message for footer_pairs,
footers, message_lines
On 2014/10/21 00:55:53, M-A Ruel wrote:
> parseMessage parses ...

Done.

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/co...
go/src/infra/libs/git/commit.go:214: i := -1
On 2014/10/21 00:55:54, M-A Ruel wrote:
> i := len(allLines)

Done.

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/co...
go/src/infra/libs/git/commit.go:215: for i = len(allLines) - 1; i >= 0; i-- {
On 2014/10/21 00:55:53, M-A Ruel wrote:
> for ; i>= 0; i-- {

Done.

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/co...
go/src/infra/libs/git/commit.go:234: key, value := bits[0], bits[1]
On 2014/10/21 00:55:53, M-A Ruel wrote:
> This will crash if there was no ": "

yep, which is impossible due to line 219.

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/co...
go/src/infra/libs/git/commit.go:266: t, err := time.Parse("-0700", fields[5])
On 2014/10/21 00:55:54, M-A Ruel wrote:
> I'm not a big fan of named return value. This line is a good example, where
it's
> ambiguous if err is being aliased or not. I'd recommend against.

Done.

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/ob...
File go/src/infra/libs/git/object.go (right):

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/ob...
go/src/infra/libs/git/object.go:18: TreeType
On 2014/10/21 00:55:54, M-A Ruel wrote:
> Y U NO TAG? Even if you explicitly do not support it, it's worth stating this
> explicitly.

Omission, though "tag" doesn't show up in tree objects. I added it anyway, since
trees explicitly scope to just {Commit, Blob, Tree} types, and it could show up
in a cat-file request.

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/ob...
go/src/infra/libs/git/object.go:69: type ObjectID struct {
On 2014/10/21 00:55:54, M-A Ruel wrote:
> It's fine and shorter to use 
> type ObjectID [20] byte
> 
> iif you want. I don't mind.

Modifiable, and I can't have a type alias whose 'real' type is private to the
package :(

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/re...
File go/src/infra/libs/git/repo.go (right):

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/re...
go/src/infra/libs/git/repo.go:24: NoBlobs           = false
On 2014/10/21 00:55:54, M-A Ruel wrote:
> If the type is not exported, the consts should not be exported either.

Why not? I don't want them constructing their own flavor of it (e.g. I can
reserve the right to change the underlying type), but let them use the constant
values. That seems reasonable to me, but maybe I'm missing something?

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/re...
go/src/infra/libs/git/repo.go:50: // Actino is one of "ACDMRTUX"
On 2014/10/21 00:55:54, M-A Ruel wrote:
> Action

oops. done.

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/re...
go/src/infra/libs/git/repo.go:82: RawString() string
On 2014/10/21 00:55:54, M-A Ruel wrote:
> As with the other part, I think []byte is a bit more appropriate, which would
> mean renaming the function. :)

but then I have to copy all that data or ppl could modify the underlying stuff
:(

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/re...
go/src/infra/libs/git/repo.go:87: func (r *Repo) RunInput(input string, cmd
...string) (string, bool) {
On 2014/10/21 00:55:54, M-A Ruel wrote:
> That's an example of a function that could accept a Repo as a parameter.

I'm not sure why that would be helpful though, since it's also never useful
/without/ a Repo.

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/re...
go/src/infra/libs/git/repo.go:161: // HasObject will return true iff the Repo
contains the objectish
On 2014/10/21 00:55:54, M-A Ruel wrote:
> objectish or ref?

objectish. could be a hash, ref, hash:path/to/file, ref~2, etc.

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/re...
go/src/infra/libs/git/repo.go:172: func (r *Repo) HasObjectID(id ObjectID) bool
{
On 2014/10/21 00:55:54, M-A Ruel wrote:
> What about just this function and not have HasObject()? It's much clearer
IMHO.

It is, but it's less powerful.

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/tr...
File go/src/infra/libs/git/tree.go (right):

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/tr...
go/src/infra/libs/git/tree.go:65: func (t *Tree) ID() ObjectID { return t.id }
On 2014/10/21 00:55:55, M-A Ruel wrote:
> Why not call Rehash automatically?

Because if it's an empty tree, rehashing it would cause it to obliterate the ID
information (and replace it with the ID of an empty tree).

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/tr...
go/src/infra/libs/git/tree.go:126: // TreeFromText returns a Complete() Tree for
a `git ls-tree`-formatted
On 2014/10/21 00:55:55, M-A Ruel wrote:
> Note that there could be cases where it doesn't fit in memory.

The thought crossed my mind, but for now I think it's OK. In practice, we don't
have any trees which even approach a megabyte (the /recursive/ enumeration of
blink's LayoutTests directory is 11MB).

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/tr...
go/src/infra/libs/git/tree.go:258: if t.cachedRawString == nil {
On 2014/10/21 00:55:55, M-A Ruel wrote:
> It's not necessary that it is a pointer, because it can't be "" and still be
> valid, so you can do instead:
> 
> if t.cachedRawString == "" {
>   ...
> 
> and not use a pointer.

Except that it /can/ be "" :).

$ git hash-object --stdin -t tree < /dev/null
4b825dc642cb6eb9a060e54bf8d69288fbee4904

tah-dah!

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/tr...
go/src/infra/libs/git/tree.go:282: /// Private
On 2014/10/21 00:55:54, M-A Ruel wrote:
> assuming is a file block section, add an empty line.

Done.

https://codereview.chromium.org/662113003/diff/20001/go/src/infra/libs/git/tr...
go/src/infra/libs/git/tree.go:341: r.Intern(t)
On 2014/10/21 00:55:55, M-A Ruel wrote:
> What about a directory containing a directory that is empty?

It's not a standard thing in git. You can contrive a tree object containing an
entry with an empty tree, but by default git will not produce such a thing.
Hence the existence of .gitkeep files.