-
Notifications
You must be signed in to change notification settings - Fork 136
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
Add a merge strategy #77
Conversation
…h --add-tested, remove the Tested-by trailer in job.push_merged_version()
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.
Looks very good! I left a couple of minor comments, let me know what you think
marge/app.py
Outdated
'(enable this is you use git merge as\n' | ||
'git tends to misbehave when both are used)\n' | ||
), | ||
) |
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.
I'd maybe call the option something like --no-rebase
and modify the help
to say something like "enable if you use a workflow based on merge-commits and not a linear history". What do you think?
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.
I see you later renamed this to --use-merge-strategy
, which I like better.... The rest still applies.
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.
Yes, I prefer your phrasing. I'll change it.
marge/git.py
Outdated
@@ -59,6 +59,38 @@ def tag_with_trailer(self, trailer_name, trailer_values, branch, start_commit): | |||
raise | |||
return self.get_commit_hash() | |||
|
|||
def merge(self, branch, new_base, source_repo_url=None): | |||
"""Merge `new_base` into `branch` and return the new HEAD commit id. |
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.
I suspect new_base
makes more sense as a name for the case of rebase()
since it represents the destination of the re-basing. I'm not sure what would be the right name for the case of merge()
. Maybe main_branch
or something like that?
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.
Maybe source
and target
instead of branch
and new_base
. Or source_branch
and target_branch
.
marge/job.py
Outdated
source_repo_url=None, | ||
reviewers=None, | ||
tested_by=None, | ||
part_of=None, |
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.
Something is off with the indentation?
marge/job.py
Outdated
changes_pushed = True | ||
except git.GitError: | ||
if not branch_merged: | ||
raise CannotMerge('got conflicts while rebasing, your problem now...') |
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.
This shouldn't say "rebasing", right?
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.
I see this was later fixed!
I fixed the two wording points you mentioned. I also updated |
Fixes #72.
Adds a
--use-merge-strategy
option. It will cause marge to usegit merge
instead ofgit rebase
when updating the merge request branch. This option conflicts with--add-tested
.I struggled a bit trying to find correct generic terms englobing both merge and rebase. Depending on the context I used update and fuse. There may be better ones.