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

Fix reading with limit and global-revisions #164

Merged
merged 6 commits into from
Oct 22, 2024

Conversation

felixjung
Copy link
Contributor

@felixjung felixjung commented Oct 11, 2024

I think I introduced a bug with #158. This should address it.

When processing the earliest commit of the commit history, no file content
outside the history is present. This means that no changes can be calculated
for the earliest commit.

This issue was likely always present but surfaced by the introduction of the --global-revisions flag. Previously, the issue would only appear when reaching the commit that introduced a file or the end of the repository's history.

To address the problem, we now read the file's content at the previous commit in the repo. The code fails gracefully, if git returns an error when attempting to read the file.

When processing the earliest commit of the commit history, no data from the file
outside of the history is present. This means that no changes can be calculated
for the first commit.

This issue was likely always present but surfaced by the introduction of the
`--global-revisions` flag. Previously, the issue would only appear when reaching
the commit that introduced a file.

To address the problem, we now read the file's content at the previous commit in the
repo.

This still does not resolve the problem caused by a file simply not being
present at the previous commit.
@felixjung felixjung marked this pull request as ready for review October 11, 2024 13:52
@felixjung felixjung changed the title Fix reading with limit Fix reading with limit and global-revisions Oct 11, 2024
@felixjung
Copy link
Contributor Author

This works for me locally, but it somehow fails in GitHub actions. Going to debug before asking to merge this.

Error: unable to read git repository './' (are you sure it's a git repo?): exit status 128

@felixjung felixjung marked this pull request as draft October 11, 2024 14:06
@daveshanley
Copy link
Member

This works for me locally, but it somehow fails in GitHub actions. Going to debug before asking to merge this.

Error: unable to read git repository './' (are you sure it's a git repo?): exit status 128

I have no idea what is going on here. Could be a depth cloning issue.

I had to add fetch-depth to vacuum the other day.

    steps:
      - name: Checkout code
        uses: actions/checkout@v4
        with:
          fetch-depth: 50
          fetch-tags: true
          show-progress: true

@felixjung
Copy link
Contributor Author

felixjung commented Oct 13, 2024

It was indeed the fetch depth. Pretty annoying. I still ended up adding some code that I think makes the parsing of positional args a bit more robust. I see that code is duplicated across the different commands. Should I extract into a function and apply it everywhere?

@felixjung felixjung marked this pull request as ready for review October 13, 2024 10:55
@daveshanley
Copy link
Member

It was indeed the fetch depth. Pretty annoying. I still ended up adding some code that I think makes the parsing of positional args a bit more robust. I see that code is duplicated across the different commands. Should I extract into a function and apply it everywhere?

If you have the time? Always appreciated.

@daveshanley daveshanley merged commit 32a921a into pb33f:main Oct 22, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants