Skip to content
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

feat: Use work trees instead of copying the whole directory #553

Merged
merged 1 commit into from
Sep 3, 2024

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Aug 31, 2024

This should speed up backports by not copying the repository but create a clean work tree with the required branches set.

Best reviewed without whitespace: https://github.com/nextcloud/backportbot/pull/553/files?diff=split&w=1

@susnux susnux added the enhancement New feature or request label Aug 31, 2024
@susnux susnux requested a review from skjnldsv August 31, 2024 20:40
// create worktree
const git = simpleGit(cachedRepoRoot)
// fetch upstream version of the branch - well we need to fetch all because we do not know where the commits are located we need to cherry-pick
await git.fetch(['-p', '--all'])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can cause race conditions if ran on the cache directory.
I've had this happen before.

Multiple runs can happen at the same time ⚠️

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multiple runs can happen at the same time ⚠️

How could this happen? There is a concurrency of 1 in queue.ts

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I honestly don't even remember 🙈
You can see in the old code I was keeping the cache updated initially, but that created issues 🤷


let queue: PQueue

export const addToQueue = (task: Task): Promise<void> => {
export async function addToQueue(task: Task): Promise<void> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tssss 😁

src/index.ts Outdated Show resolved Hide resolved
@skjnldsv
Copy link
Member

skjnldsv commented Sep 3, 2024

Did you encounter some slow backports @susnux ?
What made you start this PR?

@susnux
Copy link
Contributor Author

susnux commented Sep 3, 2024

What made you start this PR?

The recent backport bot issues with dirty repository. Using worktrees will never touch the "original" repository, moreover it does not require to copy all ~4GB of the .git directory.

This should speed up backports by not copying the repository but create a clean work tree with the required branches set.

Signed-off-by: Ferdinand Thiessen <[email protected]>
@skjnldsv
Copy link
Member

skjnldsv commented Sep 3, 2024

The recent backport bot issues with dirty repository. Using worktrees will never touch the "original" repository

The original cached repository was never touched before. :)
The issue was different.

Everything runs on ssd, copying 4GB takes barely a few seconds.
I'm not against this, but I'm too unfamiliar with the git worktree 😬

@susnux
Copy link
Contributor Author

susnux commented Sep 3, 2024

Yeah saw that, but then I already starting using worktrees and I thought maybe you consider this a nice to have (not sure on what hardware this runs but probably copying 4GB takes always quite some time 😅
But feel free to dismiss this PR if you do not think so - would be ok for me too^^

@skjnldsv
Copy link
Member

skjnldsv commented Sep 3, 2024

I would rather merge it and deploy it on a day you and i are both ready to focus on it in case something needs adjusting if that's ok 👍

@skjnldsv skjnldsv merged commit 48e0945 into master Sep 3, 2024
8 checks passed
@skjnldsv skjnldsv deleted the feat/use-work-trees branch September 3, 2024 12:06
@susnux
Copy link
Contributor Author

susnux commented Sep 3, 2024

I would rather merge it and deploy it on a day you and i are both ready to focus on it in case something needs adjusting if that's ok 👍

Conference?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants