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

More diff functionality #629

Merged
mbfr merged 24 commits from feature-diff-extra into master 2020-08-18 08:14:03 -05:00
Conversation 0 Commits 24 Files Changed 3 +486 -1
mbfr commented 2020-08-14 05:24:50 -05:00 (Migrated from github.com)
Copy Link

This PR adds

  • The ability to apply a Diff object to the repo
  • Support for git_apply_hunk_cb and git_apply_delta_cb callbacks in options for applying the diffs
  • The ability to import a diff from a raw buffer (for example, one exported by ToBuf) into a Diff object associated with the repo
  • Tests for the above

The test files might be a bit messy but there's quite a lot of stuff to cover.

This PR adds - The ability to apply a Diff object to the repo - Support for git_apply_hunk_cb and git_apply_delta_cb callbacks in options for applying the diffs - The ability to import a diff from a raw buffer (for example, one exported by ToBuf) into a Diff object associated with the repo - Tests for the above The test files might be a bit messy but there's quite a lot of stuff to cover.
lhchavez (Migrated from github.com) reviewed 2020-08-14 19:51:05 -05:00
lhchavez (Migrated from github.com) left a comment
Copy Link

nice test coverage!

nice test coverage!
diff.go
@ -847,3 +848,174 @@ func DiffBlobs(oldBlob *Blob, oldAsPath string, newBlob *Blob, newAsPath string,
lhchavez (Migrated from github.com) commented 2020-08-14 19:16:34 -05:00
Copy Link

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 (Migrated from github.com) commented 2020-08-14 19:24:47 -05:00
Copy Link

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 (Migrated from github.com) commented 2020-08-14 19:25:17 -05:00
Copy Link

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 (Migrated from github.com) commented 2020-08-14 19:33:02 -05:00
Copy Link

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 (Migrated from github.com) commented 2020-08-14 19:34:02 -05:00
Copy Link
		opts.payload = pointerHandles.Track(a)
```suggestion opts.payload = pointerHandles.Track(a) ```
lhchavez (Migrated from github.com) commented 2020-08-14 19:45:26 -05:00
Copy Link

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 (Migrated from github.com) commented 2020-08-14 19:48:13 -05:00
Copy Link

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.
diff.go
@ -850,0 +900,4 @@
if opts.ApplyDeltaCallback == nil {
return 0
}
lhchavez (Migrated from github.com) commented 2020-08-14 19:38:58 -05:00
Copy Link

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 ```
diff.go
@ -850,0 +940,4 @@
opts := &C.git_apply_options{
version: C.GIT_APPLY_OPTIONS_VERSION,
flags: C.uint(a.Flags),
}
lhchavez (Migrated from github.com) commented 2020-08-14 19:28:06 -05:00
Copy Link

since there is no interaction with the thread-local storage in libgit2 (e.g. git_error-related stuff), this is not required here.

since there is no interaction with the thread-local storage in libgit2 (e.g. `git_error`-related stuff), this is not required here.
diff.go
@ -850,0 +999,4 @@
// abbreviated object IDs, so the object IDs in a git_diff_delta produced by
// this function will also be abbreviated.
//
// This function will only read patch files created by a git implementation, it
lhchavez (Migrated from github.com) commented 2020-08-14 19:14:56 -05:00
Copy Link
	runtime.KeepAlive(v)
	runtime.KeepAlive(diff)
	runtime.KeepAlive(cOpts)
```suggestion runtime.KeepAlive(v) runtime.KeepAlive(diff) runtime.KeepAlive(cOpts) ```
diff_test.go
lhchavez (Migrated from github.com) commented 2020-08-14 19:34:47 -05:00
Copy Link
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.
wrapper.c
lhchavez (Migrated from github.com) commented 2020-08-14 19:37:38 -05:00
Copy Link

can this use tabs for consistency?

	options->delta_cb = (git_apply_delta_cb)deltaApplyCallback;
	options->hunk_cb = (git_apply_hunk_cb)hunkApplyCallback;
can this use tabs for consistency? ```suggestion options->delta_cb = (git_apply_delta_cb)deltaApplyCallback; options->hunk_cb = (git_apply_hunk_cb)hunkApplyCallback; ```
mbfr (Migrated from github.com) reviewed 2020-08-17 02:00:01 -05:00
diff.go
@ -847,3 +848,174 @@ func DiffBlobs(oldBlob *Blob, oldAsPath string, newBlob *Blob, newAsPath string,
mbfr (Migrated from github.com) commented 2020-08-17 02:00:00 -05:00
Copy Link

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 (Migrated from github.com) reviewed 2020-08-17 02:05:07 -05:00
diff.go
@ -847,3 +848,174 @@ func DiffBlobs(oldBlob *Blob, oldAsPath string, newBlob *Blob, newAsPath string,
mbfr (Migrated from github.com) commented 2020-08-17 02:05:07 -05:00
Copy Link

Fixed

Fixed
mbfr (Migrated from github.com) reviewed 2020-08-17 02:05:53 -05:00
diff.go
@ -850,0 +999,4 @@
// abbreviated object IDs, so the object IDs in a git_diff_delta produced by
// this function will also be abbreviated.
//
// This function will only read patch files created by a git implementation, it
mbfr (Migrated from github.com) commented 2020-08-17 02:05:52 -05:00
Copy Link

Fixed

Fixed
mbfr (Migrated from github.com) reviewed 2020-08-17 02:07:16 -05:00
diff.go
@ -850,0 +940,4 @@
opts := &C.git_apply_options{
version: C.GIT_APPLY_OPTIONS_VERSION,
flags: C.uint(a.Flags),
}
mbfr (Migrated from github.com) commented 2020-08-17 02:07:15 -05:00
Copy Link

Fixed

Fixed
mbfr (Migrated from github.com) reviewed 2020-08-17 02:07:41 -05:00
wrapper.c
mbfr (Migrated from github.com) commented 2020-08-17 02:07:41 -05:00
Copy Link

Weird, don't know how that happened

Weird, don't know how that happened
mbfr (Migrated from github.com) reviewed 2020-08-17 02:16:48 -05:00
diff.go
@ -847,3 +848,174 @@ func DiffBlobs(oldBlob *Blob, oldAsPath string, newBlob *Blob, newAsPath string,
mbfr (Migrated from github.com) commented 2020-08-17 02:16:48 -05:00
Copy Link

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 (Migrated from github.com) reviewed 2020-08-17 02:18:14 -05:00
diff.go
@ -847,3 +848,174 @@ func DiffBlobs(oldBlob *Blob, oldAsPath string, newBlob *Blob, newAsPath string,
mbfr (Migrated from github.com) commented 2020-08-17 02:18:14 -05:00
Copy Link

Fixed

Fixed
mbfr (Migrated from github.com) reviewed 2020-08-17 02:20:54 -05:00
diff.go
@ -850,0 +900,4 @@
if opts.ApplyDeltaCallback == nil {
return 0
}
mbfr (Migrated from github.com) commented 2020-08-17 02:20:53 -05:00
Copy Link

Fixed

Fixed
mbfr (Migrated from github.com) reviewed 2020-08-17 02:36:53 -05:00
diff.go
@ -847,3 +848,174 @@ func DiffBlobs(oldBlob *Blob, oldAsPath string, newBlob *Blob, newAsPath string,
mbfr (Migrated from github.com) commented 2020-08-17 02:36:53 -05:00
Copy Link

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 (Migrated from github.com) reviewed 2020-08-17 08:12:14 -05:00
diff.go
@ -847,3 +848,174 @@ func DiffBlobs(oldBlob *Blob, oldAsPath string, newBlob *Blob, newAsPath string,
lhchavez (Migrated from github.com) commented 2020-08-17 08:12:14 -05:00
Copy Link

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 (Migrated from github.com) reviewed 2020-08-17 08:25:04 -05:00
diff.go
lhchavez (Migrated from github.com) commented 2020-08-17 08:24:58 -05:00
Copy Link

(from the other thread, in case it gets lost)

```suggestion ``` (from the other thread, in case it gets lost)
diff.go
@ -847,3 +848,174 @@ func DiffBlobs(oldBlob *Blob, oldAsPath string, newBlob *Blob, newAsPath string,
lhchavez (Migrated from github.com) commented 2020-08-17 08:14:16 -05:00
Copy Link
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 (Migrated from github.com) commented 2020-08-17 08:21:11 -05:00
Copy Link
	ApplyHunkCallback  ApplyHunkCallback
	ApplyDeltaCallback ApplyDeltaCallback

(as mentioned above, this is possible)

```suggestion ApplyHunkCallback ApplyHunkCallback ApplyDeltaCallback ApplyDeltaCallback ``` (as mentioned above, this is possible)
diff.go
@ -850,0 +913,4 @@
return 0
} else {
return 1
}
lhchavez (Migrated from github.com) commented 2020-08-17 08:19:50 -05:00
Copy Link

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 (Migrated from github.com) reviewed 2020-08-18 02:11:25 -05:00
diff.go
@ -847,3 +848,174 @@ func DiffBlobs(oldBlob *Blob, oldAsPath string, newBlob *Blob, newAsPath string,
mbfr (Migrated from github.com) commented 2020-08-18 02:11:25 -05:00
Copy Link

Fixed

Fixed
mbfr (Migrated from github.com) reviewed 2020-08-18 02:14:36 -05:00
diff.go
@ -847,3 +848,174 @@ func DiffBlobs(oldBlob *Blob, oldAsPath string, newBlob *Blob, newAsPath string,
mbfr (Migrated from github.com) commented 2020-08-18 02:14:36 -05:00
Copy Link

Removed and hardcoded like the other options

Removed and hardcoded like the other options
mbfr (Migrated from github.com) reviewed 2020-08-18 02:16:35 -05:00
diff.go
@ -850,0 +913,4 @@
return 0
} else {
return 1
}
mbfr (Migrated from github.com) commented 2020-08-18 02:16:34 -05:00
Copy Link

Fixed

Fixed
lhchavez (Migrated from github.com) approved these changes 2020-08-18 08:09:41 -05:00
lhchavez (Migrated from github.com) left a comment
Copy Link

Thanks a lot for this patch!

Thanks a lot for this patch!
Sign in to join this conversation.
Reviewers
No reviewers
lhchavez
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#629
No description provided.
Delete Branch "feature-diff-extra"

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?