Issues with CL Link Validation

[timbeiko]

Proposed Change

The regex used to match CL spec links in EIP-1 does not accommodate common CL spec linking practices.

Two recent examples of issues:

1. It's not possible to refer to a PR directly, which seems useful when things are in draft (see @dapplion's EIP here: https://github.com/ethereum/EIPs/pull/7044/files#diff-b9072b226494f022f46b59068e6f9144194aeb301e57ed74dbefe9fba49ef39dR27)

2. It's not possible to refer to feature directories, which are expected to be the canonical way to specify WIP features in the CL spec (see @djrtwo's EIP here: https://github.com/ethereum/EIPs/pull/6914/files#diff-768009de73cc7d111224f20db3c7f40c7eb3836aeaac51191810754ed62b8446R27)

Proposed solutions:

1. Allow linking to PRs (https://github.com/ethereum/consensus-specs/pull/*), potentially only for Draft EIPs?
2. Allow linking to feature directories (https://github.com/ethereum/consensus-specs/specs/_features/*)

[SamWilsn]

1. Allow linking to PRs (https://github.com/ethereum/consensus-specs/pull/*), potentially only for Draft EIPs?

Would putting such a link in a TODO comment be acceptable? Something like:

Execution clients MUST floop the pig as defined in <!-- TODO: https://github.com/ethereum/EIPs/pull/7044/files#diff-b9072b226494f022f46b59068e6f9144194aeb301e57ed74dbefe9fba49ef39dR27 -->.

2. Allow linking to feature directories (https://github.com/ethereum/consensus-specs/specs/_features/*)

Not a terribly hard change, just need to modify EIP-1 and eipw to use this regex instead:

^https://(www\.)?github\.com/ethereum/consensus-specs/(blob|tree)/[a-f0-9]{40}/.+$

[g11tech]

Just explored if the PR commits hashes can be used to get a perma-link, following issues pop up

1. PR could refer multiple files in very speadout folders
2. a permalink to commit is from the source repo for e.g. https://github.com/dapplion/consensus-specs/tree/1c35eb1c3303fe1e0b101323106d766d8f848cd6 is the commit of a PR to ethereum/consensus-specs so would obviously fail current validation
3. If commit never is merged in main repo, it might get pruned out (even in merges github deletes source branch tag) and squash would anyway lead to new commit ?

[timbeiko]

Would putting such a link in a TODO comment be acceptable? Something like:

I think so, @SamWilsn, but it might be simplest to simply allow /pulls for Draft and then require a folder directory to move to Review?

[SamWilsn]

1. PR could refer multiple files in very speadout folders

Hm, yeah. I think linking to the tip commit of the pull request would be the best option.

2. a permalink to commit is from the source repo for e.g. https://github.com/dapplion/consensus-specs/tree/1c35eb1c3303fe1e0b101323106d766d8f848cd6 is the commit of a PR to ethereum/consensus-specs so would obviously fail current validation

https://github.com/ethereum/consensus-specs/tree/1c35eb1c3303fe1e0b101323106d766d8f848cd6 seems to work for me. I believe GitHub adds commits from PRs to the destination repository under a weird ref.

3. If commit never is merged in main repo, it might get pruned out (even in merges github deletes source branch tag) and squash would anyway lead to new commit ?

That's a great point. Hell, if you force push to your default branch, you can completely remove the linked commit. That's a whole other problem we should think about...

[g11tech]

Thanks to the input from @SamWilsn that the main repo to which PR has been made has the commit, we have come to consensus on the EIP editors call on May 31 that we can allow compare links in regex

ethereum/consensus-specs@53a9324...1c35eb1
(https://github.com/ethereum/consensus-specs/compare/53a9324cf0afd90ea06c5ebb0bfef8fc95a01fd1...1c35eb1c3303fe1e0b101323106d766d8f848cd6)

which basically generates the PR diff being refereed to

UPDATE: github only generates diff for the commit not the full chain, so the EIP author will have to squash all the relevant PR commits into 1 commit which imo should be an ok requirement

UPDATE: two .. instead of ... show full diff (so we regex for 2 ... or 3 ...)
https://github.com/ethereum/consensus-specs/compare/53a9324cf0afd90ea06c5ebb0bfef8fc95a01fd1..1c35eb1c3303fe1e0b101323106d766d8f848cd6 but then it also includes the diffs introduced by first commit id: 53a9324cf0afd90ea06c5ebb0bfef8fc95a01fd1 so one will have to generate the link point to the repo's commit when PR was generated (or rebase their PR to bring their commit upto date and then generate the link)

[g11tech]

I think this https://github.com/ethereum/consensus-specs/compare/1c35eb1c3303fe1e0b101323106d766d8f848cd6~3...1c35eb1c3303fe1e0b101323106d766d8f848cd6 (ethereum/consensus-specs@1c35eb1~3...1c35eb1) this sort of diff would be best to include: which says last 3 commits including and preceding 96d29b7662f148842486d46117786ccb7fcc8018

[Pandapip1]

https://github.com/ethereum/consensus-specs/tree/1c35eb1c3303fe1e0b101323106d766d8f848cd6 seems to work for me. I believe GitHub adds commits from PRs to the destination repository under a weird ref.

Specifically, it's that the commits and objects from forks are stored in the parent repository, because GitHub logic.

[github-actions]

There has been no activity on this issue for 1 week. It will be closed after 3 months of inactivity.

[github-actions]

This issue was closed due to inactivity. If you are still pursuing it, feel free to reopen it and respond to any feedback.