-
Notifications
You must be signed in to change notification settings - Fork 899
Teach MergeOptions about CheckoutOptions #685
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
Conversation
I am going to approach this problem slightly differently. I will update the PR when it is ready |
So, I don't think each of the individual operations should take a |
@jamill That looks very cool. I wonder if that would make sense to move the |
{ | ||
/// <summary> | ||
/// When a region of a file is changed in both branches, a conflict | ||
/// will be recorded in the index so that `git_checkout` can produce |
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.
git_checkout -> the checkout operation ?
That is an option. I thought that there was discussion of making these mandatory parameters at some point. If we want them to be mandatory, then they should stay as independent parameters. Otherwise, I will fold them into the |
@nulltoken so, the two open questions:
For 1, I am leaning to keeping this on the actual method. This way, operations like Pull can just take the For 2 - I also am thinking to have this enabled by default, as I think this is what is expected by default. I can update the documentation for merge to reflect the default for 2. |
@nulltoken What else needs to be done here to merge this? |
Checkout methods now use CheckoutOptions Merge now takes several options: - Option to specify what is checked out for file conflicts. - Report CheckoutProgress and CheckoutNotify - Option to specify MergeFileFavor Updates for code review feedback
@nulltoken Thanks! Squashed and rebased! |
💥 |
This change:
CheckoutOptions
to the Checkout methodsgit_checkout_options
struct, to make it easier to useCheckoutOption
s across different operations.CheckoutOptions
To Merge.FileConflictStrategy
inCheckoutOptions
The
FileConflictStrategy
parameters currently only applies to the checkout during a merge operation. Longer term this property should be generally applicable to anyCheckout
operation, but it currently is only applicable toMerge
operations. Given where I think this parameter will be used longer term, I decided to include it in theCheckoutOption
s class, and then expose theCheckoutOption
s parameter throughMergeOption
s. The other option would be to (for the short-term) just include this parameter in theMergeOption
s class.The other benefit is that the
Merge
operations would now pick up all the options exposed by theCheckoutOption
s. However, onlyFileConflictStrategy
is exercised through the merge tests. If people approve of the general direction, I can add some more tests.