Rugged 1.7.1 breaks gem

[cooljacob204]

Rugged update 1.7.1 which came out yesterday breaks the gem.

I'm using it in a github action.

/lib/rugged/repository.rb:114:in `diff': undefined method `[]' for nil:NilClass (NoMethodError)

        right.diff(left, opts.merge(:reverse => !opts[:reverse]))
                                                     ^^^^^^^^^^
	from /opt/hostedtoolcache/Ruby/3.1.4/x64/lib/ruby/gems/3.1.0/gems/pronto-0.11.1/lib/pronto/git/repository.rb:30:in `diff'
	from /opt/hostedtoolcache/Ruby/3.1.4/x64/lib/ruby/gems/3.1.0/gems/pronto-0.11.1/lib/pronto.rb:64:in `run'
	from /opt/hostedtoolcache/Ruby/3.1.4/x64/lib/ruby/gems/3.1.0/gems/pronto-0.11.1/lib/pronto/cli.rb:70:in `block in run'
	from /opt/hostedtoolcache/Ruby/3.1.4/x64/lib/ruby/gems/3.1.0/gems/pronto-0.11.1/lib/pronto/cli.rb:6[8]:in `chdir'
	from /opt/hostedtoolcache/Ruby/3.1.4/x64/lib/ruby/gems/3.1.0/gems/pronto-0.11.1/lib/pronto/cli.rb:68:in `run'
	from /opt/hostedtoolcache/Ruby/3.1.4/x64/lib/ruby/gems/3.1.0/gems/thor-1.2.2/lib/thor/command.rb:27:in `run'
	from /opt/hostedtoolcache/Ruby/3.1.4/x64/lib/ruby/gems/3.1.0/gems/thor-1.2.2/lib/thor/invocation.rb:127:in `invoke_command'
	from /opt/hostedtoolcache/Ruby/3.1.4/x64/lib/ruby/gems/3.1.0/gems/thor-1.2.2/lib/thor.rb:3[9]2:in `dispatch'
	from /opt/hostedtoolcache/Ruby/3.1.4/x64/lib/ruby/gems/3.1.0/gems/thor-1.2.2/lib/thor/base.rb:485:in `start'
	from /opt/hostedtoolcache/Ruby/3.1.4/x64/lib/ruby/gems/3.1.0/gems/pronto-0.[11].1/bin/pronto:6:in `<top (required)>'
	from /opt/hostedtoolcache/Ruby/3.1.4/x64/bin/pronto:25:in `load'
	from /opt/hostedtoolcache/Ruby/3.1.4/x64/bin/pronto:25:in `<main>'

Github action workflow:

---
name: Pronto
on: [pull_request]

jobs:
  pronto:
    if: ${{ !contains(github.actor, '[bot]') }}
    runs-on: ubuntu-latest
    steps:
      - name: Checkout code
        uses: actions/checkout@v3
      - run: |
          git fetch --no-tags --prune --depth=50 origin +refs/heads/*:refs/remotes/origin/*
      - name: Setup Ruby
        uses: ruby/setup-ruby@v1
      - name: Setup pronto
        run: gem install rubocop-rails pronto pronto-rubocop
      - name: Run Pronto
        run: >
          PRONTO_PULL_REQUEST_ID="${{ github.event.pull_request.number }}"
          PRONTO_GITHUB_ACCESS_TOKEN="${{ github.token }}"
          pronto run -f github_status text -c origin/${{ github.base_ref }}

Fixed by pinning rugged to 1.6.3. Action runs with ruby 3.1.4 which is specified in our .ruby-version.

[rdlugosz]

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
...

[ashkulz]

Is there a repository which can reproduce this? I have a fix in mind, but won't know if it works without reproducing it...

[cooljacob204]

I have reproduced it in a pet project of mine: gcp-sargeras/gcp-sargeras.com#32

[ashkulz]

@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?

[cooljacob204]

@ashkulz I have been only able to reproduce it in a GitHub action, I also haven't been able to hit the error locally.

[pbstriker38]

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

[pbstriker38]

And I think this is happening on that PR because the only file is a new file.

[pbstriker38]

@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

[ashkulz]

@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.

[pbstriker38]

Other option is to fail early if there is no file

[cooljacob204]

@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.

[pbstriker38]

@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 }}

[cooljacob204]

And you got that git fetch command from the README here?

Yup.

[X-sam]

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

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.

[X-sam]

In fact here's a PR with that fix, if it is indeed useful #459

[X-sam]

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

[ashkulz]

@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?

[X-sam]

@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.

[jeantristan]

Is it possible to merge the PR associated? #459

[ashkulz]

@jeantristan see this comment from the PR author @X-sam -- it seems to be a bug in rugged itself.