-
Notifications
You must be signed in to change notification settings - Fork 4
Fix keepalive in forks #15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix keepalive in forks #15
Conversation
When upstream truly diverged from the downstream repository, a merge base needs to be determined, otherwise the merge will fail with: fatal: refusing to merge unrelated histories Unfortunately, merge base calculation will also fail when one of the branches is _shallow_, in which case the error message is actually highly misleading. Let's just ensure that we do not have a shallow clone when running in a fork. There would be an opportunity to make this logic smarter, by using the GitHub REST API to check whether there _are_ upstream commits that aren't reachable from the downstream HEAD, but https://xkcd.com/1205/... Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
When issuing a workflow dispatch, the intention is to keep the repository alive, i.e. a commit _must_ be made, one way or another. Let's side-step the 3-weeks logic in that scenario. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
id: merge | ||
if: github.event.repository.fork == true | ||
run: | | ||
git fetch --unshallow origin && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First, can this be done through the checkout action?
Second, the next line fetches from the upstream which may contain new updates that are not the keep alive. Would this possibly pull in changes unexpectedly? Sorry if I have misunderstood.
Would it work to just have the keep alive on a separate branch that never gets merged?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the checkout
step could get something like:
with:
depth: ${{ github.event.repository.fork != true && 1 || 0 }}
Honestly, I find the --unshallow
approach more obvious.
And yes, this can drag in new upstream changes. But isn't that a good thing? Relying on gitgitgadget/gitgitgadget@v1
shares that trait...
It might count as activity if a branch different than the main branch is pushed. The documentation is not terribly clear about that. Judging by the fact that workflow dispatches do not count as activity, I seriously doubt it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I was going to suggest only doing the unshallow if a merge is needed. More steps but the unshallow will slowly get bigger over time so only doing if necessary could help.
I can see confusion when anyone (especially for non-git users of ggg) makes a config change and finds out a merge (or rebase) is needed due to keep alive being pushed.
Not running workflows due to no activity has a purpose. On the other hand there can be workflows that would create activity based on external data the workflow examined. This case is more unusual since the workflows are running to handle activity in other repos.
Just thinking about order of execution where the fork could consistently run before the upstream. It would create a commit and then do a merge the next time it ran to pull in the upstream keepalive commit. What if the commit was only done with the upstream and the fork merges if there are local changes and pushes the upstream changes to the fork. Still trying to this clear in my head.
I am okay with implementing this as is. I am just concerned there could be unexpected merging (fork has keep alive but we want to pull updates from upstream without waiting).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to suggest only doing the unshallow if a merge is needed.
The unshallow is needed to determine whether a merge is needed :-)
Actually, we could use the repos/.../compare
GitHub REST API endpoint which reports ahead_by
/behind_by
numbers. That could also inform the workflow about how much the depth has to be increased.
But that feels utterly over-engineered.
How about simply using depth: 150
always, no matter whether in a fork or not? That should be good enough for merging, be reasonably fast, and first and foremost: simple.
Not running workflows due to no activity has a purpose.
Right. And it is worth mentioning that when you fork, the keepalive
workflow is disabled by default, thanks to being a scheduled workflow.
And yes, the ideal case is a fast-forward in keepalive
, and I do want to encourage that forks keep their changes in a shape that they can be upstreamed so that eventually they are in sync with upstream again (as in: identical tip commit).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The unshallow is needed to determine whether a merge is needed
If HEAD does not match FETCH_HEAD a merge is needed. Which is really refs/head/main compared to FETCH_HEAD. BUT the upstream fetch is not going to be limited if HEAD is not part of the upstream history. Just thinking out loud convincing myself the unshallow is needed (either explicitly or through the checkout).
How about simply using
depth: 150
always
I think we can go with the current setup for now (KISS). If there is no extra activity, it could be years before that depth is reached.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gitGraph
commit
commit
branch downstream
commit
commit
If HEAD
does not match FETCH_HEAD
, it is possible that a merge cannot be done because FETCH_HEAD
(in this diagram, main
) is an ancestor of HEAD
(in this diagram, downstream
). But an unshallow is needed to be able to determine that, otherwise HEAD^
won't be present and the revision walk will end there, claiming unrelated histories (which is what happened in my fork, triggering my work on this here PR 😉).
In the
cygwingitgadget
fork, which I use as a test-bed for developing the modifications required to let GitGitGadget support projects other than Git, a scheduled run of thekeepalive
workflow failed.The reason is that it needs to determine the merge base between upstream and downstream tips, and that requires non-shallow commit histories. Let's ensure that.
While at it, also fix the design wart that the
keepalive
workflow, when triggered manually, would still not produce any commit if the latest change is younger than 3 weeks. I mean, if it is triggered manually, there usually is a need for that, so let's stop the workflow from trying to be smarter than its human handler.