Cleanup: allow analysis of merge commits

[jayaddison]

Follows updates to CodeQL integration guidance per github/codeql-action#297

[sigmavirus24]

None of this explains why. Can you share why we would or why it didn't used to suggest this and nwo does?

[jayaddison]

In short, it's that the CodeQL analysis action has been updated to detect and analyze the content changes applied during merge commits.

Previously after a pull request was merged, the CodeQL action would have naively analyzed the HEAD (merge) commit, which in itself does not include content modifications. What a merge commit does have, however, are merged commit references. For a merged pull request, the first merge reference would be the parent (previous) commit, and the second merge reference (HEAD^2) would include the contents of the pull request.

(probably a few technical inaccuracies there, but that's roughly as I understand it currently)

All things being equal, a simpler and easier to read configuration seems better, especially if it matches the recommended guidance. Your question's given me pause though, and so I'm digging through the CodeQL action itself to see exactly how it detects and handles merge commits.

[jayaddison]

Yep - going by the plugin's source code, since the end-of-November release of codeql-action, use of git checkout HEAD^2 isn't required or encouraged any longer. That doesn't mean we shouldn't still potentially be skeptical of the claim, but it's the rationale for the change here.

[bhrutledge]

@sigmavirus24 Any more comments on this? Is it okay to merge?

[sigmavirus24]

No other comments or thoughts

[jayaddison]

Some retrospection about this pull request:

I answered more of the what about this change than the why. It's true that this was newly available CodeQL functionality, and that it had become a recommended practice, but that didn't directly address the question, so my apologies.

In git history where commits are linked linearly (without merge commits), then I don't think that the additional git clone overhead implied by the CodeQL action config change here would be necessary; so that could have been one reason that Twine did not use it (the history in this repository appears mostly linear).

On the CodeQL side: that tooling may have begun analyzing merge commits because those commits can include previously-unseen changes introduced during conflict resolution.

In other words: although from a cursory (automated or human) inspection, a merge commit may appear to simply combine together two existing, known components -- and, often, that is indeed the sum total of what it does -- sometimes a merge commit can also include a conflict resolution edit (a change that may be less-well-reviewed and/or accident-prone -- and worth analyzing).

I don't know whether that's helpful context, especially relatively long after-the-fact, but it's an attempt to improve upon my previous responses.