Skip to content
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

Use merge-base (three dot diff) instead of direct diff for git-diff-ase (#1653) #1654

Merged
merged 8 commits into from Jan 26, 2022

Conversation

bdsl
Copy link
Contributor

@bdsl bdsl commented Jan 24, 2022

This PR:

Fixes #1653

Currently when using --git-diff-base=master infection generates mutants for any lines that appear in a direct diff between HEAD and master. This PR changes that to use diff between HEAD and the merge-base of HEAD and master - i.e. the diff you see on a github pull request.

We use the three dot diff syntax for git to get the comparison between the merge base and HEAD. This diff shows work done on the HEAD branch, and should ignore any changes made in the parallel on the base branch. This seems more useful for evaluating the head branch, and fits the description:

'Mutates only added and modified <comment>lines</comment> in files.',

bdsl added a commit to bdsl/site that referenced this pull request Jan 24, 2022
@maks-rafalko maks-rafalko added this to the next milestone Jan 25, 2022
@maks-rafalko
Copy link
Member

hello

  1. please, run make cs to fix code style issues
  2. seems like there is now an issue with this job: Annotations / Mutation Testing Code Review Annotations 8.0 (pull_request) https://github.com/infection/infection/runs/4932358491?check_suite_focus=true#step:7:9 It says [OK] No files in diff found, skipping mutation analysis. while there is obviously a PHP file in the diff that should be mutated

@bdsl
Copy link
Contributor Author

bdsl commented Jan 25, 2022

Thanks Maks. Would you mind taking a look? I added some temporary debugging code, which shows that the generated command line is git diff 'origin/master...HEAD' --diff-filter='AM' --name-only | grep src/ | paste -s -d "," -. Running that on my laptop I get the expected output:

src/Logger/GitHub/GitDiffFileProvider.php

@maks-rafalko
Copy link
Member

maks-rafalko commented Jan 25, 2022

I didn't dig yet, but I think this is related to how GitHub clones the repo.

Try locally to do the same as GitHub does, here:

/usr/bin/git -c protocol.version=2 fetch --no-tags --prune --progress --no-recurse-submodules --depth=1 origin +ec43076a8189aa8a0dfc6c8028a6e5c9bc178e21:refs/remotes/pull/1654/merge

then here

git fetch --depth=1 origin $GITHUB_BASE_REF
php bin/infection -j2 --git-diff-lines --git-diff-base=origin/$GITHUB_BASE_REF --logger-github --ignore-msi-with-no-mutations --only-covered

so, probably after such cloning, there won't be any diff by using git diff 'origin/master...HEAD' --diff-filter='AM' --name-only | grep src/ | paste -s -d "," -

@bdsl
Copy link
Contributor Author

bdsl commented Jan 25, 2022

ah right, that depth=1 truncates the history. Without access to the history it would be impossible to distinguish between lines changed on the head branch and lines changed on master.

@bdsl
Copy link
Contributor Author

bdsl commented Jan 25, 2022

I think we can make it use the merge-base for comparison if available in the checkout, if not we can fall back to doing a direct diff between commits. Probably worth advising users to make CI servers do deep checkouts to ensure we can find the merge-base and don't mutate more (or occasionally less) than needed.

Copy link
Member

@maks-rafalko maks-rafalko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

cool, thank you @bdsl

@maks-rafalko maks-rafalko enabled auto-merge (squash) January 26, 2022 11:49
@maks-rafalko maks-rafalko merged commit ae4435e into infection:master Jan 26, 2022
@bdsl
Copy link
Contributor Author

bdsl commented Jan 26, 2022

@maks-rafalko Welcome, thank you too. Do you know when this is most likely to be in a release?

@bdsl
Copy link
Contributor Author

bdsl commented Jan 26, 2022

It would be great if it can be on the 0.26 branch (maybe requires backport / cherry-pick ?) since we're still using PHP 7.4

@maks-rafalko
Copy link
Member

maks-rafalko commented Jan 26, 2022

if you want it to be released for 7.4, please open a new PR with cherry-picking it to 0.26 branch as a target and I will release it ASAP

@bdsl
Copy link
Contributor Author

bdsl commented Jan 26, 2022

Will do, thanks.

bdsl added a commit to bdsl/infection that referenced this pull request Jan 26, 2022
…se (infection#1653) (infection#1654)

* Use merge-base (three dot diff) instead of direct diff for git-diff-base (infection#1653)

* Add command line dumping for debugging

* Fetch full history to allow finding common ancestor, not just last commit

* Remove var_dumps

* Debug with git depth 1

* Fall back to direct diff if merge base commit is not avialable

* Restore deep git fetch

* Kill UnwrapTrim mutant
@bdsl
Copy link
Contributor Author

bdsl commented Jan 26, 2022

PR for 0.26: #1655

maks-rafalko pushed a commit that referenced this pull request Jan 27, 2022
#1655)

* Use merge-base (three dot diff) instead of direct diff for git-diff-ase (#1653) (#1654)

* Use merge-base (three dot diff) instead of direct diff for git-diff-base (#1653)

* Add command line dumping for debugging

* Fetch full history to allow finding common ancestor, not just last commit

* Remove var_dumps

* Debug with git depth 1

* Fall back to direct diff if merge base commit is not avialable

* Restore deep git fetch

* Kill UnwrapTrim mutant

* Fix php parse error

* Add test_it_falls_back_to_direct_diff_if_merge_base_is_not_availabe

* Skip test on Windows
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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