Use triple-dot diff YAML in CI

[cottsay]

The current diff mechanism in rosdep_repo_check uses a 'bare' diff with the configured upstream ref. If the upstream ref contains additional commits which are not present in the PR's HEAD which remove lines of YAML, this bare diff will report those lines as 'added' when comparing the PR to the target, which is certainly not what we want to do.

We can use the git triple-dot notation to compare only commits which are present in HEAD but not in target, effectively comparing between the PR's HEAD and the merge base with the target branch, instead of the target branch directly.

One disadvantage of this approach is that uncommitted changes are no longer considered part of the diff. For this reason, I'm only adding the triple dot during diffs with the specifically-configured upstream branch and not the 'fallback' comparison with origin/master. This way, you can still invoke the test locally without the need to commit the changes first.

Fixes part of #31439

[cottsay]

Oh lame. I'm pretty sure this doesn't work because we're using a shallow clone for "upstream". Without the full history, we can't compute the merge base to figure out what changes are present in what trees, so the best we can do is continue to diff the blobs without considering history. I don't think we have the information we need to answer the question we're asking here, at least not without changing some of the more core CI setup.

We're pretty backed into a corner here. I'm inclined to leave the existing code as-is and address this more directly in the next iteration of the rosdep_repo_check.

[nuclearsandwich]

One disadvantage of this approach is that uncommitted changes are no longer considered part of the diff. For this reason, I'm only adding the triple dot during diffs with the specifically-configured upstream branch and not the 'fallback' comparison with origin/master. This way, you can still invoke the test locally without the need to commit the changes first.

This seems like a good baseline heuristic to me.

I'm pretty sure this doesn't work because we're using a shallow clone for "upstream". Without the full history, we can't compute the merge base to figure out what changes are present in what trees, so the best we can do is continue to diff the blobs without considering history.

We could get a bit over-engineer-y and do repeated shallow clones (depth = 8, depth = 16, depth = 32, ..) until we find a merge base

There are also other tools to limit what we actually need to fetch until just-in-time: https://github.blog/2020-12-21-get-up-to-speed-with-partial-clone-and-shallow-clone/

That being said, an initial clone of ros/rosdistro outright is 100MiB which at 30MiB/s is ~3.3s of GitHub's bandwidth and energy. I think the python setup for the test takes 10x that time. I'd be fine with a non-shallow clone.

[nuclearsandwich]

We're pretty backed into a corner here. I'm inclined to leave the existing code as-is and address this more directly in the next iteration of the rosdep_repo_check.

What do you have in mind that changes things in a future iteration?

[github-actions]

This PR hasn't been activity in 14 days. If you are still are interested in getting it merged please provide an update. Otherwise it will likely be closed by a rosdistro maintainer following our contributing policy. It's been labeled "stale" for visibility to the maintainers. If this label isn't appropriate, you can ask a maintainer to remove the label and add the 'persistent' label.

[nuclearsandwich]

Outstanding questions don't actually block merging this improvement.

[cottsay]

Outstanding questions don't actually block merging this improvement.

I'll need to switch away from the shallow clone for this to work. I need to get an idea of how much that will cost.

[mjcarroll]

@cottsay is this still relevant with the new bot?

[cottsay]

is this still relevant with the new bot?

The new automation Does This Right. I'd like to keep this open for just a LITTLE while longer until I finish moving more of the existing pytest checks into rosdistro-reviewer.