Ensure options is always a Hash in diff to avoid Rugged errors

[lmonzon4]

Description

This PR fixes an issue where Rugged::Repository#diff could receive nil as the options argument, causing the following error:

undefined method `[]' for nil:NilClass (NoMethodError)

This happens because the diff method in Pronto::Git::Repository passes options directly to Rugged without ensuring it’s a Hash. When options is nil (the default value), Rugged fails.

What changed

Initialized options with an empty Hash if nil:

options ||= {}

In :workdir case simplified .merge(options || {}) to .merge(options) since options is guaranteed to be a Hash.

Why

This makes the method safer and consistent across all cases. Previously:

For :workdir, a merge fallback (options || {}) was already in place.

For other cases (:unstaged, :index, :staged, else), Rugged could fail when options was nil.

Example

Before:

repo.diff(commit, nil) # => NoMethodError

After:

repo.diff(commit, {}) # Works as expected

No breaking changes. This only ensures options is never nil.

Specs

No spec changes needed

[ashkulz]

Wrt the "Why", can you point to the exact code which triggers this problem for the other cases?

[jlurena]

Wrt the "Why", can you point to the exact code which triggers this problem for the other cases?

@ashkulz

This fixes the default case:
https://github.com/lmonzon4/pronto/blob/6f87274a2fb4d563d281e2a40ee2888024864e64/lib/pronto/git/repository.rb#L31

[ashkulz]

@jlurena you said that you were able to get an error

undefined method `[]' for nil:NilClass (NoMethodError)

pronto is mostly used as a CLI, so unless this can be reproduced that way I'm not sure it makes sense to fix a potential bug due to passing a wrong argument to an internal API.

[jlurena]

@jlurena you said that you were able to get an error

undefined method `[]' for nil:NilClass (NoMethodError)

pronto is mostly used as a CLI, so unless this can be reproduced that way I'm not sure it makes sense to fix a potential bug due to passing a wrong argument to an internal API.

Yes there is an error when you clone a repository with a shallow depth.

Snippet of stacktrace of error, can't share more as its internal.

/opt/ruby/lib/ruby/gems/3.2.0/gems/rugged-1.9.0/lib/rugged/repository.rb:114:in `diff': undefined method `[]' for nil:NilClass (NoMethodError)
--
  |   |   |  
  |   |   | right.diff(left, opts.merge(:reverse => !opts[:reverse]))
  |   |   | ^^^^^^^^^^
  |   |   | from /opt/ruby/lib/ruby/gems/3.2.0/gems/pronto-0.11.4/lib/pronto/git/repository.rb:30:in `diff'
  |   |   | from /opt/ruby/lib/ruby/gems/3.2.0/gems/pronto-0.11.4/lib/pronto.rb:64:in `run'

This is an extremely minmal and safe PR, is there an issue with approving this, even if its just to standardize and improve the code itself?

@ashkulz

[ashkulz]

Ah okay, so looks like you're hitting #447. It is actually an upstream issue (libgit2/rugged#973) and a PR similar yours was closed due to it being an incomplete fix -- see #459.

I'd appreciate if you could help with detailed steps reproducing the original issue -- work stalled as I don't want to merge a workaround without understanding the impact.

[jlurena]

Ah okay, so looks like you're hitting #447. It is actually an upstream issue (libgit2/rugged#973) and a PR similar yours was closed due to it being an incomplete fix -- see #459.

I'd appreciate if you could help with detailed steps reproducing the original issue -- work stalled as I don't want to merge a workaround without understanding the impact.

For us this issue occurs during a Github Merge queue, Github creates a temporary branch (merge of target and base)

1. CI Shallow clones repository using the new temporary branch created by Github
2. Pronto is ran. Base is Merge Queue branch and the temp branch is against it.
3. There are no diffs.
4. Because there are no diff, the second argument passed in to #diff is nil

Making it be the correct data type which Rugged expects fixes this. This is fixing an edge case sure, but it is a proper fix. There is no impact to anything else.

@ashkulz hope that help. We're simply trying to contribute to Open Source community and we have validated internally, by monkey patching Pronto, that this indeed fixes the issue.

The PR is here if you change your mind.

Thanks!