jcarr
/
git2go
1
0
Fork 0
Code Issues 52 Pull Requests 31 Packages Projects Releases Wiki Activity

Rebase wrapper #332

Merged
ezwiebel merged 10 commits from rebase-wrapper into master 2017-04-13 13:50:42 -05:00
Conversation 7 Commits 10 Files Changed 3 +634
ezwiebel commented 2016-08-07 02:47:21 -05:00 (Migrated from github.com)
Copy Link

Hello,

I needed rebase services to be wrapped in git2go so I propose this PR.
Should I propose it on next branch instead of master ?

There's still some work though :

  • test what happens when a conflict arise during the rebase chain
  • manage git_rebase_options and wrap git_rebase_init_options(...)
  • wrap git_rebase_open(...)

I'll probably do the test for merge conflicts as I will need it quite soon.
Other services are of no interest for me at the moment, let me know if you'd like them wrapped as I'm into the work. Shouldn't be too long.

Let me know what you think. Cheers.

Hello, I needed `rebase` services to be wrapped in `git2go` so I propose this PR. Should I propose it on `next` branch instead of `master` ? There's still some work though : - test what happens when a conflict arise during the rebase chain - manage `git_rebase_options` and wrap `git_rebase_init_options(...)` - wrap `git_rebase_open(...)` I'll probably do the test for merge conflicts as I will need it quite soon. Other services are of no interest for me at the moment, let me know if you'd like them wrapped as I'm into the work. Shouldn't be too long. Let me know what you think. Cheers.
ezwiebel commented 2016-08-07 03:19:46 -05:00 (Migrated from github.com)
Copy Link

uh oh, there are probably things I don't understand in the test run.
it runs normally on my side. any clue?

EDIT : My bad, I made a mistake in the Free() service in rebase wrapper, all good now

uh oh, there are probably things I don't understand in the test run. it runs normally on my side. any clue? EDIT : My bad, I made a mistake in the Free() service in rebase wrapper, all good now
carlosmn (Migrated from github.com) reviewed 2016-10-31 14:57:32 -05:00
rebase.go
carlosmn (Migrated from github.com) commented 2016-10-31 14:42:19 -05:00
Copy Link

This describes the structure mechanically, but almost everything is a wrapper, if we're going to have a comment here, it should describe the object itself and/or what you can do with it.

This describes the structure mechanically, but almost everything is a wrapper, if we're going to have a comment here, it should describe the object itself and/or what you can do with it.
carlosmn (Migrated from github.com) commented 2016-10-31 14:43:23 -05:00
Copy Link

We have prior art of naming git_repository_init() as InitRepository() since the namespacing in C and Go is bit different, so this should be called InitRebase().

We have prior art of naming `git_repository_init()` as `InitRepository()` since the namespacing in C and Go is bit different, so this should be called `InitRebase()`.
carlosmn (Migrated from github.com) commented 2016-10-31 14:44:06 -05:00
Copy Link

Like with the init function, this should be called OpenRebase(). The documentation shouldn't reference the C function git_rebase_init but our own.

Like with the init function, this should be called `OpenRebase()`. The documentation shouldn't reference the C function `git_rebase_init` but our own.
carlosmn (Migrated from github.com) commented 2016-10-31 14:45:41 -05:00
Copy Link

This return value does not match how Go reports errors. I would expect a uint to return the current operation and an error to return any errors.

This return value does not match how Go reports errors. I would expect a `uint` to return the current operation and an `error` to return any errors.
carlosmn (Migrated from github.com) commented 2016-10-31 14:46:46 -05:00
Copy Link

There is no GIT_REBASE_OPERATION_EXEC in git2go, it's RebaseOperationExec.

There is no `GIT_REBASE_OPERATION_EXEC` in git2go, it's `RebaseOperationExec`.
carlosmn (Migrated from github.com) commented 2016-10-31 14:47:37 -05:00
Copy Link

There is no git_rebase_next in git2go, we have Next().

There is no `git_rebase_next` in git2go, we have `Next()`.
carlosmn (Migrated from github.com) commented 2016-10-31 14:50:26 -05:00
Copy Link

Missing space. The pointer isn't public so let's not mention it. "unmanaged resources" covers whatever we decide to put in there.

Missing space. The pointer isn't public so let's not mention it. "unmanaged resources" covers whatever we decide to put in there.
carlosmn (Migrated from github.com) commented 2016-10-31 14:54:32 -05:00
Copy Link

The other public id fields are named Id.

The other public id fields are named `Id`.
rebase.go
@ -0,0 +95,4 @@
return nil
}
return C.CString(ref)
}
carlosmn (Migrated from github.com) commented 2016-10-31 14:40:54 -05:00
Copy Link

This looks like it should have a much more generic name, since what it does is unrelated to note rewriting and it's simply mapping an empty string and NULL.

This looks like it should have a much more generic name, since what it does is unrelated to note rewriting and it's simply mapping an empty string and `NULL`.
rebase.go
carlosmn (Migrated from github.com) commented 2016-10-31 14:48:20 -05:00
Copy Link

This leaks the committer signature.

This leaks the committer signature.
rebase_test.go
carlosmn (Migrated from github.com) commented 2016-10-31 14:56:01 -05:00
Copy Link

Don't hard-code 1 here, the point of the version field is to increase it.

Don't hard-code 1 here, the point of the version field is to increase it.
ezwiebel (Migrated from github.com) reviewed 2016-10-31 18:57:15 -05:00
rebase.go
ezwiebel (Migrated from github.com) commented 2016-10-31 18:57:15 -05:00
Copy Link

Ok, I followed go convention suggested by my IDE plugin

Ok, I followed go convention suggested by my IDE plugin
ezwiebel (Migrated from github.com) reviewed 2016-10-31 18:58:29 -05:00
rebase.go
@ -0,0 +95,4 @@
return nil
}
return C.CString(ref)
}
ezwiebel (Migrated from github.com) commented 2016-10-31 18:58:29 -05:00
Copy Link

I understand your point. Renamed to mapEmptyStringToNull :p

I understand your point. Renamed to mapEmptyStringToNull :p
ezwiebel (Migrated from github.com) reviewed 2016-10-31 18:58:39 -05:00
rebase.go
ezwiebel (Migrated from github.com) commented 2016-10-31 18:58:38 -05:00
Copy Link

Done

Done
ezwiebel (Migrated from github.com) reviewed 2016-10-31 18:58:43 -05:00
rebase.go
ezwiebel (Migrated from github.com) commented 2016-10-31 18:58:42 -05:00
Copy Link

Done

Done
ezwiebel (Migrated from github.com) reviewed 2016-10-31 18:58:49 -05:00
rebase.go
ezwiebel (Migrated from github.com) commented 2016-10-31 18:58:49 -05:00
Copy Link

Done

Done
ezwiebel (Migrated from github.com) reviewed 2016-10-31 19:01:02 -05:00
rebase.go
ezwiebel (Migrated from github.com) commented 2016-10-31 19:01:02 -05:00
Copy Link

Done.
I also added ErrClassRebase in git.go
Hope this is correct

Done. I also added `ErrClassRebase` in `git.go` Hope this is correct
ezwiebel (Migrated from github.com) reviewed 2016-10-31 19:01:09 -05:00
rebase.go
ezwiebel (Migrated from github.com) commented 2016-10-31 19:01:09 -05:00
Copy Link

Done

Done
ezwiebel (Migrated from github.com) reviewed 2016-10-31 19:01:19 -05:00
rebase.go
@ -0,0 +200,4 @@
committerSig, err := committer.toC()
if err != nil {
return err
}
ezwiebel (Migrated from github.com) commented 2016-10-31 19:01:19 -05:00
Copy Link

Oups!
Done

Oups! Done
ezwiebel (Migrated from github.com) reviewed 2016-10-31 19:01:27 -05:00
rebase.go
ezwiebel (Migrated from github.com) commented 2016-10-31 19:01:27 -05:00
Copy Link

Done

Done
ezwiebel (Migrated from github.com) reviewed 2016-10-31 19:03:04 -05:00
rebase_test.go
ezwiebel (Migrated from github.com) commented 2016-10-31 19:03:04 -05:00
Copy Link

Not sure how to handle this. It's a test that checks the default version returned by DefaultRebaseOptions. After a look into C code, I thought it was a #define or a constant that was used as default version number.
The test doesn't have great value though

Not sure how to handle this. It's a test that checks the default version returned by DefaultRebaseOptions. After a look into C code, I thought it was a `#define` or a constant that was used as default version number. The test doesn't have great value though
carlosmn (Migrated from github.com) reviewed 2016-11-13 12:33:45 -06:00
rebase_test.go
carlosmn (Migrated from github.com) commented 2016-11-13 12:33:45 -06:00
Copy Link

The version it needs to return in the version that is specified by the libgit2 headers in the #define. That's what the compiler is building and that's what it will use in order to figure out the size of the struct, which has to match what we're telling libgit2 we have.

The version it needs to return in the version that is specified by the libgit2 headers in the `#define`. That's what the compiler is building and that's what it will use in order to figure out the size of the struct, which has to match what we're telling libgit2 we have.
carlosmn commented 2016-11-13 12:35:22 -06:00 (Migrated from github.com)
Copy Link

Note that the test build has flagged one of the calls as having invalid thread locking. We need that in order for the error handling to work.

Note that the test build has flagged one of the calls as having invalid thread locking. We need that in order for the error handling to work.
👍 1
ezwiebel (Migrated from github.com) reviewed 2016-11-14 02:13:44 -06:00
rebase_test.go
ezwiebel (Migrated from github.com) commented 2016-11-14 02:13:44 -06:00
Copy Link

Thanks for that. However I'm not sure how I should do as the cgo feature seems to be forbidden in tests... I will just drop this very low value test I think.

Thanks for that. However I'm not sure how I should do as the cgo feature seems to be forbidden in tests... I will just drop this very low value test I think.
apflieger commented 2017-01-27 16:57:16 -06:00 (Migrated from github.com)
Copy Link

Any update on this ?
BTW, thanks for taking care of it.

Any update on this ? BTW, thanks for taking care of it.
ezwiebel commented 2017-01-29 01:48:33 -06:00 (Migrated from github.com)
Copy Link

Just waiting for validation.

Just waiting for validation.
apflieger commented 2017-03-01 13:36:13 -06:00 (Migrated from github.com)
Copy Link

I'm using this for a while now. Works well. One thing though:
I have a minor suggestion : 79d6f6c18c
Since Repository.InitRebase takes opts *RebaseOptions as argument, I makes more sense that DefaultRebaseOptions returns opts *RebaseOptions instead of opts RebaseOptions
I can't get travis running on this patch since aws outage yesterday...

I'm using this for a while now. Works well. One thing though: I have a minor suggestion : https://github.com/apflieger/git2go/commit/79d6f6c18c61894fd459c6312db419e4776702c7 Since `Repository.InitRebase` takes `opts *RebaseOptions` as argument, I makes more sense that `DefaultRebaseOptions` returns `opts *RebaseOptions` instead of `opts RebaseOptions` I can't get travis running on this patch since aws outage yesterday...
ezwiebel commented 2017-03-05 19:59:36 -06:00 (Migrated from github.com)
Copy Link

Fair enough, I can take care of it.
But I can't decide to accept the PR.

Fair enough, I can take care of it. But I can't decide to accept the PR.
apflieger commented 2017-04-13 14:10:40 -05:00 (Migrated from github.com)
Copy Link

🎉 🎈

🎉 🎈
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
bug

   
duplicate

   
enhancement

   
invalid

   
question

   
up for grabs

   
wontfix
No Label
bug
duplicate
enhancement
invalid
question
up for grabs
wontfix
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
1 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: jcarr/git2go#332
No description provided.
Delete Branch "rebase-wrapper"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?