fix: Improve error logging for push events without a before sha (like tag pushes)

[sheck]

When a push event is sent for a tag, the before SHA is set to 0000000000000000000000000000000000000000 — which is not something we can run a diff against.

We were trying to pass it to rubocop, where it would silenty fail as rubocop fatal: bad object 0000000000000000000000000000000000000000

So let's just skip trying to run rubocop in those instances.

---

• To see the specific tasks where the Asana app for GitHub is being used, see below:
  • https://app.asana.com/0/0/1203239803057349

[danielma]

Thanks for figuring this one out. It's a good change. My question is definitely not a blocker, but I would love your thoughts on whether it's worth trying to make this sha validation a little more resilient into the future?

[danielma]

This kind of exact check on a magic value feels risky to me. Like, what if the previous sha is nil or, an empty string, or any other value?

What would you think about opening up this condition, or maybe even reversing them?

Option 1 (something more open):

if previous_sha.chars.uniq == ["0"]

Option 2 (broader validation check)

if previous_sha.length == 40 && previous_sha.chars.uniq.many?
  generate_annotations(compare_sha: previous_sha)
else
  ActionUtils.debug "Skipping check run since we don't have a valid sha to compare #{previous_sha.inspect}"
  []
end

[molawson]

I was feeling a similar thing about the relative riskiness of this check.

I quickly started zooming out a bit to how this particular problem might show up in other places.

Do we want to silently return no annotations in this scenario or do we want to throw an error? This seems like an exceptional circumstance where use an error to indicate the issue (maybe even checking the presence of the sha with git cat-file -e {sha}?) seems reasonable.

That codifies the underlying issue, and then we can prevent the issue in our repo (and same for other folks using this action) by filtering out problematic pushes?

In general, does saying something went just fine when we didn't do any of the work feel potentially misleading?

(if this broader conversation needs to wait in order to get a quick fix out, I'm good with that too)

[sheck]

These are all great points. Here's what I'm thinking:

1. Validations could be wider, I'll work on that
2. It does feel weird that it would be listed the same as a successful check when this happens. I'll look into seeing what we can do here to change that.