On Wed, Jun 09, 2021 at 12:44:47PM -0700, Allison Henderson wrote: > Hi Darrick, > > I've created a branch and tag for the delay ready attribute series. I'ved > added the rvbs since the last review, but otherwise it is unchanged since > v20. > > Please pull from the tag decsribed below. Yay! At last! Good work, Allison. :) Nothing to worry about here, but I thought I'd make an observation on the construction branches for pull requests seeing as pull request are becoming our way of saying "this code is ready to merge". > Thanks! > Allison > > The following changes since commit 0fe0bbe00a6fb77adf75085b7d06b71a830dd6f2: > > xfs: bunmapi has unnecessary AG lock ordering issues (2021-05-27 08:11:24 -0700) This looks like it has been built on top of a specific commit in the linux-xfs tree - perhaps a for-next branch before all the most recent development branches have been merged in. The problem with doing this is that the for-next tree can rebase, which can leave your tree with orphaned commits that are no longer in the main development branch or upstream. While these commits are upstream now, this isn't an issue for this particular branch and pull request. i.e. if the recent rebase of the for-next branch rewrote the above commit, pulling this branch would cause all sorts of problems. So to avoid this sort of issue with pull requests, and to allow the maintainer (darrick) to be able to easily reformulate the for-next tree just by merging branches, pull requests should all come from a known upstream base. In the case of pull requests for the xfs tree, that known base is the master branch of the XFS tree. The only time that you wouldn't do this is when your work is dependent on some other set of fixes. Those fixes then need to be in a stable upstream branch somewhere, which you then merge into your own dev branch based on xfs/master and the put your own commits on top of. IOWs, you start your own branch with a merge commit... If you do this, you should note in the pull request that there are other branches merged into this pull request and where they came from. THat way the maintainer can determine if the branch is actually stable and will end up being merged upstream unchanged from it's current state. It is also nice to tell the maintainer that you've based the branch on a stable XFS commit ahead of the current master branch. This might be necessary because there's a dependency between multiple development branches that are being merged one at a time in seperate pull requests. In terms of workflow, what this means is that development is done on a xfs/master based branch. i.e. dev branches are built like this: $ git checkout -b dev-branch-1 xfs/master $ git merge remote/stable-dev-branch $ git merge local-dependent-dev-branch $ <apply all your changes> $ <build kernel and test> And for testing against the latest -rc (say 5.13-rc5) and for-next kernels you do: $ git checkout -b testing v5.13-rc5 $ git merge xfs/for-next $ git merge dev-branch-1 <resolve conflicts> $ git merge dev-branch-2 <resolve conflicts> .... $ git merge dev-branch-N <resolve conflicts> $ <build kernel and test> This means that each dev branch has all the correct dependencies built into it, and they can be pulled by anyone without perturbing their local tree for testing and review because they are all working on the same xfs/master branch as your branches are. This also means that the xfs/for-next tree can remain based on xfs/master and be reformulated against xfs/master in a repeatable manner. It just makes everything easier if all pull requests are sent from the same stable base commit... Anyway, this isn't an issue for this pull-req because it is based on a stable XFS commit in a branch based on xfs/master, but I thought it's worth pointing out the pitfalls of using random stable commits as the base for pull requests so everyone knows what they should be doing as it's not really documented anywhere. :) Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx
