Add possibility to merge when head is unborn (empty repository) #633
Conversation
| CheckoutModifiers = CheckoutModifiers.None, | ||
| }; | ||
|
|
||
| CheckoutTree(commit.Tree, null, checkoutOpts); |
There was a problem hiding this comment.
I don't know if I must do a CheckoutTree() before with the CheckoutModifiers.None or after but which need a CheckoutModifiers.Force. What do you think about that?
There was a problem hiding this comment.
The Head could be unborn for a different reason: when creating a new root in the repository.
Beside this, the index and the workdir may also be populated.
I'd go with the None option to err on the safe side, but I'm not the MergeMaster™.
There was a problem hiding this comment.
I read libgit2/libgit2#2135, but I am not convinced that lg2 shouldn't handle this case. Instead of all of the bindings special casing and handling the unborn head case separately (and e.g. handling the fast-forward, non-fast forward allowed merge types), it would seem cleaner if lg2 could handle these scenarios and then the bindings can just act of the return from the git_merge_from_* functions.
There was a problem hiding this comment.
Which checkout options to use depends on which version of git you want to pretend you called. Older versions do a forced checkout, but that's destructive in edge cases. Newer versions will refuse to update if there are local changes to files that exist in the target commit (which are all files, by definition), which is roughly what they do on a fast-forward merge; though the error message is slightly different, suggesting that the code paths aren't quite the same.
There was a problem hiding this comment.
@pmiossec So, I think this is a really good start! However, git tend to make quite difficult to overwrite local changes, and so do we.
As seen before, creating an unborn branch is quite easy (just by making the HEAD point to a non existing branch). This state doesn't mean that it's safe to blindly merge.
Considering these, I think that in this scenario, the code should:
- Ensure the Index is empty
- Ensure a Diff between the workdir and the commit tree doesn't report any updated entries.
Performing those checks should ensure that the user won't loose any data, while still allowing your scenario.
There was a problem hiding this comment.
@nulltoken what do you think about this last change?
|
@pmiossec - how eager are you for this change? If we were able to update the native bindings to handle this scenario as discussed in libgit2/libgit2#2135 fairly soon, would that work for you (in the next week or so)? |
|
I could wait. Their is really no hurry! I just saw that the merge support |
|
@jamill I will test this PR, will see if it solve my cases and will tell you... |
|
@jamill Ok. I have tested and it works well and solve my problems.... I close! |
|
Thanks for trying this out! |
|
Fixed by #643 |
See libgit2/libgit2#2135 for more details
cc @nulltoken