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

Prevent pruning remotes with branches available locally #11266

Merged
merged 5 commits into from
Dec 22, 2020

Conversation

sergiou87
Copy link
Member

@sergiou87 sergiou87 commented Dec 21, 2020

Description

When a pull request from a fork is checked out, a remote prefixed with github-desktop- is added locally for the PR's branch. To prevent a clutter of remotes, the app will periodically prune old remotes from forks that are, apparently, not used anymore: if all PRs from a given remote have been closed, that remote will be removed.

However, it's possible that one of those remotes doesn't have any open PR, but branches from them are still available locally. In that case, the remote should be preserved.

This PR makes sure remotes from forked repos are not removed if a branch is available locally. It also adds some unit tests for the function in charge of deciding which remotes must be pruned.

It's important to know that local branches from merged PRs are also removed if they haven't been checked out in more than 2 weeks.

Release notes

Notes: [Fixed] Remotes referencing forked repos would be removed even when branches from them were available locally

sergiou87 and others added 2 commits December 18, 2020 13:21
Co-Authored-By: Markus Olsson <634063+niik@users.noreply.github.com>
@niik niik added this to Awaiting Review in Desktop 2.6.2 release Dec 21, 2020
@sergiou87
Copy link
Member Author

The work here should be done, but @niik found an issue with allBranches from GitStore: it doesn't actually have all branches, because getBranches from for-each-ref.ts filters out branches from forked repos:

if (ref.startsWith(ForksReferencesPrefix)) {
// hide refs from our known remotes as these are considered plumbing
// and can add noise to everywhere in the user interface where we
// display branches as forks will likely contain duplicates of the same
// ref names
continue
}

I'll leave this as a draft until that is fixed, @niik is working on it! ❤️

@niik
Copy link
Member

niik commented Dec 21, 2020

I'll leave this as a draft until that is fixed, @niik is working on it! ❤️

#11267

@niik niik marked this pull request as ready for review December 22, 2020 11:26
@niik niik self-assigned this Dec 22, 2020
Desktop 2.6.2 release automation moved this from Awaiting Review to Review In Progress Dec 22, 2020
Copy link
Member

@niik niik left a comment

Choose a reason for hiding this comment

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

Just a few small requests here but overall this looks great!

app/src/lib/stores/git-store.ts Outdated Show resolved Hide resolved
app/src/lib/stores/git-store.ts Outdated Show resolved Hide resolved
app/src/lib/stores/helpers/find-forked-remotes-to-prune.ts Outdated Show resolved Hide resolved
app/test/unit/find-forked-remotes-to-prune-test.ts Outdated Show resolved Hide resolved
app/test/unit/find-forked-remotes-to-prune-test.ts Outdated Show resolved Hide resolved
sergiou87 and others added 2 commits December 22, 2020 16:51
Co-Authored-By: Markus Olsson <634063+niik@users.noreply.github.com>
Co-Authored-By: Markus Olsson <634063+niik@users.noreply.github.com>
Copy link
Member

@niik niik left a comment

Choose a reason for hiding this comment

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

✨ Thanks for picking this up!

@sergiou87 sergiou87 merged commit a81615a into development Dec 22, 2020
Desktop 2.6.2 release automation moved this from Review In Progress to PRs For Next Beta Dec 22, 2020
@sergiou87 sergiou87 deleted the prune-actually-unused-remotes branch December 22, 2020 16:22
@billygriffin billygriffin moved this from PRs For Next Beta to Done! in Desktop 2.6.2 release Jan 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants