Skip to content

Conversation

@ethomson
Copy link
Member

@ethomson ethomson commented May 8, 2014

When the provided FastForwardStrategy is FastForwardStrategy.Default, honor the merge.ff configuration setting. When merge.ff=only, enable fast-forward only mode; when merge.ff=false, enable no-ff mode.

@ethomson
Copy link
Member Author

ethomson commented May 8, 2014

This keeps libgit2sharp's "analysis" function to truly only perform the analysis of the mergability, such that it will always report "fastforwardable" when the merge is, indeed, fastforwardable, without contemplating policy decisions by the user.

Since LibGit2Sharp introduces the idea of a "default action", it is the layer that queries this configuration setting and applies it to the default.

/cc @nulltoken
/cc @jamill

@jamill
Copy link
Member

jamill commented May 8, 2014

libgit2 automatically handles the config options in other places (e.g. the handling of tagopts for fetching is one example). It seems inconsistent to have the library automatically take the user configurations into account in some places, but not in others. I was expecting lg2 to handle this, so that not every binding would have to implement the same logic.

@ethomson
Copy link
Member Author

ethomson commented May 8, 2014

Yes, I thought that this would be a contentious issue. This was also my first thought, but I can't think of a sensible place to put this.

Do you think that git_merge_analysis should take the merge.ff setting into account? I don't, but I agree that it's cumbersome to make everybody deal with this setting themselves. But if not there, where?

@jamill
Copy link
Member

jamill commented May 8, 2014

What if there was another function (e.g. git_merge_preferred_merge_type for lack of a better name now), that calls into git_merge_analysis and takes into account the merge.ff setting?

@ethomson
Copy link
Member Author

ethomson commented May 8, 2014

That seems reasonable to me.

@ethomson
Copy link
Member Author

ethomson commented May 8, 2014

I wonder what @carlosmn thinks about this, when he has a moment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is missing "0" as a falsy value. I'd recommend using Config.Get<bool>("merge.ff") for this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ethomson Could you please consider @carlosmn's comment?

@carlosmn
Copy link
Member

carlosmn commented May 8, 2014

I think this could fit as part of git_merge_analysis(). Since we're pulling data for what git-merge understands and may do, and git_merge_analysis_t are ORable flags, we could set a bit or two to indicate the user's preferences via this setting.

Making use of this information is still something that should be up to the tool using libgit2.

@Therzok
Copy link
Member

Therzok commented May 29, 2014

Shouldn't this be handled by the new API exposed in #722.

@ethomson
Copy link
Member Author

Yes. I will update once your PR is merged.

@Therzok
Copy link
Member

Therzok commented May 29, 2014

Perfecto!

@Therzok
Copy link
Member

Therzok commented Jun 3, 2014

Just a minor update: In case you forgot, #722 is merged.

@ethomson
Copy link
Member Author

Updated!

/cc @Therzok
/cc @nulltoken

@ethomson
Copy link
Member Author

Oops; I rebased the wrong thing onto the wrong thing, let's try this again!

@nulltoken
Copy link
Member

@ethomson Could you please update FastForwardStrategy.Default xml doc in order to clearly state that it will honor the merge.ff config value?

When the provided FastForwardStrategy is FastForwardStrategy.Default,
honor the merge.ff configuration setting.  When merge.ff=only, enable
fast-forward only mode; when merge.ff=false, enable no-ff mode.
@ethomson
Copy link
Member Author

@ethomson Could you please update FastForwardStrategy.Default xml doc in order to clearly state that it will honor the merge.ff config value?

@nulltoken Done!

@nulltoken nulltoken merged commit 6bfe4ab into libgit2:vNext Jun 18, 2014
@nulltoken
Copy link
Member

Very very neat PR! 💖

@nulltoken nulltoken added this to the v0.19.0 milestone Jun 18, 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.

5 participants