Skip to content

Wrap git_merge_file #630

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

Closed
wants to merge 3 commits into from
Closed

Conversation

alehed
Copy link
Contributor

@alehed alehed commented May 28, 2017

Add a wrapper for git_merge_file

@alehed
Copy link
Contributor Author

alehed commented May 28, 2017

Just some changes I had lying around.

Copy link
Member

@pietbrauer pietbrauer left a comment

Choose a reason for hiding this comment

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

Same here: Thanks a lot for the PullRequest! As there is a lot going on some tests to verify the functionality would be great.

/// error - The error if one occurred. Can be NULL.
///
/// Returns The file content annotated with conflict markers or null on error
- (NSString* _Nullable)stringForConflictWithAncestor:(GTIndexEntry *)ancestor ourSide:(GTIndexEntry *)ourSide theirSide:(GTIndexEntry *)theirSide withError:(NSError **)error;
Copy link
Contributor

Choose a reason for hiding this comment

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

Small nitpicks :

  • * should be right-aligned
  • withError: => error:

Copy link
Contributor

Choose a reason for hiding this comment

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

There's a few more * spacing weirdness in the hunk below as well.



git_merge_file_result result;
git_merge_file(&result, &ancestorInput, &ourInput, &theirInput, nil);
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing error checking.

@tiennou
Copy link
Contributor

tiennou commented May 30, 2017

Thanks for the PR ! A few more thoughts though :

  • I'd say using NSString will prevents that from working with binary files. Would it be acceptable to switch to using NSData inside the method, just in case ?
  • I find the -stringForConflictWithAncestor:… name unclear. May I suggest something like -contentsOfDiffWithAncestor:… or something along those lines ?

@alehed
Copy link
Contributor Author

alehed commented May 30, 2017

@tiennou thanks for the feedback! I'll fix it this weekend.

@alehed
Copy link
Contributor Author

alehed commented Jun 4, 2017

Addressed the main issues. The test is pretty messy, but I don't see a nicer or shorter way to produce conflicting commits on two different branches and merging them.

@alehed
Copy link
Contributor Author

alehed commented Jun 6, 2017

This should be ready.

strncpy(cString, result.ptr, result.len);
cString[result.len] = '\0';

NSString *mergedContent = [[NSString alloc] initWithCString:cString encoding:NSUTF8StringEncoding];
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe -initWithBytesNoCopy:length:encoding:freeWhenDone: ? That should allow you to just pass result.ptr/result.len, skipping the need for the malloc/strncpy step. You would have to result.ptr = NULL though, so git_merge_file_result_free doesn't break it by freeing before returning.

@tiennou
Copy link
Contributor

tiennou commented Jun 6, 2017

After a more thorough look at what libgit2 exposes, I have a feeling that it might be a too brittle wrapper around the git_merge_file_result structure. But I'll defer to @pietbrauer on that though.

@alehed
Copy link
Contributor Author

alehed commented Jun 7, 2017

I could wrap git_merge_file_result in a class if you mean that. But I don't know where else it would be used.

@pietbrauer
Copy link
Member

I'm ok with not having a wrapper for now.

@pietbrauer pietbrauer dismissed their stale review June 28, 2017 20:21

@tiennou has the last word on this one.

@tiennou
Copy link
Contributor

tiennou commented Aug 17, 2018

Closing in favor of #664. Thanks for the PR @alehed, and sorry for the long turnaround!

@tiennou tiennou closed this Aug 17, 2018
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.

3 participants