More diff functionality #629

Merged
mbfr merged 24 commits from feature-diff-extra into master 2020-08-18 08:14:03 -05:00
2 changed files with 136 additions and 0 deletions
Showing only changes of commit de24abbdae - Show all commits

74
diff.go
View File

@ -3,6 +3,7 @@ package git
/* /*
#include <git2.h> #include <git2.h>
extern void _go_git_apply_init_options(git_apply_options *options);
extern int _go_git_diff_foreach(git_diff *diff, int eachFile, int eachHunk, int eachLine, void *payload); extern int _go_git_diff_foreach(git_diff *diff, int eachFile, int eachHunk, int eachLine, void *payload);
extern void _go_git_setup_diff_notify_callbacks(git_diff_options* opts); extern void _go_git_setup_diff_notify_callbacks(git_diff_options* opts);
extern int _go_git_diff_blobs(git_blob *old, const char *old_path, git_blob *new, const char *new_path, git_diff_options *opts, int eachFile, int eachHunk, int eachLine, void *payload); extern int _go_git_diff_blobs(git_blob *old, const char *old_path, git_blob *new, const char *new_path, git_diff_options *opts, int eachFile, int eachHunk, int eachLine, void *payload);
@ -847,3 +848,76 @@ func DiffBlobs(oldBlob *Blob, oldAsPath string, newBlob *Blob, newAsPath string,
lhchavez commented 2020-08-14 19:16:34 -05:00 (Migrated from github.com)
Review

the C.CString() needs to be C.free()d. Although it might be better to use C.CBytes() since the buffer should be able to contain NULL bytes.

the `C.CString()` needs to be `C.free()`d. Although it might be better to use [`C.CBytes()`](https://golang.org/cmd/cgo/) since the buffer _should_ be able to contain NULL bytes.
lhchavez commented 2020-08-14 19:24:47 -05:00 (Migrated from github.com)
Review

Is it possible to avoid embedding the callbacks into this struct? otherwise it gives the appearance of there being an 'is-a' relationship between ApplyOptions and ApplyHunkCallback.

Is it possible to avoid [embedding](https://golang.org/doc/effective_go.html#embedding) the callbacks into this struct? otherwise it gives the appearance of there being an 'is-a' relationship between `ApplyOptions` and `ApplyHunkCallback`.
lhchavez commented 2020-08-14 19:25:17 -05:00 (Migrated from github.com)
Review

can the pointerHandles only store the pointer to the ApplyOptions?

	opts, ok := pointerHandles.Get(_payload).(*ApplyOptions)
can the `pointerHandles` only store the pointer to the `ApplyOptions`? ```suggestion opts, ok := pointerHandles.Get(_payload).(*ApplyOptions) ```
lhchavez commented 2020-08-14 19:33:02 -05:00 (Migrated from github.com)
Review

other places (like fc6eaf3638/diff.go (L514)) use the function initialization rather than the initialization constant to avoid having to create a wrapper:

	ecode := C.git_apply_options_init(&opts, C.GIT_APPLY_OPTIONS_VERSION)
other places (like https://github.com/libgit2/git2go/blob/fc6eaf36388841b16ff004e1d48e887d3f9613dc/diff.go#L514) use the function initialization rather than the initialization constant to avoid having to create a wrapper: ```suggestion ecode := C.git_apply_options_init(&opts, C.GIT_APPLY_OPTIONS_VERSION) ```
lhchavez commented 2020-08-14 19:34:02 -05:00 (Migrated from github.com)
Review
		opts.payload = pointerHandles.Track(a)
```suggestion opts.payload = pointerHandles.Track(a) ```
lhchavez commented 2020-08-14 19:45:26 -05:00 (Migrated from github.com)
Review

can the .toC() call be outlined? all other places use the

cOpts = opts.toC()
...
ecode := C.git_...(cOpts)

pattern (except for Oid, since those are just cast to unsafe pointers).

can the `.toC()` call be outlined? all other places use the ```go cOpts = opts.toC() ... ecode := C.git_...(cOpts) ``` pattern (except for `Oid`, since those are just cast to unsafe pointers).
lhchavez commented 2020-08-14 19:48:13 -05:00 (Migrated from github.com)
Review

the version is not needed to be exposed. it's only needed internally by libgit2 to ensure that the size of the struct is what it expects.

```suggestion ``` the version is not needed to be exposed. it's only needed internally by libgit2 to ensure that the size of the struct is what it expects.
mbfr commented 2020-08-17 02:00:00 -05:00 (Migrated from github.com)
Review

I exposed it because it is exposed in some of the other structs (For example, CherrypickOptions, RebaseOptions, etc). Should I just convert this to a non-exported field instead?

I exposed it because it is exposed in some of the other structs (For example, CherrypickOptions, RebaseOptions, etc). Should I just convert this to a non-exported field instead?
mbfr commented 2020-08-17 02:05:07 -05:00 (Migrated from github.com)
Review

Fixed

Fixed
mbfr commented 2020-08-17 02:16:48 -05:00 (Migrated from github.com)
Review

I didn't realise that, I suppose that's why it passes the size as a separate parameter (so it can do a binary diff?) I'm not sure about the CString, I wasn't able to find out how Go behaves if theres a null byte in the middle of a string so I converted it to a CBytes as recommended

I didn't realise that, I suppose that's why it passes the size as a separate parameter (so it can do a binary diff?) I'm not sure about the CString, I wasn't able to find out how Go behaves if theres a null byte in the middle of a string so I converted it to a CBytes as recommended
mbfr commented 2020-08-17 02:18:14 -05:00 (Migrated from github.com)
Review

Fixed

Fixed
mbfr commented 2020-08-17 02:36:53 -05:00 (Migrated from github.com)
Review

I thought there was a reason I did it this way but after changing it to call this it works fine 🤔

I thought there was a reason I did it this way but after changing it to call this it works fine :thinking:
lhchavez commented 2020-08-17 08:12:14 -05:00 (Migrated from github.com)
Review

I'd rather remove it altogether since that version number is just there to account for version drift in C, where there is no way to differentiate between different versions of the same struct. As long as there is a layer that accounts for this (e.g. git2go), it's an implementation detail that's best to hide from the end user.

RevertOptions was recently de-Versionified, so we should probably make the other Options follow suit: 4bca045e5a (diff-30188d21ac9afa73021c8dd3ae818448) I'll probably do that at v31 branch creation time to avoid breaking the interface for older folks.

I'd rather remove it altogether since that version number is just there to account for version drift in C, where there is no way to differentiate between different versions of the same struct. As long as there is a layer that accounts for this (e.g. git2go), it's an implementation detail that's best to hide from the end user. RevertOptions was recently de-Versionified, so we should probably make the other Options follow suit: https://github.com/libgit2/git2go/commit/4bca045e5aa98b0b791fb467705de0692fe3514f#diff-30188d21ac9afa73021c8dd3ae818448 I'll probably do that at v31 branch creation time to avoid breaking the interface for older folks.
lhchavez commented 2020-08-17 08:14:16 -05:00 (Migrated from github.com)
Review
type ApplyHunkCallback func(*DiffHunk) (apply bool, err error)
type ApplyDeltaCallback func(*DiffDelta) (apply bool, err error)

the older name was better for consistency:

~/git2go$ git grep 'type.*Cb func' | wc -l
0
~/git2go$ git grep 'type.*Callback func' | wc -l
24

in L860-L861, the same name can be used for the field and the type, which is the way to express that something is not to be embedded.

```suggestion type ApplyHunkCallback func(*DiffHunk) (apply bool, err error) type ApplyDeltaCallback func(*DiffDelta) (apply bool, err error) ``` the older name was better for consistency: ```shell ~/git2go$ git grep 'type.*Cb func' | wc -l 0 ~/git2go$ git grep 'type.*Callback func' | wc -l 24 ``` in L860-L861, the same name can be used for the field and the type, which is the way to express that something is not to be embedded.
lhchavez commented 2020-08-17 08:21:11 -05:00 (Migrated from github.com)
Review
	ApplyHunkCallback  ApplyHunkCallback
	ApplyDeltaCallback ApplyDeltaCallback

(as mentioned above, this is possible)

```suggestion ApplyHunkCallback ApplyHunkCallback ApplyDeltaCallback ApplyDeltaCallback ``` (as mentioned above, this is possible)
mbfr commented 2020-08-18 02:11:25 -05:00 (Migrated from github.com)
Review

Fixed

Fixed
mbfr commented 2020-08-18 02:14:36 -05:00 (Migrated from github.com)
Review

Removed and hardcoded like the other options

Removed and hardcoded like the other options
return nil return nil
} }
type ApplyOptions struct {
Version uint
Flags uint
// TODO: there are some more flags, not currently used
}
func DefaultApplyOptions() (*ApplyOptions, error) {
opts := C.git_apply_options{}
runtime.LockOSThread()
defer runtime.UnlockOSThread()
C._go_git_apply_init_options(&opts)
return applyOptionsFromC(&opts), nil
}
func (a *ApplyOptions) toC() *C.git_apply_options {
if a == nil {
return nil
}
opts := &C.git_apply_options{
version: C.uint(a.Version),
flags: C.uint(a.Flags),
}
return opts
}
func applyOptionsFromC(opts *C.git_apply_options) *ApplyOptions {
return &ApplyOptions{
Version: uint(opts.version),
Flags: uint(opts.flags),
}
}
type GitApplyLocation int
const (
GitApplyLocationWorkdir GitApplyLocation = C.GIT_APPLY_LOCATION_WORKDIR
GitApplyLocationIndex GitApplyLocation = C.GIT_APPLY_LOCATION_INDEX
GitApplyLocationBoth GitApplyLocation = C.GIT_APPLY_LOCATION_BOTH
)
func (v *Repository) ApplyDiff(diff *Diff, location GitApplyLocation, opts *ApplyOptions) error {
runtime.LockOSThread()
defer runtime.UnlockOSThread()
ecode := C.git_apply(v.ptr, diff.ptr, C.git_apply_location_t(location), opts.toC())
runtime.KeepAlive(v)
if ecode < 0 {
lhchavez commented 2020-08-14 19:38:58 -05:00 (Migrated from github.com)
Review

can this also use the same pattern as the other change?

		if gitError, ok := err.(*GitError); ok {
			return C.int(gitError.Code)
		}
		return -1
can this also use the same pattern as the other change? ```go if gitError, ok := err.(*GitError); ok { return C.int(gitError.Code) } return -1 ```
mbfr commented 2020-08-17 02:20:53 -05:00 (Migrated from github.com)
Review

Fixed

Fixed
return MakeGitError(ecode)
}
return nil
}
func DiffFromBuffer(buffer []byte, repo *Repository) (*Diff, error) {
var diff *C.git_diff
runtime.LockOSThread()
defer runtime.UnlockOSThread()
ecode := C.git_diff_from_buffer(&diff, C.CString(string(buffer)), C.size_t(len(buffer)))
lhchavez commented 2020-08-17 08:19:50 -05:00 (Migrated from github.com)
Review

Can these be returned to their old place? The reason why they are needed is that when runtime.LockOsThread()/runtime.UnlockOSThread() combo is called, it asks Go to guarantee that only this code executes on that thread (since Go is completely free to move stuff around when needed, at any point in time). That's the only way in which it can be guaranteed that if git_apply_options_init happens to place any error information in the Thread-local storage, MakeGitError() below can still find it.

We may need to tweak script/check-MakeGitError-thread-lock.go to also complain if these functions don't happen before any cgo calls in the function to make this less error-prone.

Can these be returned to their old place? The reason why they are needed is that when [`runtime.LockOsThread()`](https://golang.org/pkg/runtime/#LockOSThread)/`runtime.UnlockOSThread()` combo is called, it asks Go to guarantee that _only_ this code executes on that thread (since Go is completely free to move stuff around when needed, at any point in time). That's the only way in which it can be guaranteed that if `git_apply_options_init` happens to place any error information in the Thread-local storage, `MakeGitError()` below can still find it. We may need to tweak `script/check-MakeGitError-thread-lock.go` to _also_ complain if these functions don't happen before any cgo calls in the function to make this less error-prone.
mbfr commented 2020-08-18 02:16:34 -05:00 (Migrated from github.com)
Review

Fixed

Fixed
if ecode < 0 {
return nil, MakeGitError(ecode)
}
runtime.KeepAlive(diff)
return newDiffFromC(diff, repo), nil
}

View File

@ -236,3 +236,65 @@ func TestDiffBlobs(t *testing.T) {
t.Fatalf("Bad number of lines iterated") t.Fatalf("Bad number of lines iterated")
} }
} }
lhchavez commented 2020-08-14 19:34:47 -05:00 (Migrated from github.com)
Review
func TestApplyDiffAddfile(t *testing.T) {

so far, no tests have underscores in their names.

```suggestion func TestApplyDiffAddfile(t *testing.T) { ``` so far, no tests have underscores in their names.
func Test_ApplyDiff_Addfile(t *testing.T) {
repo := createTestRepo(t)
defer cleanupTestRepo(t, repo)
seedTestRepo(t, repo)
addFirstFileCommit, addFileTree := addAndGetTree(t, repo, "file1", `hello`)
addSecondFileCommit, addSecondFileTree := addAndGetTree(t, repo, "file2", `hello2`)
diff, err := repo.DiffTreeToTree(addFileTree, addSecondFileTree, nil)
checkFatal(t, err)
t.Run("check does not apply to current tree because file exists", func(t *testing.T) {
err = repo.ResetToCommit(addSecondFileCommit, ResetHard, &CheckoutOpts{})
checkFatal(t, err)
err = repo.ApplyDiff(diff, GitApplyLocationBoth, nil)
if err == nil {
t.Error("expecting applying patch to current repo to fail")
}
})
t.Run("check apply to correct commit", func(t *testing.T) {
err = repo.ResetToCommit(addFirstFileCommit, ResetHard, &CheckoutOpts{})
checkFatal(t, err)
err = repo.ApplyDiff(diff, GitApplyLocationBoth, nil)
checkFatal(t, err)
})
t.Run("check convert to raw buffer and apply", func(t *testing.T) {
err = repo.ResetToCommit(addFirstFileCommit, ResetHard, &CheckoutOpts{})
checkFatal(t, err)
raw, err := diff.ToBuf(DiffFormatPatch)
checkFatal(t, err)
if len(raw) == 0 {
t.Error("empty diff created")
}
diff2, err := DiffFromBuffer(raw, repo)
checkFatal(t, err)
err = repo.ApplyDiff(diff2, GitApplyLocationBoth, nil)
checkFatal(t, err)
})
}
func addAndGetTree(t *testing.T, repo *Repository, filename string, content string) (*Commit, *Tree) {
commitId, err := commitSomething(repo, filename, content)
checkFatal(t, err)
commit, err := repo.LookupCommit(commitId)
checkFatal(t, err)
tree, err := commit.Tree()
checkFatal(t, err)
return commit, tree
}