-
-
Notifications
You must be signed in to change notification settings - Fork 59
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
Recommend a procedure for branch management #475
Comments
Additional points were discussed but omitted for privacy. |
ProblemsI'll try to elaborate on the problems I see with the forks-of-forks methodology: lack of guidance, divergent approachesAlthough some developers have an intuition for how to manage changes from a private fork, it's also a convention. There's no default practice to follow, as is evidenced by a commiter prefacing their workflow with "My local checkout...". Especially if it's a recommended procedure, it would be nice if the developer documentation provided explicit guidance on a methodology that one could follow. It would include terminology ("origin", "upstream") that could be re-used in discussions to reason about it. A developer could adopt a different workflow, but at least someone following the guidance wouldn't have to make these decisions about concerns that haven't affected them. dependent PR trackingGithub has a feature that allows one PR to target the branch of another PR on which it depends, and when that PR is merged, the dependent PR is re-targeted to the merge target of the original PR, allowing for the creation of series of changes that depend on each other but which should be merged separately. This feature is particularly valuable when introducing a deprecation, where the deprecation step and the removal step are necessarily separated by time between releases. It's not obvious if Github supports repointing a PR from one org to another based on dependencies. In fact, I'm not even sure that would make sense, because the dependent PR couldn't be created in the origin project.
|
If we're following the KISS principle, the simplest solution is one with the least proliferation of forks, where everyone shares the same remote and there's one remote repo to reason about. For someone just getting familiar with version control, a single remote is the only workflow. For smaller projects with a few developers, pushing to a single repo is the norm. Even some large companies have a single monorepo with branches right off that repo. The Mercurial project uses Heptapod and all contributors push branches direct to the main repo (no private forks). I understand that for CPython, and many large Github projects, the private fork (fork of forks) model is the norm, but that doesn't mean it has to be the case.
This is demonstrably false. As observed in python/cpython#94528, when merging the PR on which that one depended, it automatically repointed the target: I stumbled on this feature earlier this year and was pleased to see that it was a feature that Github was working on. Here's a blog describing in detail how one might use it. That blog and other resources acknowledge that the functionality exposes some ambiguity and has room for improvement. It may be the case that this feature only works if the developer is pushing branches to the canonical repo. If the methodology that CPython uses prohibits or discourages this type of workflow, it's a major disadvantage to everything but the most simple contributions. |
To better understand the motivation for a private-fork recommendation, I'd still like to see answers to these questions:
In particular, what is the harm if a handful of developers push direct to the canonical repo? What is the harm if everyone does it? |
The harm is that it's noise. When you clone the repo, you're not getting any of my in-flight changes because I keep them in my fork. Unless you go and look for an open PR, those changes don't get in the way of your own work. They do get in the way because now you're sharing a flat namespace for branches with random other developers. There is no convention for naming PR branches and even if we put one in the devguide, automation will ignore it, the Github UI will ignore it, and -- guaranteed! -- some core developers will ignore it. All branches on the main repository for a project with >100 committers look official and long-living. This is not true for branches only created out of convenience by a handful of core developers. They aren't official, they aren't long-living projects, as we require PRs for every change (besides some release management shenanigans). There is already a pre-existing recommendation against pushing to the main repository: As such, this issue here isn't about choosing a recommendation, it's about whether we want to change the existing recommendation to allow for branches littering the main repository. I'm -1 to such a change. |
Thanks for this reference. I'd missed it when explicitly looking for it. It may be worth linking this recommendation from other locations so it's more prominent. Thanks also for articulating the harm. That's helpful for making tradeoffs between that and other concerns.
It feels to me like you're weighting the concern of noise far greater than any of the concerns raised above (or discounting them entirely), including concerns that have no feasible solution in a fork-only scenario. In one sentence, you've declared "wontfix" on a whole suite of problems. Was that your intention? |
Do you want to get all these branches on your local computer? And there are 20-30 other active core developers. |
To summarize: GitHub sucks for serious development practices. We knew that going in. It still beat what we had by a long shot and enabled new contributors - https://peps.python.org/pep-0512/. An issue isn't a good place for these conversations. Use discuss.python.org. This narrow scope issue looks to me like one merely to clarify our existing defacto "don't push branches on purpose" community standard in whatever committer dev guide docs we have if needed. Nobody is debating that GH lacks productivity features many of us take for granted in professional environments. But this isn't the place for it. That can't be planned enumerated and resolved in an issue tracker. |
Also: you quoted people from a private chat. Sure, you later edited that to "remove" the content. But it was already too late - it was sent out in emails and remained available in the edit history on the comment. I've deleted the old revisions of the comment edit history that I found as GH happens to let me. (But all of that has been sent via email to anyone watching the full firehose of the repo or live scraping). In the future please don't paste conversation snippets from a private forum into another access level one without explicit permission from everyone first. |
These are documented and used throughout the docs:
It would be interesting to figure out how to handle such workflow and document it in the devguide. Locally you would usually create a branch
There is a related discussion here: python/bedevere#461 (comment)
I haven't found anything official in the GitHub docs (there is an article about what happens to the forks when upstream is deleted, but not what happens to PRs when the fork is deleted. I give it a try in zware/test#7 and even though deleting my fork closed the PR, the changes still seem visible and there is a "Reopen" button.
If they allow maintainers to push changes to the branch, then this might not be an issue (even though I'm not sure if it's possible after the fork is deleted). I guess another option would be to create a new branch off of the one created by the OP.
Other than the annoyance this might cause, this is generally not an issue. Locally I pull
This used to happened to me too occasionally, but not anymore. When you push the branch to your fork, it shows you a link that you can follow to create the PR upstream. Alternatively you can go to the main page of the upstream repo, and a green button at the top will let you create a PR from the branch you just pushed to the fork. Both these solutions already have the target branch set automatically to the upstream
This is true, but expected. I work on a number of projects, some of which I have access to (and I can push branches/commit to |
Definitely not on a Github issue tracker 😂 |
CI resources are limited per org or user account where the repo is (e.g. 20 concurrent jobs). If everyone pushed branches directly to the upstream repo, the CI would get backed up and it would take longer to get feedback from tests, for them and all other PR authors. |
Thanks for all the tips. I'm working on migrating my workflows to align with the recommendations. One thing I noticed when following the current guidance is that the main branch doesn't track upstream, but instead tracks origin, so
I think there's a step missing to set
|
I may or may not follow up on chained PRs. And the documentation is actually quite complete. I'm unsure where I got the impression that the documentation did not capture some basic guidance on branching strategy. |
* Update docs to configure 'main' to pull from 'upstream'. Ref python/core-workflow#475. * Quote 'main'. Co-authored-by: Ezio Melotti <[email protected]> * Direct 'upstream' push to the user's fork. Now pull comes from upstream and push goes to origin. * Apply the changes also to the git boot camp. * Prefer https for upstream fetch. Co-authored-by: C.A.M. Gerlach <[email protected]> * Update expectations and sync guidance for d7e9065. Remove superfluous .git from https. Co-authored-by: Ezio Melotti <[email protected]> Co-authored-by: C.A.M. Gerlach <[email protected]>
* Update docs to configure 'main' to pull from 'upstream'. Ref python/core-workflow#475. * Quote 'main'. Co-authored-by: Ezio Melotti <[email protected]> * Direct 'upstream' push to the user's fork. Now pull comes from upstream and push goes to origin. * Apply the changes also to the git boot camp. * Prefer https for upstream fetch. Co-authored-by: C.A.M. Gerlach <[email protected]> * Update expectations and sync guidance for d7e9065cbf37. Remove superfluous .git from https. Co-authored-by: Ezio Melotti <[email protected]> Co-authored-by: C.A.M. Gerlach <[email protected]>
I discord, @ambv made a simple request:
And at face value, it sounds like a small request to tweak one's approach in a single project, when in fact the situation is much more complicated.
@jaraco responded
This is that issue.
The text was updated successfully, but these errors were encountered: