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

Reflog support #730

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Reflog support #730

wants to merge 1 commit into from

Conversation

pokstad
Copy link

@pokstad pokstad commented Jan 18, 2021

Adds support and tests for all reflog functions listed on https://libgit2.org/libgit2/#HEAD/group/reflog.

Closes #467

@pokstad pokstad changed the title Reflog support (#467) Reflog support Jan 18, 2021
Copy link
Contributor

@lhchavez lhchavez left a comment

Choose a reason for hiding this comment

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

sorry for the delay! for some reason i saw this, accidentally marked as read and completely forgot about it until today u_u

return l
}

func (repo *Repository) ReadReflog(name string) (*Reflog, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

in order for these functions to be more easy to find in the docs, what do you think about modelling this similar to the other *Collection structs in https://godoc.org/github.com/libgit2/git2go#Repository? (i.e. there would be an Reflogs ReflogCollection). That way folks can do repo.Reflogs.Read("name") and repo.Reflogs.Delete("name").

also, i know we have not been super diligent about this, but could all public methods being introduced have a docstring? the godocs need a bit of love ^^;;

}
}

func (l *Reflog) EntryByIndex(index uint) *ReflogEntry {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
func (l *Reflog) EntryByIndex(index uint) *ReflogEntry {
func (l *Reflog) EntryByIndex(index uint64) *ReflogEntry {

would it be possible to make this the widest size for any architecture that we support today? supporting more than 2^32 entries might be silly, but i'd rather be safe than sorry.

same in all the other places that use size_t in the C api.

runtime.LockOSThread()
defer runtime.UnlockOSThread()

var rewriteHistoryInt int
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
var rewriteHistoryInt int
var rewriteHistoryInt C.int

might as well define it as C.int from the get-go to avoid the cast in L144.

cMsg := C.CString(message)
defer C.free(unsafe.Pointer(cMsg))

C.git_reflog_append(l.ptr, oid.toC(), cSignature, cMsg)
Copy link
Contributor

Choose a reason for hiding this comment

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

can the error be handled?

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.

Reflog support
2 participants