Repository: get an instance of MergeFileResult from git_merge_file_from_index #1376
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! |
eantoranz
force-pushed
the
3way-merge-blobs-rename--
branch
2 times, most recently
from
8cdac54 to
a8e75e2
Compare
pygit2/repository.py
Outdated
| C.git_merge_file_result_free(cmergeresult) | ||
|
|
||
| return ret | ||
| if not return_full: | ||
| return content |
There was a problem hiding this comment.
Could you add a deprecation warning?
pygit2/repository.py
Outdated
| if not return_full: | ||
| return content | ||
|
|
||
| return automergeable, content, filemode, path |
There was a problem hiding this comment.
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.
Yep... will do both things. Thanks for the feedback.
eantoranz
force-pushed
the
3way-merge-blobs-rename--
branch
from
a8e75e2 to
676a795
Compare
…om_index 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 a new class named MergeFileResult that maps to libgit2's git_merge_file_result. Add a flag called "use_deprecated" in Repository.merge_file_from_index to control whether the caller wants to use the deprecated functionality receiving an string. In case use_deprecated==False, an instance of MergeFileResult will be used, instead. The default value for use_deprecated is True so not to break existing callers all of the sudden. Later, giving enough time for callers to notice the deprecation warning and adjust their calls to use use_deprecated=False, the default value can be switched to False. And even more time later the flag can disappear completely from the method once all callers have removed the parameter because they are already using the non-deprecated version by removing the parameter completely from their calls. Add a warning when using the deprecated implementation.
eantoranz
force-pushed
the
3way-merge-blobs-rename--
branch
from
676a795 to
f29c606
Compare
|
|
||
| def __str__(self): | ||
| # FIXME Should I include contents over here? | ||
| return f'<automergeable={self.automergeable} path={self.path} mode={self.mode} content={len(self.contents)}>' |
There was a problem hiding this comment.
There are a couple of FIXME/questions about whether the content should be included in __str__() and __repr__(). Let me know so that either I add the content (or perhaps the length) or if they should not be included so that I can adjust and remove the FIXMEs
There was a problem hiding this comment.
I think it might be useful for __str__ to just return contents, to make migration from the deprecated method easier. Otherwise it's maybe not worth implementing it.
If the contents are included in __repr__ then they should be truncated, just displaying the beginning. Otherwise I think it's fine the way it is.
I would try making it a dataclass, it should save the implementations of __init__ and __eq__.
| hello_txt, hello_world, hello_world, use_deprecated=False | ||
| ) | ||
| assert res.automergeable | ||
| assert res.path is None |
There was a problem hiding this comment.
I think this case is not handled correctly from libgit2 but it is the way it behaves right now so I the test reflects current libgit2 current behavior.
eantoranz
changed the title
eantoranz
changed the title
|
|
||
| def __str__(self): | ||
| # FIXME Should I include contents over here? | ||
| return f'<automergeable={self.automergeable} path={self.path} mode={self.mode} content={len(self.contents)}>' |
There was a problem hiding this comment.
I think it might be useful for __str__ to just return contents, to make migration from the deprecated method easier. Otherwise it's maybe not worth implementing it.
If the contents are included in __repr__ then they should be truncated, just displaying the beginning. Otherwise I think it's fine the way it is.
I would try making it a dataclass, it should save the implementations of __init__ and __eq__.
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.