Skip to content

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

Merged
merged 1 commit into from
May 3, 2014
Merged

Conversation

jamill
Copy link
Member

@jamill jamill commented Apr 23, 2014

This change:

  1. Brings CheckoutOptions to the Checkout methods
  2. Refactors some of the logic creating the native git_checkout_options struct, to make it easier to use CheckoutOptions across different operations.
  3. Brings CheckoutOptions To Merge.
  4. Exposes FileConflictStrategy in CheckoutOptions

The FileConflictStrategy parameters currently only applies to the checkout during a merge operation. Longer term this property should be generally applicable to any Checkout operation, but it currently is only applicable to Merge operations. Given where I think this parameter will be used longer term, I decided to include it in the CheckoutOptions class, and then expose the CheckoutOptions parameter through MergeOptions. The other option would be to (for the short-term) just include this parameter in the MergeOptions class.

The other benefit is that the Merge operations would now pick up all the options exposed by the CheckoutOptions. However, only FileConflictStrategy is exercised through the merge tests. If people approve of the general direction, I can add some more tests.

@jamill
Copy link
Member Author

jamill commented Apr 23, 2014

I am going to approach this problem slightly differently. I will update the PR when it is ready

@jamill
Copy link
Member Author

jamill commented Apr 23, 2014

So, I don't think each of the individual operations should take a CheckoutOption parameter (for instance). Each of these options structures contain some options that might not make sense. For instance, CheckoutOptions contains a CheckoutModifiers property. Going further, MergeOptions contains options that only apply to the merge operation and not (for instance) revert. Internally, each of these *Options objects will expose the necessary properties to generate the corresponding libgit2 structures.

@jamill jamill changed the title WIP: Teach MergeOptions about CheckoutOptions Teach MergeOptions about CheckoutOptions Apr 29, 2014
@nulltoken
Copy link
Member

@jamill That looks very cool.

I wonder if that would make sense to move the Signatures as properties of CheckoutOptions and MergeOptions?

{
/// <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
Copy link
Member

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 ?

@jamill
Copy link
Member Author

jamill commented Apr 29, 2014

I wonder if that would make sense to move the Signature s as properties of CheckoutOptions and MergeOptions ?

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 *Options classes.

@jamill
Copy link
Member Author

jamill commented May 1, 2014

@nulltoken so, the two open questions:

  1. Signature as a parameter to the method, or contained in the Option struct.
  2. Do we find renames by default.

For 1, I am leaning to keeping this on the actual method. This way, operations like Pull can just take the Signature input and apply it for both the Fetch and Merge sub operations. Also, I thought there was talk of making this mandatory, which wouldn't fit with the optional parameter.

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.

@ethomson
Copy link
Member

ethomson commented May 2, 2014

  1. I think this makes sense.
  2. Yes, I think this is expected by consumers. You definitely want rename detection if you can get it.

@nulltoken What else needs to be done here to merge this?

@nulltoken
Copy link
Member

@jamill @ethomson Thanks a lot for this! Let's merge it as is. Could you please rebase/squash?

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
@jamill
Copy link
Member Author

jamill commented May 3, 2014

@nulltoken Thanks! Squashed and rebased!

@nulltoken nulltoken merged commit 9015578 into vNext May 3, 2014
@nulltoken nulltoken deleted the jamill/checkout_options branch May 3, 2014 10:40
@nulltoken
Copy link
Member

💥

@nulltoken nulltoken added this to the v0.18.0 milestone May 5, 2014
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