-
Notifications
You must be signed in to change notification settings - Fork 129
Add functionality to detect build 'conflicts' while merging #64
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
base: master
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -553,6 +553,14 @@ class AutomaticMergeFailed(Exception): | |
| self.commit1, self.commit2 = commit1, commit2 | ||
|
|
||
|
|
||
| class AutomaticBuildFailed(Exception): | ||
| def __init__(self, commit1, commit2): | ||
| Exception.__init__( | ||
| self, 'Automatic build of %s and %s failed' % (commit1, commit2,) | ||
| ) | ||
| self.commit1, self.commit2 = commit1, commit2 | ||
|
|
||
|
|
||
| def automerge(commit1, commit2, msg=None): | ||
| """Attempt an automatic merge of commit1 and commit2. | ||
|
|
||
|
|
@@ -572,8 +580,18 @@ def automerge(commit1, commit2, msg=None): | |
| # added in git version 1.7.4. | ||
| call_silently(['git', 'reset', '--merge']) | ||
| raise AutomaticMergeFailed(commit1, commit2) | ||
| else: | ||
| return get_commit_sha1('HEAD') | ||
|
|
||
| build_script = MergeState.get_default_test_command() | ||
| if build_script: | ||
| try: | ||
| check_call(['/bin/sh', build_script]) | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The way you are invoking the test is unnecessarily restrictive. It requires the test to be embodied in a single shell script, which in many cases the user would have to write just for this purpose. If instead you would invoke it using then or even Please note that By the way, deviation from
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
| except CalledProcessError as e: | ||
| if e.returncode == 125: | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting. So "the source at this commit cannot be built or cannot be tested" is treated as success as far as We could also handle this return code specially during bisection. It could be treated as "something is fishy. Don't treat it as a failure, as long as the status of other tests are definitive". So if an earlier merge is found to FAIL definitively, then we would continue to the left of that merge. And if a later merge is found to SUCCEED definitively, then we would continue to the right of that merge. And if an indeterminate merge is found at the boundary between SUCCEED and FAIL, then we would treat the indeterminate status the same as a FAIL. But without some kind of special handling, I don't see the value of treating return code 125 specially. The test script could just as well return 0 because it would have the same end result.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah it's treated as a success... I guess it was a placeholder for doing the right thing. I agree with the middle paragraph. I'll see if acting on an indeterminate state during the bisect stage is possible.
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is definitely something that doesn't have to be implemented in the first version.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed for another version |
||
| return get_commit_sha1('HEAD') | ||
| else: | ||
| raise AutomaticBuildFailed(commit1, commit2) | ||
|
|
||
| return get_commit_sha1('HEAD') | ||
|
|
||
|
|
||
| class MergeRecord(object): | ||
|
|
@@ -1474,11 +1492,15 @@ class Block(object): | |
| 'Attempting automerge of %d-%d...' % self.get_original_indexes(i1, i2) | ||
| ) | ||
| try: | ||
| print("Automerging from is_mergeable") | ||
| automerge(self[i1, 0].sha1, self[0, i2].sha1) | ||
| sys.stderr.write('success.\n') | ||
| return True | ||
| except AutomaticMergeFailed: | ||
| sys.stderr.write('failure.\n') | ||
| sys.stderr.write('Merge failure.\n') | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The old message was lower case because it was a continuation of the line started above, like: I think lower-case looks better here because it is punctuated like a single sentence. The same comment applies a few lines down, too.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah I see
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
| return False | ||
| except AutomaticBuildFailed: | ||
| sys.stderr.write('Build failure.\n') | ||
| return False | ||
|
|
||
| def auto_outline(self): | ||
|
|
@@ -1497,6 +1519,7 @@ class Block(object): | |
| sys.stderr.write(msg % (i1orig, i2orig)) | ||
| logmsg = 'imerge \'%s\': automatic merge %d-%d' % (self.name, i1orig, i2orig) | ||
| try: | ||
| print("Automerging from auto_outline") | ||
| merge = automerge(commit1, commit2, msg=logmsg) | ||
| sys.stderr.write('success.\n') | ||
| except AutomaticMergeFailed as e: | ||
|
|
@@ -1571,6 +1594,7 @@ class Block(object): | |
| sys.stderr.write('Attempting to merge %d-%d...' % (i1orig, i2orig)) | ||
| logmsg = 'imerge \'%s\': automatic merge %d-%d' % (self.name, i1orig, i2orig) | ||
| try: | ||
| print("Automerging from auto_fill_micromerge") | ||
| merge = automerge( | ||
| self[i1, i2 - 1].sha1, | ||
| self[i1 - 1, i2].sha1, | ||
|
|
@@ -1778,6 +1802,8 @@ class MergeState(Block): | |
| re.VERBOSE, | ||
| ) | ||
|
|
||
| DEFAULT_BUILD_COMMAND = None | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Usually |
||
|
|
||
| @staticmethod | ||
| def iter_existing_names(): | ||
| """Iterate over the names of existing MergeStates in this repo.""" | ||
|
|
@@ -1860,6 +1886,38 @@ class MergeState(Block): | |
| except CalledProcessError: | ||
| return None | ||
|
|
||
| @staticmethod | ||
| def set_default_test_command(name): | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this method also set/unset |
||
| """Set the default test command to the specified one. | ||
|
|
||
| name can be None to cause the default to be cleared.""" | ||
|
|
||
| if name is None: | ||
| try: | ||
| check_call(['git', 'config', '--unset', 'imerge.command']) | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should probably be
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done, and fixed the other place where this was used. I actually think this helps with some confusion that I was having with set_default_name. |
||
| except CalledProcessError as e: | ||
| if e.returncode == 5: | ||
| # Value was not set | ||
| pass | ||
| else: | ||
| raise | ||
| else: | ||
| check_call(['git', 'config', 'imerge.command', name]) | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the name of this config variable should be more reminiscent of the name of the option; maybe
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
|
|
||
| @staticmethod | ||
| def get_default_test_command(): | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, why is "
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe I was following get_default_name() for getting the imerge name. There doesn't seem to be any calls to 'git config' that are not in functions with "default" in the name. How do you record different imerge names for multiple imerges?
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The only thing that Otherwise, nothing has been stored in the configuration for imerges; instead, the rest of the state was recorded in the object database as commits and blobs and references. The main information for an imerge is stored in a blob referred to by If you want to store a command in the configuration as a string for the imerge named and to retrieve it again, Hope that helps! |
||
| """Get the name of the test command, or None if none is currently set.""" | ||
|
|
||
| if MergeState.DEFAULT_BUILD_COMMAND: | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please compare explicitly against Also, you can be more idiomatic (and save a line or two of code) by inverting the Finally, please note that in the (common) case that there is no such config setting, you will call the same
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was trying to avoid the case of calling git config every time by using the static variable DEFAULT_BUILD_COMMAND. The variable is initialized to None and assigned a non None value by check_output. My confusing is coming from 'set_default_name'. I was attempting to set the test command in git config the same way the imerge name is set. I couldn't figure out the right way to set the test command to be anything other than a default value. How are other values of the imerge name set in the git config file?
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See my comment above where I show how to store the command in the git config.
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The way I read the code is that if the configuration value is unset, then I think you need a way to distinguish "command hasn't been checked yet; we don't know what it is" from "command has been checked but it was not set"; either an additional boolean variable that keeps track of whether you have tried to read the command already, or maybe store another value ( |
||
| return MergeState.DEFAULT_BUILD_COMMAND | ||
| else: | ||
| try: | ||
| MergeState.DEFAULT_BUILD_COMMAND = \ | ||
| check_output(['git', 'config', 'imerge.command']).rstrip() | ||
| return MergeState.DEFAULT_BUILD_COMMAND | ||
| except CalledProcessError: | ||
| return None | ||
|
|
||
| @staticmethod | ||
| def _check_no_merges(commits): | ||
| multiparent_commits = [ | ||
|
|
@@ -1891,7 +1949,7 @@ class MergeState(Block): | |
| name, merge_base, | ||
| tip1, commits1, | ||
| tip2, commits2, | ||
| goal=DEFAULT_GOAL, manual=False, branch=None, | ||
| goal=DEFAULT_GOAL, manual=False, branch=None, build_command=None, | ||
| ): | ||
| """Create and return a new MergeState object.""" | ||
|
|
||
|
|
@@ -1915,6 +1973,7 @@ class MergeState(Block): | |
| goal=goal, | ||
| manual=manual, | ||
| branch=branch, | ||
| build_command=build_command, | ||
| ) | ||
|
|
||
| @staticmethod | ||
|
|
@@ -2109,13 +2168,15 @@ class MergeState(Block): | |
| goal=DEFAULT_GOAL, | ||
| manual=False, | ||
| branch=None, | ||
| build_command=None, | ||
| ): | ||
| Block.__init__(self, name, len(commits1) + 1, len(commits2) + 1) | ||
| self.tip1 = tip1 | ||
| self.tip2 = tip2 | ||
| self.goal = goal | ||
| self.manual = bool(manual) | ||
| self.branch = branch or name | ||
| self.build_command = build_command | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see where this attribute is ever used. ...which probably goes some way towards answering why the class field has "default" in its name.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed this (although I feel like it might have to be re-added and the DEFAULT_TEST_COMMAND variable removed)
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's my feeling, too. |
||
|
|
||
| # A simulated 2D array. Values are None or MergeRecord instances. | ||
| self._data = [[None] * self.len2 for i1 in range(self.len1)] | ||
|
|
@@ -2675,6 +2736,18 @@ def main(args): | |
| action='store', default=None, | ||
| help='the name of the branch to which the result will be stored', | ||
| ) | ||
| subparser.add_argument( | ||
| '--build-command', | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's a lot of duplication here among subparser options. It would be great to have a function to add this option. The same function could also handle other options that are common to the subcommands that initiate incremental merges, like
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah that's what I thought too.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've fallen in love with the really awesome 'docopt' package for help messages and options. I understand that we don't want to add any dependencies however. Added a function for this.
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I do like about |
||
| action='store', default=None, | ||
| help=( | ||
| 'in addition to identifying for textual conflicts, run the build ' | ||
| 'or test script specified by TEST to identify where logical ' | ||
| 'conflicts are introduced. The test script is expected to return 0 ' | ||
| 'if the source is good, exit with code 1-127 if the source is bad, ' | ||
| 'except for exit code 125 which indicates the source code can not ' | ||
| 'be built or tested.' | ||
| ), | ||
| ) | ||
| subparser.add_argument( | ||
| '--manual', | ||
| action='store_true', default=False, | ||
|
|
@@ -2716,6 +2789,18 @@ def main(args): | |
| action='store', default=None, | ||
| help='the name of the branch to which the result will be stored', | ||
| ) | ||
| subparser.add_argument( | ||
| '--build-command', | ||
| action='store', default=None, | ||
| help=( | ||
| 'in addition to identifying for textual conflicts, run the build ' | ||
| 'or test script specified by TEST to identify where logical ' | ||
| 'conflicts are introduced. The test script is expected to return 0 ' | ||
| 'if the source is good, exit with code 1-127 if the source is bad, ' | ||
| 'except for exit code 125 which indicates the source code can not ' | ||
| 'be built or tested.' | ||
| ), | ||
| ) | ||
| subparser.add_argument( | ||
| '--manual', | ||
| action='store_true', default=False, | ||
|
|
@@ -2754,6 +2839,18 @@ def main(args): | |
| action='store', default=None, | ||
| help='the name of the branch to which the result will be stored', | ||
| ) | ||
| subparser.add_argument( | ||
| '--build-command', | ||
| action='store', default=None, | ||
| help=( | ||
| 'in addition to identifying for textual conflicts, run the build ' | ||
| 'or test script specified by TEST to identify where logical ' | ||
| 'conflicts are introduced. The test script is expected to return 0 ' | ||
| 'if the source is good, exit with code 1-127 if the source is bad, ' | ||
| 'except for exit code 125 which indicates the source code can not ' | ||
| 'be built or tested.' | ||
| ), | ||
| ) | ||
| subparser.add_argument( | ||
| '--manual', | ||
| action='store_true', default=False, | ||
|
|
@@ -2894,6 +2991,18 @@ def main(args): | |
| action='store', default=None, | ||
| help='the name of the branch to which the result will be stored', | ||
| ) | ||
| subparser.add_argument( | ||
| '--build-command', | ||
| action='store', default=None, | ||
| help=( | ||
| 'in addition to identifying for textual conflicts, run the build ' | ||
| 'or test script specified by TEST to identify where logical ' | ||
| 'conflicts are introduced. The test script is expected to return 0 ' | ||
| 'if the source is good, exit with code 1-127 if the source is bad, ' | ||
| 'except for exit code 125 which indicates the source code can not ' | ||
| 'be built or tested.' | ||
| ), | ||
| ) | ||
| subparser.add_argument( | ||
| '--manual', | ||
| action='store_true', default=False, | ||
|
|
@@ -3040,9 +3149,11 @@ def main(args): | |
| tip2, commits2, | ||
| goal=options.goal, manual=options.manual, | ||
| branch=(options.branch or options.name), | ||
| build_command=options.build_command, | ||
| ) | ||
| merge_state.save() | ||
| MergeState.set_default_name(options.name) | ||
| MergeState.set_default_test_command(options.build_command) | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So there is only one build command shared by all It seem to me that there are two possible self-consistent policies:
I prefer the former.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, the former makes sense. I'll see what I can do.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is your above comment true for the imerge name? Where is the git config value set for different imerge branches? This will help me understand how to accomplish this for different test commands. |
||
| elif options.subcommand == 'start': | ||
| require_clean_work_tree('proceed') | ||
|
|
||
|
|
@@ -3068,9 +3179,11 @@ def main(args): | |
| tip2, commits2, | ||
| goal=options.goal, manual=options.manual, | ||
| branch=(options.branch or options.name), | ||
| build_command=options.build_command, | ||
| ) | ||
| merge_state.save() | ||
| MergeState.set_default_name(options.name) | ||
| MergeState.set_default_test_command(options.build_command) | ||
|
|
||
| try: | ||
| merge_state.auto_complete_frontier() | ||
|
|
@@ -3126,9 +3239,11 @@ def main(args): | |
| tip2, commits2, | ||
| goal=options.goal, manual=options.manual, | ||
| branch=options.branch, | ||
| build_command=options.build_command, | ||
| ) | ||
| merge_state.save() | ||
| MergeState.set_default_name(name) | ||
| MergeState.set_default_test_command(options.build_command) | ||
|
|
||
| try: | ||
| merge_state.auto_complete_frontier() | ||
|
|
@@ -3188,9 +3303,11 @@ def main(args): | |
| tip2, commits2, | ||
| goal=options.goal, manual=options.manual, | ||
| branch=options.branch, | ||
| build_command=options.build_command, | ||
| ) | ||
| merge_state.save() | ||
| MergeState.set_default_name(options.name) | ||
| MergeState.set_default_test_command(options.build_command) | ||
|
|
||
| try: | ||
| merge_state.auto_complete_frontier() | ||
|
|
||
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 think it would be appropriate for
AutomaticMergeFailedandAutomaticBuildFailedto inherit from a common base class, as suggested here. After all, they both represent a failure of an automatic merge, albeit for different reasons. The base class could also manage the commit arguments.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.
Yep
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.
Done