0

I'm a bit confused about a pull request I did on a fairly large open source project I use in my work. I won't reveal the project, but it contains a large collection of mostly user submitted scripts that are used to monitor various aspects of a mission critical system or application. I found a user-submitted shell script that I needed to use for my own work, but it had several major bugs, and was stylistically a wreck. I fixed the bugs, and refactored almost the entire script, bringing it up to a fairly clean "bash form". I did a pull request on the script, and the project lead rejected the patch with quote:

"This is mostly coding style changes. Your effort is really appreciated, but we won't get anywhere if we start accepting that kind of patches. Please try to focus on the matter, i.e. stuff which really needs fixing. Thanks!"

Here's an example of a bash coding style change for readability I made throughout the script:

- start_time=`date +%s%N`
+ start_time=$(date +%s%N)

Is this common on open source projects? Most projects I've committed to were my own, and I refactor stylistically bad code all the time. If code will be used by other people like the script in question, shouldn't a usability refactor be welcomed? I'm just a bit confused, as the project has no coding style guides.

0

3 Answers 3

2

Did you split the patches so that the bugs are fixed first and the style changes made later? If not, I'd reject the patch as well. A patch that changes logic should be as small and self-contained as possible. Nobody wants to go through dozens of style changes to ensure they have no effect on the logic when trying to understand what you fixed.

1
  • Yes indeed, split cosmetic and functional changes into separate commits. Sep 9, 2012 at 2:29
1

This is absolutely not a universal thing among open-source projects. I'll just give one counterexample, from a recent pull request:

https://github.com/djcb/mu/pull/65

My pull request related to the Emacs component of mu, called mu4e. My only changes were to make many more variables from the project available to edit via M-x customize (the Emacs equivalent of "Edit -> Preferences", or "Tools -> Options"). There were no changes to any of the program logic at all. As you can see, the pull request was merged without any fuss after just two hours. (I had never contributed to this project before, so I didn't get fast-tracked based on my past contributions or anything like that.) So that's one example of a pull request of only cosmetic changes that got happily accepted without any complaint.

As to why the project in question rejected your patch, maybe they're just being stubborn or too proud to let someone else mess around in their code or something like that, but on the other hand, there are valid reasons not to merge cosmetic non-user-facing changes. If they accept your code and they are at least minimally responsible, they will have to at least look at all the changes you're making and make sure you didn't break something, and make sure that your code changes match what your commit log says that you changed. I'm guessing that your pull request changed a whole bunch of isolated lines or blocks throughout many files, right? That's probably what you'd end up with if you did a blanket search and replace of backticks with $(). That kind of patch can be hard to verify, because you look at the same change on line after line and your eyes glaze over and you miss the one case where the close-paren is missing which causes the entire project to break.

The point is that even though you're giving them code for free, actually merging your code is not free, and the work involved in integrating your code into the project must be done by the people who run the project, or else they will be merging code that they can't trust. In some cases, the people who own the project may look at what your changes claim to improve and make the rational decision that those improvements do not justify the effort required to merge your code in a responsible way. If you disagree, you're free to create your own fork (not a "hostile" fork, just a run-of-the-mill "Project X with my customizations" fork) and put your changes there, and use them yourself. If your changes are really worth having, people might start switching to your fork, at which point the original developers might reconsider their position and merge your changes after all.

0

Each project has its own rules and guidelines. You've been told that this project works to different rules from the ones you'd work to. If you want to contribute, it sounds like you'll have to follow their rules. (I tend to be on your side in the debate — but when I'm not in charge, I go by the rules of those who are!)

Not the answer you're looking for? Browse other questions tagged or ask your own question.