isaacs / github

Just a place to track issues and feature requests that I have for github
2.21k stars 128 forks source link

Mark pull request as depending on another #959

Open OliverJAsh opened 7 years ago

OliverJAsh commented 7 years ago

Quite often I find myself raising a pull request that contains another within it as a dependency. I want the diff to be exclusive of the dependency pull request’s changes, otherwise my colleagues end up reviewing more than they need to, or worse, the same changes twice (in both pull requests).

To workaround this, I usually set the base of my pull request to the dependency pull request, however this has caveats:

I would like to see some UI in GitHub to make this workflow first class:

cirosantilli commented 7 years ago

related https://github.com/isaacs/github/issues/210

phil-davis commented 7 years ago

It would be nice if the branch to which the PR is targeted even automagically updated: 1) PR branch my-new-infrastructure-thing is awaiting review and merging to master. 2) PR branch my-new-ui-thing is made against my-new-infrastructure-thing (because it makes use of some new infrastructure in my-new-infrastructure-thing) 3) Diff of PR my-new-ui-thing just shows the diffs to my-new-infrastructure-thing 4) When my-new-infrastructure-thing is merged to master, PR for my-new-ui-thing automagically switches to being a PR against master. (and mostly there will be no conflicts to resolve, but there could be because other stuff might have happened in master also that causes conflicts - those would be resolved by the person who "owns" my-new-ui-thing, like they would for any conflict situation)

aspiers commented 6 years ago

See also #805 (can't move base branch of PR to a different fork).

gelldur commented 6 years ago

Also after merging branch cache still shows changes from dependent branch

dstanek commented 6 years ago

This is a feature that I really miss from Gerrit. It's more common for me to have pull requests that depend on other pull requests than to have isolated pull requests.

courtney-miles commented 6 years ago

When my-new-infrastructure-thing is merged to master, PR for my-new-ui-thing automagically switches to being a PR against master.

This is the workflow I imagined also. The dependant PR should follow the merge of its dependency.

brauner commented 6 years ago

Fwiw, I had requested something similar directly from Github in 2016:

From christian.brauner@mailbox.org Wed Feb  3 14:02:38 2016
Date: Wed, 3 Feb 2016 14:02:38 +0100
From: Christian Brauner <christian.brauner@mailbox.org>
To: support@github.com
Subject: Incremental pull requests
Message-ID: <20160203130238.GA27328@mailbox.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
User-Agent: Mutt/1.5.24 (2015-08-30)

Hello,

I have a feature request/suggestion/question. A workflow that happens rather
often is to have multiple but not-thematically connected commits but which
nonetheless are incremental, i.e.

        Commit 1:
                - Implement **A** by making changes to file **F**
        Commit 2:
                - Implement **B** by making changes to file **F**

Where the features **A** and **B** are not thematically connected but make
changes to the same file and functions therin and are thus incremental (i.e.
build upon each other).

Let's assume I implement feature **A** first. Then I create a new branch

        git branch implement_feature_**A** origin/master

based on the current master. But for the implementation of feature **B** I would
logically use origin/implement_feature_**A** as base

        git branch implement_feature_**B** origin/implement_feature_**A**

Is there a way to group these PRs together such that it is obvious for other
collaborators that they should be merged incrementally, i.e. first merge

        implement_feature_**A**

and only then merge

        implement_feature_**B**

?

Otherwise they could always just merge

        implement_feature_**B**

and the PR for

        implement_feature_**A**

becomes rather pointless + this way neglects the fact feature **A** and **B**
are just incremental but thematically unrelated.

Best,
Christian

They told me that they'd consider it:

Date: Tue, 09 Feb 2016 04:52:55 -0800
From: "Petros (GitHub Staff)" <support@github.com>
To: Christian Brauner <christian.brauner@mailbox.org>
Message-ID: <discussions/73b2738eca7611e58e1586af4be1788f/comments/2614357@github.com>
In-Reply-To: <20160203130238.GA27328@mailbox.org>
References: <20160203130238.GA27328@mailbox.org>
Subject: Re: Incremental pull requests
Mime-Version: 1.0
Content-Type: text/plain;
 charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: list
X-SG-EID: /isQyw1HWYOYdYDPy2G3B3zgegQeGDLWVRLJ7aoZ5uDHe0Js9MIhxWsZqnOheP5i+al8M60D37ZiLY
 5SddaxWW+FNYaG5PkvackH0fkAZe5afVFP4jCsqJB8ea/RmszlNd0mJq4qFA8yV4FKAX0jbkzV8vom
 3Dfb1nCfmOCEdYRQtnT6DdFtY0jlys6BwwBO6u12YIzcENYjHB0KbK9BK6Qi3IHc2j9UPJ18IoEzuq
 p8tsl0ud7oezPxxmCSdYg+

Hi Christian!

Thanks for writing in!

> Is there a way to group these PRs together such that it is obvious for other
> collaborators that they should be merged incrementally, i.e. first merge

Not currently. You could use labels, group them together by using a prefix in the title, and as always, communicate your intentions in the pull request body by mentioning your team(s) or other collaborators.

Your feedback is interesting, so I have shared it with our team to consider.

Cheers,
Petros
aspiers commented 6 years ago

Duplicate of #950 and #867, although this one has more interesting comments than those.

MV10 commented 5 years ago

This is especially annoying when the repo you're contributing to can take weeks to review / merge, and devs are left having to somehow manually keep track of the follow-up changes instead of getting completed work into the pipeline and move on with their life.

wxianxin commented 5 years ago

Is it normal that an issue has been open for over a year?

dgw commented 5 years ago

This is just an unofficial "ideas" repo. GitHub doesn't officially follow these issues.

amcsi commented 5 years ago

GitHub should make it so that if a dependency PR was merged, all dependent PRs that had that merged PR's branch as a base should automatically have their base branches changed to that of the merged PR.

This way it would become really straight-forward to merge PRs in a bottom to top approach (merging PRs closer to develop first).

aspiers commented 5 years ago

@amcsi commented on November 29, 2018 11:07 AM:

GitHub should make it so that if a dependency PR was merged, all dependent PRs that had that merged PR's branch as a base should automatically have their base branches changed to that of the merged PR.

This way it would become really straight-forward to merge PRs in a bottom to top approach (merging PRs closer to develop first).

Agreed. The need for this auto-rebase (or at least auto-change-base) feature was also noted in #950.

OliverJAsh commented 5 years ago

These might help:

pmalekn commented 5 years ago

So is there any chance that we're getting this What's the status of this? I assume that if the issue is still "Open" it might get implemented.

karlhorky commented 4 years ago

Wonder if this "early next year for stacked diffs" tweet by Nat has anything to do with this discussion: https://twitter.com/natfriedman/status/1170804894241972224

tylerjw commented 4 years ago

I'm just here to say that after working on a project with gerrit I'm really struggling to use github because of the lack of this feature.

kylehovey commented 4 years ago

I'm currently six PR's deep into a dependency chain, so I'll chime in here too. This feature would be slick.

lhanson commented 4 years ago

GitLab has this :eyes:.

fankaidev commented 4 years ago

@lhanson how to do this in Gitlab ?

lhanson commented 4 years ago

@fankaidev https://docs.gitlab.com/ee/user/project/merge_requests/merge_request_dependencies.html

acoulon99 commented 4 years ago

Raising my hand for this one as well

marc-h38 commented 4 years ago

I think we're unlikely to ever get this, or at least never any "first class citizen" version of this because github doesn't really support rebase & force-push, it's primary review model is "Fixup and Squash" instead.

To workaround this, I usually set the base of my pull request to the dependency pull request, however this has caveats:

  1. ...
  2. ...
  1. After addressing review comments on the first request, you must rebase and force push the second request.

Raising my hand for this one as well

No, you're just adding an empty comment. Imagine if the 400+ people who thumbed this up did the same.

amcsi commented 4 years ago

I think we're unlikely to ever get this, or at least never any "first class citizen" version of this because github doesn't really support rebase & force-push, it's primary review model is "Fixup and Squash" instead.

We're not talking about rebasing and squashing here. This is about plain old fashioned proper merges where no force push happens.

dantman commented 4 years ago
  1. After addressing review comments on the first request, you must rebase and force push the second request.

Rebase is not necessary when one PR branch is based on another. When the first branch is updated those changes can optionally be merged into the second branch. The Git diff toolset is already capable of handling this. And GitHub's PR merging and merge conflict handling is already capable of handling the scenario where PR 2 branches off PR 1, then PR 1 gets another commit, PR 1 gets merged, and PR 2 hasn't merged the later commits. The only thing missing in this scenario are: GitHub's diff for PR 2 being based off PR 1 instead of the base branch; and PR 2 having something that either requires PR 1 to be merged first or tells you that a merge will also merge PR 1 and auto-closes PR 1 as merged if you force it to merge PR 2.

dantman commented 4 years ago

This is what a Git history graph where you have one branch based off another and merge them in in order looks like.

Dependent PR merge

This is that same scenario but if there was a "dependent PRs must merge in master after the PR they depend on is merged" policy to account for things a CI system may need to check. Note that this extra merge could be done automatically by GitHub if there are no merge conflicts.

Dependent PR merge with merge back
marc-h38 commented 4 years ago

Thanks Daniel, the graphs are especially useful. The review model they assume is even more restrictive than what github supports, it's a "fixup and NO squash" review model where you don't change commits themselves and don't care about leaving "dirty" commits in the main history of the project and don't rewrite them.

A "dirty" commit is one with code that a reviewer asked you to change. See this explanation of a "clean" history by the person who invented git: https://www.mail-archive.com/dri-devel@lists.sourceforge.net/msg39091.html

The graphs also assume AA, AAA, don't conflict with anything in PR-2 which I find relatively frequent in practice. Take for instance PR-1 being a pure refactoring that a reviewer asks you to refactor slightly differently.

In summary this feature request is useful but only in a restrictive "fixup and NO squash" review model, thanks a lot for helping clarify this.

dantman commented 4 years ago

The graphs also assume AA, AAA, don't conflict with anything in PR-2 which I find relatively frequent in practice. Take for instance PR-1 being a pure refactoring that a reviewer asks you to refactor slightly differently.

Given projects have many files and a dependent PR doesn't always mean it's modifying the same files as the base PR, there are also a lot of dependent PRs that don't conflict with updates to the PR they are based on.

But that doesn't really change anything anyways. The only difference that happens when pr-2 has conflicts with pr-1 is that pr-2 has to merge from pr-1/master and resolve the conflicts before it can be merged to master. i.e. The git graph looks exactly like the second graph (if merging from master) or a 3rd graph I could create (if merging from pr-2). In that case the "Merge branch 'master' into pr-2" includes merge conflict fixes and needs to be done manually instead of automatically by GitHub. GitHub is already capable of detecting merge conflicts that happen when the base branch is updated and offers information about this in a PR and an on-site way to fix merge conflicts – this same UI can be used to handle merge conflicts in dependent PRs.

In summary this feature request is useful but only in a restrictive "fixup and NO squash" review model, thanks a lot for helping clarify this.

"fixup and NO squash" is basically the default review model expected by GitHub and used by many many projects (90% of the ones I've encountered) so there is nothing strange about this.

However to address your concerns. GitHub already correctly handles user initiated rebases. If you force push a new history to a PR GitHub properly replaces the history and updates the PR, it does not leave the PR in a weird state with conflicting previous and current history. This is probably not an officially "supported" model in that GitHub does not offer any UI to make using this model easier; but it does not break this review model if you enforce it yourself and make force pushes.

The only difference this would make in dependent PRs is that GitHub would need to make note of when it sees the history of a PR replaced (though they probably have this information available given the way the added commits UI looks) and on dependent PRs make note that the PR is invalid as the base branch it depends on is no longer the one in the PR it depends on (which would also be a fairly similar scenario to a branch being deleted perhaps), which would require the PR author to rebase.

marc-h38 commented 4 years ago

there are also a lot of dependent PRs that don't conflict with updates to the PR they are based on.

Yes of course, however version control features that work only "most" of the time in the absence of conflicts need a well-defined and clear fallback in case of conflicts:

The only difference that happens when pr-2 has conflicts with pr-1 is that pr-2 has to merge from pr-1/master and resolve the conflicts before it can be merged to master.

So now the git history is polluted not just by fixups, it's also polluted by "backmerges" and conflict resolutions that exist purely to avoid cleaning up commits before they're actually "committed" to the project's history. Not a problem as long as you rarely ever use the git history :-(

"fixup and NO squash" is basically the default review model expected by GitHub and used by many many projects (90% of the ones I've encountered) so there is nothing strange about this.

Sincere thanks lowering my github expectations even lower, it really did help me understand this feature request better. So, it's not strange for github but it is strange from the perspective of someone used to "real git" (or Gerrit) and cleaned up git histories that can be be re-used easily for cherry-picks/backports, reverts, bisects, auto-generated changelogs, etc.

From https://wiki.openstack.org/wiki/GitCommitMessages#Structural_split_of_changes

The cardinal rule for creating good commits is to ensure there is only one "logical change" per commit [hence no fixup!] There are many reasons why this is an important rule...

From @dantman:

(90% of the ones I've encountered)

Thanks to the other 10% or using "real git" outside github I guess you understand where I come from? Seems like it.

If you force push a new history to a PR GitHub properly replaces the history and updates the PR, it does not leave the PR in a weird state

While I didn't remember using the word "weird", I experienced extensively and thoroughly listed all the... "inconveniences" of shoehorning a "pure git"/rewrite/rebase review model inside Github. I recommend you have a look there: https://github.com/zephyrproject-rtos/zephyr/pull/14444#issuecomment-575320941

The only difference this would make in dependent PRs is that GitHub would need to make note of when it sees the history of a PR replaced [..], which would require the PR author to rebase.

I agree it does not seem like rocket science.

HayfaFtirich commented 4 years ago

We've been using dpulls.com and it works great for us. Pros:

Cons:

yairhaimo commented 4 years ago

I created a small Github app and CLI that lets you create and manage Stacked Pull Requests. I would love some feedback: https://stacker-site.now.sh/

ryanhiebert commented 4 years ago

@yairhaimo: cool idea. The linked write-up on stacked PRs doesn't allow for merging from the tree outward, which is a real bummer. The GitHub app I wrote, Chain (https://github.com/apps/chain), supports that case by automatically updating the base of the pull request, which seems preferable to me, plus its a painfully simple implementation.

At this point, though, I'm really wanting more than what either of these provide, AFAICT. I'd be interested to understand a bit more about your motivation for making a CLI. I've been trying to avoid that as I have been thinking about possible solutions.

My goals for my design include:

yairhaimo commented 4 years ago

@ryanhiebert Thanks for the feedback! I agree that the Stacked Pull Requests paradigm is more limiting. This is not necessarily a bad thing. It tells you "this is how you work" so you can put your mental CPU to a better use. I'm sure some users might be frustrated at times by the rigidity of the process but you'll have that with any limiting process. What I like about this is that you don't have any floating/decaying PRs. All the PRs must be merged together - and they can be squashed merged into a single commit into master. I feel as though it also guides you into making bite-sized PRs, but thats just a feeling.

The CLI is just a thin wrapper above the git and hub CLIs. All it does is create a branch and open a PR in a specific convention for easier management. Since all the PRs in the same stack follow a certain convention, you can handle them easily with a simple Chrome Extension (hiding them and showing only the main PR - for example).

bje- commented 4 years ago

Another advantage of dependent PRs is that not every PR needs to be built right-away. If B depends on A, then there is no point building and testing B in Travis CI (or Github workflows) unless A builds and tests successfully first. This can save on a lot of currently wasted build system resources.

nokite commented 4 years ago

@bje- I think this depends on the workflow in your team. In some teams it may be valid to create a separate PR (B) to fix and extend tests. So you need to build B regardless of whether A passes. It sounds OK though if what you're suggesting is an opt-in option.

OliverJAsh commented 4 years ago

GitHub should make it so that if a dependency PR was merged, all dependent PRs that had that merged PR's branch as a base should automatically have their base branches changed to that of the merged PR.

It looks like GitHub now does this? 🎉 More information here: https://github.com/ryanhiebert/probot-chain#note-you-probably-dont-need-this

ryanhiebert commented 4 years ago

Oooh! I'm glad you found my note!

ryanhiebert commented 4 years ago

It doesn't solve all the ideas that this issue is about, but the particular issue of changing the base branch when a dependency PR is merged is, I think, solved very successfully by GitHub natively. Perhaps it always was, I'm not sure.

ryanhiebert commented 4 years ago

This is the note he's referring to for convenience here on this issue. It's specifically about the "Chain" app no longer being needed.


Note: You probably don't need this

GitHub will automatically change the base of pull requests that currently have their head set to the branch being deleted if the deleting is done automatically on merge or via the button on the pull request.

So if you merge a pull request, and then you click the "Delete branch" button:

Delete branch button

Then you'll see this on all the dependent pull requests:

Base automatically changed activity

And this also works if you have GitHub delete branches automatically on merge:

Automatically delete head branches checkbox in settings

So you probably don't need this. If you think you actually do need this, open up an issue and tell me. I'll probably delete this app in a while if you don't.

One particular case that might catch you up is that if you delete the branch via the git cli, it will NOT change the base branch of dependent pull requests, it will just close the issues. I expect that for most cases, though, having it delete head branches automatically, potentially combined with branch protection rules, will be the best way to deal with those cases.

amcsi commented 4 years ago

Since when? 😲 that's amazing news by the way

sffc commented 4 years ago

Does this work when the branches live in another fork? For example, if I have two branches in my fork, one based on the other, how do I open PRs for both of them to the upstream repository and have the head branch automatically resolved?

mosteo commented 4 years ago

Does this work when the branches live in another fork? For example, if I have two branches in my fork, one based on the other, how do I open PRs for both of them to the upstream repository and have the head branch automatically resolved?

I might be wrong, but I have been unable to make work anything when PRs come from a fork. You cannot even use the required PR as a base. You can specify the precise commit instead of a base branch, and the diff is shown properly, but the button to create the PR is not shown. The only recourse is to live with your dependent PR showing its own commits on top of the ones from the required PR. This is a constant itch.

ryanhiebert commented 4 years ago

Does this work when the branches live in another fork?

I just tested that, and no, it does not. At a very generic level, chaining across forks won't work very well, because pull requests have to be opened on the repository of the base branch, and a pull request can't be moved to a different repository.

In the specific case of chaining within a repository, but with the last item being from a fork, I would have hoped that it would be able to update the base just the same, but that doesn't seem to be the case, unfortunately.

While I find that annoying, considering the above limitations that rule out cross-fork chaining, I'm not inclined to think that it's a huge deal. If you're going to chain pull requests, they probably should all be from branches on the same repository anyway for sanity's sake.

It'd be interesting to have some kind of a cross-fork pull request that might be able to do this cross-repository chaining. But that's a way stickier project, and doesn't really fit into the current model of GitHub.

sffc commented 4 years ago

If you're going to chain pull requests, they probably should all be from branches on the same repository anyway for sanity's sake. But that's a way stickier project, and doesn't really fit into the current model of GitHub.

It's a very common workflow for team members to push branches to a private fork and open PRs against upstream master. Chaining PRs across forks would be an important part of that workflow. There's nothing insane about that workflow, and I see it as very much consistent with GitHub's model.

ryanhiebert commented 4 years ago

It's a very common workflow for team members to push branches to a private fork and open PRs against upstream master.

I'm not suggesting it's not common, I'm saying that given the limitations of how pull requests work on GitHub, specifically that they cannot change repositories, that there is in effect no reasonable way to implement chained pull requests across repositories.

You can create a pull request across the repository boundary, and then follow-up pull requests in the second repository, but if you want it to keep the chain working, you must merge all PRs for the secondary repository before merging the one to the main repository.

Maybe someday there will be a way to move pull requests to different repositories that are related via forks, but for now, I don't see a way around that limitation without making some other tool manage the pull requests in the first place.

ryanhiebert commented 4 years ago

Given what I've said, is there anyone who thinks that my Chain app provides utility to them that it justifies me keeping it running? It doesn't for my own use-cases personally. If it provided a way to get cross-repository PR chaining that wasn't so severely hampered, I'd be quick to keep it, but as it is, it seems of rather marginal and inconsistent benefit, and I'm feeling inclined to shut it down. While I've heard people say they want cross-repository chaining (and I agree that it would be awesome), I'm not sure anyone has suggested that the limitations on it make it worth it to keep my Chain app.

Fakerr commented 3 years ago
  • Dependent pull requests should not be mergeable until all dependency pull requests have been merged

Couple months ago, I started a side project called Dpulls that helps working with dependent PRs. Basically, it prevent PRs from being merged before their dependencies.

:rocket: Today, there are over 1500+ projects using Dpulls, and I've been trying to constantly improve it since the launch.

I'd love to hear what you think about it !

cc @abhijeetviswa

bje- commented 3 years ago

Thanks, this seems to do the job!

gregsdennis commented 3 years ago

Here's a GitHub action that does this: https://github.com/marketplace/actions/pr-dependency-check

z0al commented 3 years ago

FYI, I just released Dependent Issues :tada: . The successor of the 3 years old DEP bot.

It's a GitHub action that lets you mark a PR or an issue as dependent on another. It works with cross-repository dependencies as well.

demo

Check the repo for more info: https://github.com/z0al/dependent-issues