Skip to content

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Sep 8, 2025
Merged

Fix keepalive in forks #15

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2e14988
keepalive: fix merging upstream
dscho Sep 4, 2025
580b58a
keepalive: force commit when triggered manually
dscho Sep 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion .github/workflows/keepalive.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ jobs:
id: merge
if: github.event.repository.fork == true
run: |
git fetch --unshallow origin &&
Copy link
Collaborator

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?

Copy link
Member Author

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.

Copy link
Collaborator

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).

Copy link
Member Author

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).

Copy link
Collaborator

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.

Copy link
Member Author

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
Loading

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 😉).

git fetch https://github.com/gitgitgadget/gitgitgadget-workflows HEAD &&
if test 0 = $(git rev-list --count HEAD..FETCH_HEAD)
then
Expand All @@ -32,7 +33,8 @@ jobs:
id: commit
if: steps.merge.outputs.result != 'merged'
run: |
if test 0 -lt $(git rev-list --count --since=3.weeks.ago HEAD)
if test workflow_dispatch != '${{ github.event_name }}' &&
test 0 -lt $(git rev-list --count --since=3.weeks.ago HEAD)
then
echo "::notice::No need to keep alive, there were commits in the last three weeks"
echo "result=skip-push" >>$GITHUB_OUTPUT
Expand Down