-
-
Notifications
You must be signed in to change notification settings - Fork 400
[RFC] Repository: get all values from git_merge_file_from_index #1376
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
base: master
Are you sure you want to change the base?
Conversation
I know right from the get go that this is not very elegant but I wanted to start the conversation somewhere. I am working on a simplified rebasing tool. I am dealing with 3way-merges of blobs and I did not find that I noticed that I could use So I added the additional flag to be able to get all the values without breaking the implementation of existing users of the method. Let me know what you think, if you don't mind. Thanks! |
a60a7bf
to
8cdac54
Compare
In Repository.merge_file_from_index, pygit2 is only returning the resulting content out of the merge, however other values might be relevant like the filemode. Add an optional flag to make it possible to get all the values coming out of git_merge_file_from_index instead of just the file content.
8cdac54
to
a8e75e2
Compare
C.git_merge_file_result_free(cmergeresult) | ||
|
||
return ret | ||
if not return_full: | ||
return content |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a deprecation warning?
if not return_full: | ||
return content | ||
|
||
return automergeable, content, filemode, path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you return a smarter object, like a dataclass? With the name MergeFileResult
, and using "contents" instead of "content" (as is written in the libgit2 reference documentation).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep... will do both things. Thanks for the feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unit tests too please. Thanks!
In Repository.merge_file_from_index, pygit2 is only returning the resulting content out of the merge, however other values might be relevant like the filemode.
Add an optional flag to make it possible to get all the values coming out of git_merge_file_from_index instead of just the file content.