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.