2

At my work, we use Git as our version control system. We have a master branch, which I have direct commit access to. Sometimes I have to make a trivial fix, such as fixing a typo in documentation, and the unwritten standard in our office is that getting reviewers for that is a waste of time (please don't argue this point, changing company culture is not a battle I want to fight today). So I have two options for how to go about making my trivial change:

Create a Pull Request

  1. Create a branch off of master
  2. Make my trivial change
  3. Push the change to the branch remotely
  4. Go the Web UI, which is the only way to make pull requests on our system
  5. Create a pull request with zero reviewers*
  6. Merge it

Direct Commit

  1. Make my trivial change
  2. Commit directly to master

The end result for the two is the same; my change makes it into master. But the first method takes much more time than the second does. Assuming my team is not interested in reviewing the change, is there any reason, technical or organizational, that I would want to do the pull request rigmarole?

Community
  • 1
Thunderforge
  • 2,668
  • 3
  • 23
  • 30
  • I don't think we can tell you what your organization would expect. But I'd just make the change and commit it. – GrandmasterB Jan 17 '17 at 16:51
  • 1
    related question - http://softwareengineering.stackexchange.com/questions/335654/git-what-issues-arise-from-working-directly-on-master – linuxunil Jan 17 '17 at 16:52
  • If you actually follow the steps you described, then there's obviously no reason to go for the option which takes more time. A more interesting question might be "Is it worth spending the time reviewing changes I believe are obvious/small/inconsequential?" – Vincent Savard Jan 17 '17 at 16:58
  • That being said, depending on your process, maybe you have pull requests which you can't merge unless your test suite runs successfully, which is something you would lose if you committed directly into master. That may be a reason to keep using merge requests. – Vincent Savard Jan 17 '17 at 16:59
  • 1
    Get confirmation/consensus from your team mates what the protocol should be. – ratchet freak Jan 17 '17 at 17:00
  • Are you only interested in technical reasons? I think the social reasons are reasonable - sometimes people want to be notified when areas of the codebase change. If *most* work goes through PRs then seeing a change that you were completely unaware of can be off-putting. – Daenyth Jan 17 '17 at 17:10
  • @Daenyth I was thinking of technical reasons, but your comment about changes happening you were unaware of would be a fair reason. I've edited the question to clarify that, given that devs don't want to review these trivial changes, what would technical or social reasons be for doing a pull request over a direct commit. If I can make further changes, let me know. – Thunderforge Jan 17 '17 at 17:14
  • In github, a revert of a PR is trivially simple because it's one button to hit in the UI. So not only does the PR process provide better visibility, it's easier for someone to revert. – Steven the Easily Amused Sep 30 '22 at 21:45

1 Answers1

4

I once spent a full week finding a bug that was caused by a checkin like this. Not even a trivial fix, but a trivial change where a developer thought that if (p != NULL) should be replaced with if (! p) because it looks nicer, and shouldn't be code reviewed because it was trivial. (The bug only became visible after a co-worker spent six months completely rewriting a subsystem, and it led to a crash if you put your computer to sleep for 40 to 50 seconds, and in no other case).

Make sure that code reviews can be done with minimal overhead. In my workflow I can do a trivial change plus code review in two minutes. That's not worth taking risks. If it takes you longer, change your process.

gnasher729
  • 42,090
  • 4
  • 59
  • 119
  • 1
    Would you advocate doing this even for documentation typo fixes that will not have any code impact? – Thunderforge Jan 17 '17 at 17:39
  • 1
    @Thunderforge what kind of documentation fix? Technical documentation may include e.g. shell commands to run to install or configure software, where an honest typo fix could have greater consequences. A set of instructions might have very specific logic encapsulated in English grammar constructs. While a paragraph of less technical text might benefit from fixing spelling or grammar errors with lower risk. –  Jan 17 '17 at 18:26
  • A recent one was a Javadoc fix where we had something like "returns a list of candidatf ids" where the word *candidate* was misspelled. Our Javadoc is only used by devs. – Thunderforge Jan 17 '17 at 21:32