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

Get changed files from total diff #67

Merged
merged 1 commit into from
Jan 4, 2021

Conversation

DasSkelett
Copy link
Member

@DasSkelett DasSkelett commented Dec 31, 2020

Problem

See #66, to get the list of changed files we are currently merging all the changed files of each new commit, even if in the end the file didn't change at all compared to the base branch/ref.

Two examples where this can happen:

  1. One commit modifies a file, and another commit reverts it again (e.g. because of PR review)
  2. Users "rebasing"/updating their forks with merge commits instead of clean fast-forward merges (Remove KSPIE Incompatibilty NetKAN#8292). Now that merge commit contains all changes from when the last update happened to now, which could be weeks or months worth of CKAN-meta/NetKAN commits.

In both cases, these files actually haven't been changed compared to the base branch (usually current CKAN-meta/NetKAn master). So we don't need to include them in our validation.

Changes

Now instead of iterating over each commit and adding all the changes of the diff of each commit together, we get the total/final diff and extract the changed files from that one.

I've tested this using the following PoC code:
test.py:

from git import Repo

print('Old:')
changed_files = list()
for diffIndex in [commit.parents[0].diff(commit) for commit in Repo('.').iter_commits('upstream/master..HEAD')]:
    for diff in diffIndex.iter_change_type('A'):
        changed_files.append(diff.b_path)
print(changed_files) 

print('New:')
diffIndex = Repo('.').commit('upstream/master').diff('HEAD')
changed_files = [ch.b_path for ch in diffIndex.iter_change_type('A')]
print(changed_files)

Terminal:

cd NetKAN/
git reset --hard HEAD~5
# Create a merge commit similar to https://github.com/KSP-CKAN/NetKAN/pull/8292 :
git merge --no-ff upstream/master
python3 test.py

Output:

Old:
['.github/workflows/inflate.yml', 'NetKAN/KiwiTechTree.netkan', 'NetKAN/TestLite.netkan']
New:
[]

Fixes #66

@DasSkelett DasSkelett added the Bug label Dec 31, 2020
@DasSkelett DasSkelett changed the title Get changed files from diff Get changed files from total diff Dec 31, 2020
@DasSkelett
Copy link
Member Author

DasSkelett commented Dec 31, 2020

Wanted to test this with a PR in my NetKAN fork (DasSkelett/NetKAN#2), but this isn't as easy anymore since KSP-CKAN/CKAN@ad3ccbe / 6f96e28, hang on...

Edit: got it to run using another branch with a revert of 6f96e28: https://github.com/DasSkelett/NetKAN/pull/2/checks?check_run_id=1628906987
Looks like it does work.

Copy link
Member

@HebaruSan HebaruSan left a comment

Choose a reason for hiding this comment

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

Thanks!

@HebaruSan HebaruSan merged commit a0ff417 into KSP-CKAN:master Jan 4, 2021
@DasSkelett DasSkelett deleted the fix/file-detection branch January 4, 2021 03:18
@HebaruSan
Copy link
Member

Awesome.
https://github.com/KSP-CKAN/NetKAN/runs/1641738991?check_suite_focus=true

@techman83
Copy link
Member

Brilliant!

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.

[Bug] Inflating way too many things with weird git branches
3 participants