Add functionality to detect build 'conflicts' while merging

[dleen]

This commit adds functionality to test for build failures during the merge
process. The build script is specified via the command line flag:
--build-command=make_script.

The make_script should return 0 if the source is good, 1-127 if the source
is bad, except for code 125 if the source at this commit cannot be built or
cannot be tested.

Please see the man page for git-bisect for examples of such a build script.

[dleen]

This solves most of issue #58.
I need to remove some print statements which I was using to see how automerge was called. As discussed in #58 doing a build every auto_outline is very expensive. There's no guarantee however that we're not catching all build failures without doing some sort of bisect on the auto_fills.

Also if you can suggest some additional tests than the ones I've added I'd gladly add them!

I'd love to hear your thoughts!

[mhagger]

Awesome that you are working on this! I'll add some comments and thoughts to the diff.

[mhagger]

I think it would be appropriate for AutomaticMergeFailed and AutomaticBuildFailed to 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.

[dleen]

Yep

[dleen]

Done

[mhagger]

I don't think you should use the word "build" in the UI and implementation, because in general people might want to run arbitrary tests before accepting a merge--maybe less than a full build, maybe more. Why not just call them "tests" to preserve generality?

[mhagger]

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

check_call(['/bin/sh', '-c', build_script])

then build_script could be an arbitrary shell command (or even sequence of commands). Then the user could specifiy something like

git imerge --build-command='make' [...]

or even

git imerge --build-command='make test' [...]

Please note that git bisect --run does things differently. It treats all of the git bisect arguments following the --run option as individual words of the command that it will run. This is more flexible still, and maybe a bit less confusing with respect to shell quoting. But it makes it harder to parse the git bisect command itself, because the command to be invoked is "the rest of the line", so no other options or arguments can follow it on the command line. Moreover, we will need to serialize the command somewhere, and it is easier to serialize a string than a list of strings. So I think git bisect's approach is not necessarily a model that git imerge should follow.

By the way, deviation from git bisect's style of handling the command is a good reason that our option shouldn't be called --run.

[dleen]

Agree

[dleen]

Done

[mhagger]

Whew, that was a lot of comments. It's maybe a bit unfair of me to be so detailed after such a long time of inactivity. That's because I have been wishing for and thinking about this feature even though I haven't gotten around to working on it. So it's great that you have come along and obviously have more energy than I do 😃

So anyway, I hope that you find my comments helpful rather than discouraging! It will be really nice to have this feature in git-imerge!

[talios]

It's maybe a bit unfair of me to be so detailed after such a long time of inactivity.

@mhagger For what's it worth as an idle bystander - I'd love it if anyone gave such a detailed set of review notes on a pull request I made. What may seem a small pedantic comment can sometimes turn out to be a critical issue down the path - so any discussion on changes is good IMHO ( especially on something that's going to be modifying a git repository! ).

[mhagger]

@mhagger For what's it worth as an idle bystander - I'd love it if anyone gave such a detailed set of review notes on a pull request I made.

@talios: Find an itch of your own to scratch, and you too can have a turn under the microscope 😃

[dleen]

Yeah I agree, the comments were great. I'm still working on them :)

[dleen]

Got most of the low hanging fruit. I still need some guidance on how to set the git config values correctly.

[dleen]

Finally found some time to work on this!

I've tried to address storing the test command in git config as best as I could. I'd love to know what you think! I've covered the main points: each imerge stores the test command under imerge.name.testcommand. The test commands are removed upon finishing. git config should be called at most twice: once to get whatever is stored under default and once to get the config for name. I haven't added a default test command yet but that should be easy (and if it isn't then my class isn't working as well as I hoped!)

I've separated the code for interacting with git config into another PR for easier reviewing: #69

[talios]

@dleen Any further movement on this PR at all? Would love to see this pulled in. I have a feeling this might come in useful to me in the near future :)

[dleen]

Hey @talios, I think I'm just waiting for a review by @mhagger for this and #69. This was a meaty PR and in the process we've improved imerge a bunch. If @mhagger has no further comments he can merge this in.

[talios]

+100 :) I note Github is saying this has PR has merge conflicts? I suspect related to #69?

[dleen]

The merge conflicts shouldn't be an issue, I'll rebase if needed.

[mhagger]

@dleen, @talios: I'm really sorry to be such a bottleneck, and given that I'm about to go on vacation for a few weeks I'm afraid things are not likely to get better real soon.

One thing that I could suggest is that we try to spread the work around a bit more; for example, if you would each spend some time reviewing the other's PRs that would be really awesome. I'd love to make this more of a community project so that everybody doesn't have to wait on me.

[talios]

I'd love to help with the reviews on PRs but my Python knowledge these days is sorely lacking ( haven't actually touched it in about 12+ years ) so I suspect my input would be marginal at best.

[talios]

Was there any movement on this PR at all? Had a situation the other day when this could have come in handy.

[dleen]

It was sort of blocked on #69 which I lost steam on

[brucehoult]

I really really really really want to see this functionality!

I'm happy to have the test that is run be a script. Yes, I'll write it specifically for this purpose! For a start, because I'll want it to for speed reasons do an incremental build, but if that fails then I'll want to do make clean and then try a full build rather than give up.

Also, I'd like to be able to run different tests depending on what kind of merge was done. In particular, I'd like to be able to run more thorough tests at a corner, once it's been proven that merges in both directions give the same result. Or perhaps only run tests at the corners, not at every step along each edge. Or only test on horizontal increments (my local changes), not vertical ones (upstream changes). Or vice versa.

Or maybe test merge conflict resolutions more. (that can/should also be done manually before the "contiinue")

All that is best done, I think, by passing an argument to the script. If it's actually a script name then it can be an explicit argument. If it's arbitrary shell commands then maybe it's better put into an environment variable.

I suggest the following values: PROBE HINC VINC CORNER CONFLICT

[brucehoult]

One more thing.

It is unfortunately the case that a merge that succeeds textually can fail more extensive tests (compilation, running a test suite) because of a bad commit in either the local changes (of course not in mine) or upstream master.

This is, alas, all too common in for example the llvm repository.

I wrote a script specifically to sanity check every commit in master before running a huge git-imerge rebase of a local branch from llvm/clang 3.6.0 (Feb 2015) to llvm 6.0.0 (March 2018) and found literally hundreds of bad versions in upstream. In once case I recall several dozen commits in a row (before a bad commit was reverted).

Some of the bad commits made clang fail to compile. Some of them made it fail to build HelloWorld. Some of them made the resulting HelloWorld fail to work.

The worst was one commit that created a clang that infinite looped when trying to build HelloWorld. That was unexpected.

So, there should be a way to blacklist commits from both branches. The user should be able to given an initial blacklist obtained by other means, and git-imerge should update the blacklist if other bad commits are found.

A text file with one commit hash per line is probably a good way to do this. The script of course should load this into a Python set.

If an HINC or VINC merge fails tests then the corresponding commit from the original branch should be tested and blacklisted if it fails. (or CORNER, if it runs more extensive tests than the HINC and VINC leading to it).

[brucehoult]

Here's an interesting example I hit just now.

Commit https://github.com/llvm-project/llvm-project-20170507/commit/52b1e2e91ee removed a virtual function called enableMultipleCopyHints() from a header file and all implementations of it from various back ends.

Our out of tree backend has an implementation of this function. Of course everything merged fine, but now it won't compile: " marked ‘override’, but does not override".

An ideal version of git-imerge would find the commit in our backend that introduces this function (and therefore does not compile), and then find the commit in master that removes it. Both by bisection, of course.