The JGit effective contributor

The JGit effective contributor

You've submitted a patch to the JGit project. For you, it is just one change (that probably you understand well) - you want to submit it and move on. But for the maintainer, it's something they will have to take care of for the long run, after you have moved on and can't help. This difference in perspective can make the maintainer seem picky or hard to convince that the review is ready.

Still, the review process doesn't need to be long, nor tiring or frustrating. When things go well, both sides learn something.

Here are some suggestions to make the process easy for you and your reviewer:

An effective review flow

• The review process is part of the development process

  The review is not a rubber-stamp process, nor an exam nor a barrier to go over at any cost. It helps you get the changes into shape, to blend with the rest of the code.

  As tests gives you confidence on the working of your code, the review does so with other aspects of the code (e.g. somebody else understands it!).

  Submitting the change is when you can consider it done. There is no such thing as "this is ready, just waiting for review".

• The review is a dialogue, keep it alive

  If the reviewer leaves comments, but the answer from the author takes too long (~a day), the reviewer loses context. This makes it harder to revisit the review next iteration, leading to longer gaps.

• The reviewer has been around longer, respect the experience

  The reviewer has more experience in the codebase and the project, specially for maintainability. Follow their suggestions, which usually come with a reasoning. If you disagree, be concrete and provide a solid why. Reviewers are not infallible, but they are speaking from experience, so consider their perspective carefully.

  Becoming argumentative about coding style or naming, or giving lessons to the reviewer, causes reviews to devolve into conflict and is definitely not recommended.

• Make your changes easy to review

  If your change is small and clear with a good description, the maintainer can review and submit right away.

  If you have a change that first requires renaming some functions, then moves stuff to a new class, then finally does the change that you need, split that into smaller commits. Many of those changes are mechanical (rename a function) and can be reviewed/submitted without fuss. Good for you (work moves), good for the reviewer (they can see and focus in the important final change).

• Make sure to write a proper commit message. This is about content not formatting (although the format should help you to do it right)

  Linus Torvalds: “So I really want people to document their merges, not just so that I (and others) can see "oh, that's why it exists at all", but also because I want to make people think about their merges more in general.”

  • If you cannot explain clearly the problem, your solution won't look credible.
  • The reviewer should not need to guess why the change is necessary.
  • If it causes a new problem and we need to undo this change, is good to know what was it solving in the first place.
  • While writing the message you can notice that the change is doing too many things or your solution could be simpler
  • The Linux kernel has a detailed explanation about commit messages. Not everything applies to JGit commits but it captures the spirit.

What is the reviewer going to look at?

A maintainer will submit a change when they understand it and are satisfied with the code. There is no fixed checklist, but it is not a capricious process either.

Here is an approximate list of things that maintainers look at on new changes in JGit, in the order that they usually happen. This is not an exhaustive list and not everything applies to every change.

First impressions matter (usually these are covered in the commit message)

• What is the motivation for making this change?
• Is the change concise and well explained?
• Why didn't we see this before? or How did you encounter it?
• How does this impact performance?
• How safe is this change? What is the failure mode? Is it easy to detect problems?
• Is it worthy to fix/implement?

The design (how it fits)

• Is this solving the problem in the right place?

  • Right level of abstraction
  • Is it a workaround for another problem? (Fix the root cause instead)

• Is it covering all cases?

  • Are we aware of all the implications?
  • Some parts of JGit code are not well encapsulated
  • Some classes can be used in many configurations, did you test with them?

• Is the code structure efficient? Is it maintainable?

• What happens if this breaks

  • Is this going to corrupt data and require restoring a backup?
  • Can it be easily disabled?

• Does it keep the conceptual integrity of the project?

  • e.g. Generators keep a shared pool of objects, do not write one with its own pool

The coding part

At this point, there is agreement on what needs to be fixed and how. The only thing remaining is get the code up to standards and submit it. Almost there!

As this is the easiest part of the review, it is tempting to do it first, before the higher-level bits above. It's better to avoid doing it too early - what's the point in having someone fix formatting/naming on an entire class, if they're going to move it to a completely different abstraction layer anyway?

• Is it following the coding style and formatting?

• Do new files have license header?

• Is it tested enough to feel confident on the code?

  • Are the edge cases covered? Not just happy path, but all errors too?
  • Are null/empty parameters tested?
  • Are the tests robust? (reproducible, not flaky)
  • Do tests cover the API (e.g. combination of parameters)?

• Does it respect API compatibility?

  • Could this break subclasses?

• Do all public methods have javadoc?

• Is it using the JGit facilities?

  • e.g. ObjectId collections, Utils...