1. 32

  2. 10

    Title is a bit misleading. The post in question is about the failings of GitHub’s Merge Pull Request button, not the concept of merging pull requests.

    Worth a read: has a good solution that allows cleanly merging mostly-complete-but-still-lacking pull requests whose authors have vanished.

    1. 3

      I always feel uneasy when I rewrite other people’s commits and don’t change the committer…

      1. 6

        OP author here…

        I was originally uneasy about rewriting as well, and three things combined to more than offset it:

        First, it’s easy to see that I had full control to modify the patch, and that I’m ultimately responsible for it. Github makes this less obvious (but still visible), but the underlying git data is crystal clear on it: I am the committer now, not the original contributor, even if they’re credited front and center.

        Second, I’m making the contributor look good (I hope!) and simply improving on their original intent. Basically saying what they would’ve said if they knew all the things I know about the project. And to be clear, if I decided a whole different approach were necessary, I’d not derive their commit, but rather start fresh.

        And finally, one of the largest OSS projects out there, the Linux kernel, uses this approach extensively without apparent dissent that I can find.

        That’s enough for me, but of course it’s still each maintainer’s call as to what approach they’re most comfortable with. I still wouldn’t use the “Merge pull request” button, though, even if you’re not rewriting commits :-)

        1. 3

          At least the Git project asks people to rewrite patches until they are perfect.

          In my interpretation, the Author: is the person who wrote the patch (and will be listed in blame…), and the Committer: is who merged it into the project. I’m considering using a trailer like Original-by: or Contributed-by: for such cases.

          1. 2

            Yup, and I interpret those fields a bit differently, which I think is one thing interesting about git: it supports a lot of different workflows, and so much of how you use it comes down to social, not technological, choices.

            A big piece of being a maintainer is defining and sustaining the “work culture” for your project, and on projects with lots of contributors I think it’s perhaps the biggest piece.

            1. 1

              I think git commit -s adds “Signed Off by” to the commit message.

          2. 1

            Yeah, if you make changes to someone else’s patch, it really seems to make sense to make the changes in a separate commit with separate authorship.

            1. 3

              It depends, the extra commits can add quite some noise to the repo. When checking a repo, I’d much rather see the complete commit rather than having to check two or more and try to make sense of the differences between them. You’d almost have to apply patches in your head to understand the changes.

          3. 3

            For the workflow where you have a maintainer who enforces a bunch of policies then yeah, the github workflow isn’t ideal. But my experience is that the overhead of rewriting all submitted patches is one of the big things putting off maintainers, leading many projects to wither away for lack of a dedicated maintainer.

            The workflow github encourages is a different one: when you see a positive contribution, throw it in. Don’t worry over small details like formatting standards; if they’re a real problem, sooner or later you’ll get another pull request that fixes them. This makes being the maintainer a much smaller overhead, and allows projects too marginal for a maintainer to dedicate time to them to continue accepting contributions.

            1. 3

              A lot of maintainer workflow depends on the type of contributions you’re getting, and the project I help maintain - ActiveMerchant - has the bulk of its contributions from one-time contributors that won’t ever contribute again. That means that while they have super valuable input, they often don’t even know the exact API we provide, and if we took what they offered as offered we’d have a mess on our hands almost immediately. And - full disclosure - when I started helping with maintenance on ActiveMerchant, this is what had happened and the current maintainers are still doing a lot of ongoing cleanup due to it.

              Other projects cater to a more “core set” of contributors, where those contributors are likely to come back with new valuable code on a repeating basis. There’s a lot more call to both educate them and to just rapidly ingest their contributions since they themselves will probably come back and fix mistakes they made. So adapt to your situation, YMMV, etc.

              That said, I’ve used the projects where the maintainers just take “any positive contribution”, and they’re one of the reasons I’m super strict about what dependencies I’ll let in my projects. I don’t want kitchen sink dependencies, I want focused, curated dependencies that have an opinion about their function and about their quality and aren’t afraid to aggressively shape contributions to fit those opinions. I get that being a maintainer of such a project is a lot more work, but it makes the projects so much more valuable to those who consume them.

            2. 2
              1. . Hope that original contributors show back up to finish cleaning up their contributions so they can be merged cleanly.
              2. . Merge contributions that aren’t quite ready, then do another commit to clean them up.

              Those aren’t the only options though, I believe you can just clone the pull request, do the “trivial” git operations (squashing, amending messages and style) in that local checkout, and push that.

              I’m not sure it’s possible to reuse the github pull request for it, but at least it’s cleaner than the “merge + additional commit”, and offers a way out when people disappear.

              1. 1

                Which is exactly the workflow I suggested in the OP, just using the hub CLI and git am as facilitators thereof.

              2. 1

                Having Travis CI running a test suite helps here: you can automatically see if the tests pass before hitting the merge button. Your tests could also include things like linting to check the code is formatted to your project’s spec, of course.

                The first approach to reducing overheads should always be automation where possible.