git-diff-base should use merge-base instead of direct diff

[bdsl]

Question	Answer
Infection version	0.26.0
Test Framework version	PHPUnit 9.5.4
PHP version	7.4
Platform	e.g. Ubuntu/Windows/MacOS

I'm not sure whether this should be classified as a bug or a change request. The docs don't seem to specify exactly what the behaviour should be.

Currently when using --git-diff-base=master infection seems to be generating mutants for any lines that appear in a direct diff between HEAD and master. I think instead it should use a diff between HEAD and the merge-base of HEAD and master - i.e. the diff you see on a github pull request.

We are trying out infection to give us an automatic critique of work done in PRs, and it's confusing when this includes stuff that changed on master because of an unrelated PR.

I can work around the issue for now by running git merge-base HEAD origin/master and passing the output of that to the CLI param.

[maks-rafalko]

Let me ask if I understood the issue correctly.

Given the following example from merge-base docs:

	 o---o---o---B
	/
---o---X---o---o---o---A

1. do you want Infection to automatically do a diff between B and X?
2. do you say that currently Infection does a diff between B and A?

If yes & yes, what do you think if we add this information to the docs and allow people to control it themselves?

If they want to diff with merge-base, they do it like you said.
If they want to diff between HEAD and current master - just --git-diff-base=master

Does it make sense?

[bdsl]

Yes, if B is HEAD then when I do --git-diff-base=A I would like to see mutants generated only for the diff between B and X - i.e. for the changes introduced on branch B. That seems natural for evaluating the quality of work done in branch B and to inform a decision about whether it's ready to merge into A.

We could just add something to the docs, but I'm not sure what the use case is for the direct diff between B and A.

[bdsl]

I think typically the diff between B and X is going to be very similar to the diff between the hypothetical commit that merges B into A and A.

[bdsl]

Happy to make a PR for this, could probably use some guidance since I haven't contributed to infection before. I guess there's an option to introduce a new command line argument instead of changing the behaviour of the existing one.

[maks-rafalko]

Happy to make a PR for this, could probably use some guidance since I haven't contributed to infection before

awesome.

I would start from here:

infection/src/Logger/GitHub/GitDiffFileProvider.php

Lines 55 to 76 in 4e21783

	public function provide(string $gitDiffFilter, string $gitDiffBase): string
	{
	$filter = $this->shellCommandLineExecutor->execute(sprintf(
	'git diff %s --diff-filter=%s --name-only | grep src/ | paste -s -d "," -',
	escapeshellarg($gitDiffBase),
	escapeshellarg($gitDiffFilter)
	));
	if ($filter === '') {
	throw NoFilesInDiffToMutate::create();
	}
	return $filter;
	}
	public function provideWithLines(string $gitDiffBase): string
	{
	return $this->shellCommandLineExecutor->execute(sprintf(
	"git diff %s --unified=0 --diff-filter=AM | grep -v -e '^[+-]' -e '^index'",
	escapeshellarg($gitDiffBase)
	));
	}

the first method is used for --git-diff-filter option, the second one is for --git-diff-lines. I believe both options should work with the new merge-base update.

I guess there's an option to introduce a new command line argument instead of changing the behaviour of the existing one.

I'm personally ok with updating behavior for current options. We are on 0.x branch, so BC changes are kind of expected. At least it's better than introducing 2 more options in addition to 2 mentioned above

[bdsl]

Thanks, I'll see if I can get the PR in this weekend.

[maks-rafalko]

Released: https://github.com/infection/infection/releases/tag/0.26.3