Open cooljacob204 opened 10 months ago
If you're here because you're seeing a Github Action failing with this error, you can pin your version of rugged to 1.6.3 by updating the Github Actions Integration example code as follows:
...
- name: Setup pronto
run: gem install rugged:1.6.3 pronto pronto-rubocop
...
Is there a repository which can reproduce this? I have a fix in mind, but won't know if it works without reproducing it...
I have reproduced it in a pet project of mine: https://github.com/gcp-sargeras/gcp-sargeras.com/pull/32
@cooljacob204 it's not failing for me:
git clone https://github.com/gcp-sargeras/gcp-sargeras.com.git
cd gcp-sargeras.com
git checkout rugged-test
gem install rubocop-rails pronto pronto-rubocop
pronto run -f text -c origin/main # gives no error
pronto run -f github_status text -c origin/main # fails with an error about status reporting, but has gone past the error in GH Actions
Do you know what I'm missing here?
@ashkulz I have been only able to reproduce it in a GitHub action, I also haven't been able to hit the error locally.
This is happening because Pronto.run
is being called without a file
which then causes the options being passed to repo.diff
to be nil
. The nil
options are then passed to @repo.diff(merge_base, head, options)
Simple fix is to do this:
patches = @repo.diff(merge_base, head, options || {})
https://github.com/prontolabs/pronto/blob/v0.11.2/lib/pronto/cli.rb#L70 https://github.com/prontolabs/pronto/blob/v0.11.2/lib/pronto.rb#L59 https://github.com/prontolabs/pronto/blob/v0.11.2/lib/pronto.rb#L64 https://github.com/prontolabs/pronto/blob/v0.11.2/lib/pronto/git/repository.rb#L30 https://github.com/libgit2/rugged/blob/v1.7.1/lib/rugged/repository.rb#L114
And I think this is happening on that PR because the only file is a new file.
@cooljacob204 can you try removing that git fetch ...
after the actions/checkout step. Also try adding fetch depth of 0 on the checkout.
- uses: actions/checkout@v4
with:
fetch-depth: 0
@pbstriker38 I had the same fix in mind (it's obvious) but wasn't sure this would be all. Let me try and see if the information you gave can help reproduce it.
Other option is to fail early if there is no file
@cooljacob204 can you try removing that
git fetch ...
after the actions/checkout step. Also try adding fetch depth of 0 on the checkout.- uses: actions/checkout@v4 with: fetch-depth: 0
No error with this version of the checkout step.
@cooljacob204
Great. And you got that git fetch
command from the README here?
https://github.com/prontolabs/pronto#github-actions-integration
I'm not sure why that fetch would even be necessary given that actions/checkout
has a fetch-depth
option. 0 means everything, but you can also provide a specific depth depending on your needs. A depth of 0 is a safe bet and is pretty fast even on large repos.
@ashkulz
The README and wiki should probably be updated to show the setup that is most likely to work for everyone. If someone wants to speed things up by doing a specific git fetch
then that is an advanced use case that is up to them.
name: Pronto
on: [pull_request]
jobs:
pronto:
runs-on: ubuntu-latest
steps:
- name: Checkout code
uses: actions/checkout@v4
with:
fetch-depth: 0
- name: Setup Ruby
uses: ruby/setup-ruby@v1
- name: Setup pronto
run: gem install pronto pronto-rubocop
- name: Run Pronto
env:
PRONTO_PULL_REQUEST_ID: ${{ github.event.pull_request.number }}
PRONTO_GITHUB_ACCESS_TOKEN: "${{ secrets.GITHUB_TOKEN }}"
run: pronto run -f github_status github_pr_review -c origin/${{ github.base_ref }}
And you got that git fetch command from the README here?
Yup.
This is happening because
Pronto.run
is being called without afile
which then causes the options being passed torepo.diff
to benil
. Thenil
options are then passed to@repo.diff(merge_base, head, options)
Simple fix is to do this:
patches = @repo.diff(merge_base, head, options || {})
https://github.com/prontolabs/pronto/blob/v0.11.2/lib/pronto/cli.rb#L70 https://github.com/prontolabs/pronto/blob/v0.11.2/lib/pronto.rb#L59 https://github.com/prontolabs/pronto/blob/v0.11.2/lib/pronto.rb#L64 https://github.com/prontolabs/pronto/blob/v0.11.2/lib/pronto/git/repository.rb#L30 https://github.com/libgit2/rugged/blob/v1.7.1/lib/rugged/repository.rb#L114
Is there any reason not to make this one line change? As someone who's impacted by this, it would be nice to be able to use Pronto alongside a less deep checkout.
In fact here's a PR with that fix, if it is indeed useful #459
That does not fix the whole problem, actually. Seems there's a breaking change in how shallow grafts are handled- in 1.7.1
shallow graft commits don't have parents listed, so this actually is only one piece of the updates necessary.
I suspect that piece is a bug in rugged, and have posted there.
libgit2/rugged#973
@X-sam thanks for the investigation, but I'm not comfortable with making changes without being able to at least reproduce what's going on.
How were you able to determine the issue in the grafts? Do you have a standalone reproduction?
@ashkulz If you look at the issue I've opened on rugged, there's a very brief set of steps to reproduce. Clone a shallow repo at depth=1
and then open it with rugged, on 1.7.1 Rugged::Repository.new("path/to/repo").head.target.parent_oids
will be []
and so the patch list pronto makes will be wrong.
Rugged update 1.7.1 which came out yesterday breaks the gem.
I'm using it in a github action.
Github action workflow:
Fixed by pinning rugged to 1.6.3. Action runs with ruby 3.1.4 which is specified in our .ruby-version.