Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pure go reimplementation #4

Merged
merged 3 commits into from
Nov 7, 2016
Merged

Pure go reimplementation #4

merged 3 commits into from
Nov 7, 2016

Conversation

spheromak
Copy link
Contributor

@spheromak spheromak commented Oct 31, 2016

Moving away from libgit to somehting more portable. Also fixing a bug where major doesn't set the minor and patch to 0 when it bumps


This change is Reviewable

This should enable cross platform builds as well
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 1a6e65d on pure-go into * on master*.

@spheromak spheromak changed the title [WIP] Pure go reimplementation Pure go reimplementation Nov 2, 2016
type GitRepo struct {
Repo *git.Repository
walker *git.RevWalk
Repo *git.Repository
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be exported?

newVersion *version.Version
branch string
branchID string // commit id of the branch latest commit (where we will apply the tag)
}

// NewRepo is a constructor for a repo object, parsing the tags that exist
// The caller is responsible for calling Free() on the embedded Repo when they are done with it
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still accurate?

@@ -58,56 +55,64 @@ func NewRepo(repoPath string) (*GitRepo, error) {
return r, nil
}

// Parse tags on repo and
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment appears incomplete?

var versions []*version.Version
var lastParsedTagRef string
for _, tag := range tags {
if v, err := maybeVersionFromTag(tag); err == nil {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can reduce the cognitive overhead by changing this to
if err != nil { continue }

if err != nil {
return nil, err
}

r := &GitRepo{
Repo: repo,
walker: w,
branch: branch,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want the NewRepo function to allow empty branch names?


sort.Sort(version.Collection(versions))
// set the current versions
if itemLen := len(keys); itemLen >= 1 {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be if len(keys) == 0 { return err }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then we would need to get the len again to do the end of slice element we get in the following line. Can be simplified with the reverse sort. need to look at it all in that context tho.


start := false
for e := l.Back(); e != nil; e = e.Prev() {
commit := e.Value.(*git.Commit)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this ever be either another type or return a nil pointer?

log.Printf("Checking commits from %s to %s ", r.branchID, tagCommit.Id)

start := false
for e := l.Back(); e != nil; e = e.Prev() {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be comment worthy. Something like,

Chronologically (oldest to newest) process commit history

}

return nil
return r.tagNewVersion()
}

func (r *GitRepo) tagNewVersion() error {
// TODO:(jnelson) These should be configurable? Mon Sep 14 12:02:52 2015
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this comment still apply?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea probably should be able to configure that.

@@ -203,7 +221,7 @@ func (r *GitRepo) tagNewVersion() error {
func (r *GitRepo) parseCommit(commit *git.Commit) (*version.Version, error) {
var b bumper
msg := commit.Message()
log.Println("Parsing ", msg, ":")
log.Printf("Parsing %s: %s", commit.Id, msg)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want a \n in the format string? (this comment might apply to multiple output messages in this repo).

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 65ae775 on pure-go into * on master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling ae47610 on pure-go into * on master*.

@spheromak
Copy link
Contributor Author

@kf6nux addressed most the comments except the append stuff since I am not sure what you are saying we should do there.

@spheromak spheromak merged commit ac15d0a into master Nov 7, 2016
@spheromak spheromak deleted the pure-go branch November 7, 2016 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants