Add ReferenceNormalizeName #681

2020-11-10T10:43:32-06:00

segevfiner commented

2020-11-10 10:43:32 -06:00

(Migrated from github.com)

Not sure about the buffer allocation for passing to git_reference_normalize_name: Is using malloc correct? Is the size correct, or should it be a loop with retries with some increasing buffer size (But the function doesn't seem to tell you how big it should be...)?

Not sure about the buffer allocation for passing to `git_reference_normalize_name`: Is using `malloc` correct? Is the size correct, or should it be a loop with retries with some increasing buffer size (But the function doesn't seem to tell you how big it should be...)?

segevfiner commented

2020-11-10 11:11:54 -06:00

(Migrated from github.com)

Go 1.9?! Really... 🤷‍♂️

lhchavez (Migrated from github.com) reviewed 2020-11-13 07:54:28 -06:00

lhchavez (Migrated from github.com) left a comment

nice! just a few small requests.

reference.go

lhchavez (Migrated from github.com) commented

2020-11-13 07:35:02 -06:00

We can probably take this out since Go doesn't work that way.

We can probably take this out since Go doesn't work that way. ```suggestion ```

lhchavez (Migrated from github.com) commented

2020-11-13 07:37:34 -06:00

since C.GIT_REFNAME_MAX isn't exported, could that be explicitly declared?

const (
    ...
    // This should match GIT_REFNAME_MAX in src/refs.h
    refnameMaxLength = 1024
)

...
func ReferenceNormalizeName(name string, flags ReferenceFormat) (string, error) {
   ...
   bufSize := C.size_t(refnameMaxLength)

since `C.GIT_REFNAME_MAX` isn't exported, could that be explicitly declared? ```go const ( ... // This should match GIT_REFNAME_MAX in src/refs.h refnameMaxLength = 1024 ) ... func ReferenceNormalizeName(name string, flags ReferenceFormat) (string, error) { ... bufSize := C.size_t(refnameMaxLength) ```

reference_test.go

lhchavez (Migrated from github.com) commented

2020-11-13 07:52:55 -06:00

Go prefers to print the want and got when failing tests:

		t.Errorf("ReferenceNormalizeName(%q) = %q; want %q", "refs/heads//master", ref, want)

Same in L241.

Go prefers to print the [want and got](https://golang.org/pkg/testing/) when failing tests: ```suggestion t.Errorf("ReferenceNormalizeName(%q) = %q; want %q", "refs/heads//master", ref, want) ``` Same in L241.

suhaibmujahid (Migrated from github.com) reviewed 2020-11-13 09:30:14 -06:00

suhaibmujahid (Migrated from github.com) left a comment

Thank you for the PR. I have a trivial styling comment.

It is good to prefix unexported globals with _:

const (
	// This should match GIT_REFNAME_MAX in src/refs.h
	_refnameMaxLength = 1024
)

Thank you for the PR. I have a trivial styling comment. It is good to [prefix unexported globals with `_`](https://github.com/uber-go/guide/blob/master/style.md#prefix-unexported-globals-with-_): ```go const ( // This should match GIT_REFNAME_MAX in src/refs.h _refnameMaxLength = 1024 ) ```

reference.go

suhaibmujahid (Migrated from github.com) commented

2020-11-13 09:20:57 -06:00

Since this is not gonna change, you could have it as a const.

const (
	// This should match GIT_REFNAME_MAX in src/refs.h
	refnameMaxLength = 1024
	refnameBufSize = C.size_t(refnameMaxLength)
)

or just have it as:

const (
	// This should match GIT_REFNAME_MAX in src/refs.h
	refnameBufSize = C.size_t(1024)
)

Since this is not gonna change, you could have it as a const. ```go const ( // This should match GIT_REFNAME_MAX in src/refs.h refnameMaxLength = 1024 refnameBufSize = C.size_t(refnameMaxLength) ) ``` or just have it as: ```go const ( // This should match GIT_REFNAME_MAX in src/refs.h refnameBufSize = C.size_t(1024) ) ```