Skip to content
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

Use the ours strategy OPTION rather than the ours strategy #37

Merged
merged 1 commit into from
Sep 24, 2019

Conversation

HebaruSan
Copy link
Member

Problem

The new Python bot sometimes discards changes in its merge commits. Currently this is preventing the download counts from working (see #36); web hooks are also broken (see #33), maybe for the same reason, maybe with some additional complexity mixed in (will find out when we see whether this fixes it).

Cause

Git has some very confusing syntax relating to merge strategies. The user can override the merge strategy, and/or provide options for a merge strategy, and the ours keyword is allowed in both places!!!

We want this one (and used it historically in the Perl bot):

https://git-scm.com/docs/git-pull

ours
This option forces conflicting hunks to be auto-resolved cleanly by favoring our version. Changes from the other tree that do not conflict with our side are reflected to the merge result. For a binary file, the entire contents are taken from our side.

This should not be confused with the ours merge strategy, which does not even look at what the other tree contains at all. It discards everything the other tree did, declaring our history contains all that happened in it.

However, the Python bot currently uses this one:

ours
This resolves any number of heads, but the resulting tree of the merge is always that of the current branch head, effectively ignoring all changes from all other branches. It is meant to be used to supersede old development history of side branches. Note that this is different from the -Xours option to the recursive merge strategy.

This causes every pull that happens in the system to discard any changes that were added by another container!

Changes

Now the Python bot uses the ours strategy option instead of the strategy, as intended. This will make it stop discarding changes.

To accomplish this, we need to switch from the strategy parameter to the strategy-option parameter.

-s <strategy>
--strategy=<strategy>
Use the given merge strategy; can be supplied more than once to specify them in the order they should be tried. If there is no -s option, a built-in list of strategies is used instead (git merge-recursive when merging a single head, git merge-octopus otherwise).

-X <option>
--strategy-option=<option>
Pass merge strategy specific option through to the merge strategy.

In gitpython this is accomplished with an underscore in place of the dash:

Fixes #36. May fix #33, not sure about that one.

@HebaruSan HebaruSan added the Bug Something isn't working label Sep 24, 2019
@HebaruSan HebaruSan requested a review from techman83 September 24, 2019 05:59
@techman83
Copy link
Member

My gosh, what an absolute gotcha that is!! Really good investigation work @HebaruSan - that is definitely what is going on here.

@techman83 techman83 merged commit adf13e5 into KSP-CKAN:master Sep 24, 2019
@HebaruSan HebaruSan deleted the fix/strategy-ours branch September 24, 2019 06:51
@HebaruSan
Copy link
Member Author

My gosh, what an absolute gotcha that is!!

Absolutely! Definitely not anything to feel bad about. You can tell that someone pointed out how confusing it would be when the git team was doing that development, and the response was, "Just point out the difference in the man page, it'll be fine."

@techman83
Copy link
Member

I think I knew this, but the use of the shorthand parameter didn't make it immediately obvious that was my intent originally.

Fixed now, not a bad set of bugs considering it was a ground up re-write in a different language!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
2 participants