Thrift
  1. Thrift
  2. THRIFT-654

What is the best way to merge a bunch of code from Facebook?

    Details

    • Patch Info:
      Patch Available

      Description

      Hey guys. I've been kind of lax porting patches developed at Facebook back into the open-source repository, but I'm catching up now. I have a bunch of Apache-ready patches at http://gitweb.thrift-rpc.org/?p=thrift.git;a=shortlog;h=refs/heads/pri/dreiss/fb-merge-p1;hb=HEAD (long view: http://gitweb.thrift-rpc.org/?p=thrift.git;a=log;h=refs/heads/pri/dreiss/fb-merge-p1;hb=HEAD). It's almost all C++ library changes, with some other misc stuff sprinkled in. What do you guys think would be the best way of merging this stuff into Apache? I'd prefer not to create a separate issue for each C++ patch, especially since they've all been reviewed and production-tested internally. I can do it for the non-C++ stuff if you'd like. Would people be okay if I just create one big patch set for the C++ stuff, attach it here, and commit after a week if there are no objections?

        Activity

        Hide
        Todd Lipcon added a comment -

        Is it possible to rebase -i them into a smaller number of cohesive patches? I'd be fine reviewing a single JIRA of "Miscellaneous bug fixes for python", but I don't think multiple new features should be conflated into the same JIRA.

        Show
        Todd Lipcon added a comment - Is it possible to rebase -i them into a smaller number of cohesive patches? I'd be fine reviewing a single JIRA of "Miscellaneous bug fixes for python", but I don't think multiple new features should be conflated into the same JIRA.
        Hide
        Bryan Duxbury added a comment -

        I would think that at the very least you should summarize what changes you're making so that interested reviewers know what they're scanning.

        Show
        Bryan Duxbury added a comment - I would think that at the very least you should summarize what changes you're making so that interested reviewers know what they're scanning.
        Hide
        David Reiss added a comment -

        Okay. I've created individual issues for everything that isn't C+. I'll create a single issue with a summary of the C+ stuff and we'll see how it looks.

        Show
        David Reiss added a comment - Okay. I've created individual issues for everything that isn't C+ . I'll create a single issue with a summary of the C + stuff and we'll see how it looks.
        Hide
        Bryan Duxbury added a comment -

        Are you still waiting to merge in these changes?

        Show
        Bryan Duxbury added a comment - Are you still waiting to merge in these changes?
        Hide
        David Reiss added a comment -

        Yeah. I just asked if anyone cared if I commit THRIFT-665. I was going to check them all in tonight (the others are approved or trivial).

        Show
        David Reiss added a comment - Yeah. I just asked if anyone cared if I commit THRIFT-665 . I was going to check them all in tonight (the others are approved or trivial).
        Hide
        Bryan Duxbury added a comment -

        I'm assuming this has actually been done by now. If that's incorrect, please do it and retag the issue as 0.5.

        Show
        Bryan Duxbury added a comment - I'm assuming this has actually been done by now. If that's incorrect, please do it and retag the issue as 0.5.

          People

          • Assignee:
            David Reiss
            Reporter:
            David Reiss
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development