Skip to content

fall back to using PR target branch when determining "merge base" between PR branch & target branch fails in test suite#11069

Merged
vanzod merged 2 commits intoeasybuilders:developfrom
boegel:git_merge_base
Aug 6, 2020
Merged

fall back to using PR target branch when determining "merge base" between PR branch & target branch fails in test suite#11069
vanzod merged 2 commits intoeasybuilders:developfrom
boegel:git_merge_base

Conversation

@boegel
Copy link
Copy Markdown
Member

@boegel boegel commented Aug 6, 2020 •
edited
Loading

Some changes were made in Git 2.28.0 that break how we determine the list of changed files in PRs.

For example (from tests for #11068):

ERROR: test_changed_files_pull_request (test.easyconfigs.easyconfigs.EasyConfigTest)
Specific checks only done for the (easyconfig) files that were changed in a pull request.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "test/easyconfigs/easyconfigs.py", line 739, in test_changed_files_pull_request
    changed_ecs_filenames = get_eb_files_from_diff(diff_filter='M')
  File "test/easyconfigs/easyconfigs.py", line 708, in get_eb_files_from_diff
    out, ec = run_cmd(cmd, simple=False)
  File "/opt/hostedtoolcache/Python/2.7.18/x64/lib/python2.7/site-packages/easybuild/tools/run.py", line 88, in cache_aware_func
    res = func(cmd, *args, **kwargs)
  File "/opt/hostedtoolcache/Python/2.7.18/x64/lib/python2.7/site-packages/easybuild/tools/run.py", line 269, in run_cmd
    return parse_cmd_output(cmd, stdouterr, ec, simple, log_all, log_ok, regexp)
  File "/opt/hostedtoolcache/Python/2.7.18/x64/lib/python2.7/site-packages/easybuild/tools/run.py", line 533, in parse_cmd_output
    raise EasyBuildError('cmd "%s" exited with exit code %s and output:\n%s', cmd, ec, stdouterr)
EasyBuildError: 'cmd "git diff --name-only --diff-filter=M develop...HEAD" exited with exit code 128 and output:\nfatal: develop...HEAD: no merge base\n'

From https://raw.githubusercontent.com/git/git/master/Documentation/RelNotes/2.28.0.txt:

"git diff" used to take arguments in random and nonsense range
   notation, e.g. "git diff A..B C", "git diff A..B C...D", etc.,
   which has been cleaned up.

(hat tip @branfosj)

That basically means the triple dot (...) notation we were relying on is no longer working.

Can be fixed by determining the "merge base" between both branches first using git merge-base before using git diff.

edit: the changes to git diff may not actually be the cause of the problem here, it seems like determining the "merge base" is failing sometimes (reason unclear), see below

@boegel boegel added the bug fix label Aug 6, 2020
@boegel boegel added this to the next release (4.2.3?) milestone Aug 6, 2020
@boegel
Copy link
Copy Markdown
Member Author

boegel commented Aug 6, 2020

It seems that sometimes determining the "merge base" fails (without a clear reason), so falling back to just using the target branch is reasonable (it results in more "changed files" being reported, but that only slightly slows down the tests, it shouldn't result in other problems I think). Fixed in ac746c7

@boegel boegel changed the title determine merge base of target branch and PR branch via 'git merge-base', to avoid problems with git 2.28+ fall back to using PR target branch when determining "merge base" between PR branch & target branch fails in test suite Aug 6, 2020
@vanzod vanzod merged commit f1868c1 into easybuilders:develop Aug 6, 2020
@boegel boegel deleted the git_merge_base branch August 6, 2020 21:13
@boegel boegel mentioned this pull request Aug 6, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vanzod vanzod vanzod approved these changes

Assignees

No one assigned

Labels

bug fix

Projects

None yet

Milestone

4.3.0

Development

Successfully merging this pull request may close these issues.

2 participants

@boegel @vanzod