When You Merge Pull Requests You Lose Knowledge
Tags: codereview, pullrequest, knowledge, productivity
Published: 2021-08-24 15:33:30

_What happens to comments and discussions of a pull request when it is merged? Isn’t that knowledge valuable for the team? Why isn’t it readily accessible to future pull requests?_

Pull requests (PRs) are a mainstream way of proposing changes to a codebase. One of the most important features around pull requests is the ability to perform a code review, allowing developers to inspect and comment on the proposed changes.

Frequently, the knowledge in these comments is invaluable to the team. Reviewers, which are familiar with the codebase and related technologies, will notice bad patterns, suggest optimisations, enforce team guidelines and best practices, etc.

However, as soon as you merge that pull request on GitHub, Gitlab, or Bitbucket, all this information is _lost_ into the bag of closed PRs that you will rarely (if ever) inspect.

We all have had that dé_jà vu_ feeling when writing a comment in a code review: “Didn’t I write this comment already?” When preparing a PR sometimes we also have that feeling: “I think I’m forgetting something in this line of code.” Or better yet: “Didn’t someone already thought of this before?”

Let’s look at three scenarios.

#### 1\. Knowledge useful to a broader audience

Knowledge that could be beneficial to developers not involved in this codebase. For instance, [this comment](https://github.com/apache/superset/pull/14710#discussion_r637929403) could be useful for other developers modifying similar code.

![Discussions useful in the future](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/at6xrzdnuh1t2h2xu90i.png)
<figcaption><a href="https://github.com/apache/superset/pull/14710#discussion_r637929403">Discussion</a><em>&nbsp;by&nbsp;</em><a href="https://github.com/kgabryje">kgabryje</a><em>&nbsp;at&nbsp;</em><a href="https://github.com/apache/superset">apache / superset</a><em>&nbsp;“</em><a href="https://github.com/apache/superset/pull/14710">feat(native-filters): add search all filter options #14710</a>“</figcaption>

#### 2\. Knowledge about design decisions

Teams with a lot of contributors can suffer from problems associated with the lack of context in design decisions. For instance, when a decision was taken on a discussion on a previous pull request. Consider the following discussion:

![Useful knowledge about design decisions in discussions](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/6k8ih0g0glzg1tc3ic04.png)

<figcaption><a href="https://github.com/dapr/dapr/pull/3297#discussion_r665609722">Discussion</a><em>&nbsp;with&nbsp;</em><a href="https://github.com/halspang">halspang</a><em>&nbsp;and&nbsp;</em><a href="https://github.com/artursouza">artursouza</a><em>&nbsp;at&nbsp;</em><a href="https://github.com/dapr/dapr">dapr / dapr</a><em>&nbsp;“</em><a href="https://github.com/dapr/dapr/pull/3297">Write reminders in multiple partitions #3297</a><em>“</em></figcaption>

Assuming that this PR is merged, when someone else modifies this code in the future, it would be useful to retrieve this discussion. In this case, that would be particularly relevant if this person starts the support for a decrementing path.

#### 3\. Review déjà vu

Senior developers that are active reviewers also experience what we call review déjà vus. These are situations where you are repeating similar comments in different PRs, like this one:

![Review dejá vù in discussions](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/lc7wccblup0rzmnal1n8.png)

<figcaption><a href="https://github.com/nim-lang/Nim/pull/16203#discussion_r534435743">Comment</a><em>&nbsp;by&nbsp;</em><a href="https://github.com/timotheecour">timotheecour</a><em>&nbsp;at&nbsp;</em><a href="https://github.com/nim-lang/Nim">nim-lang / Nim</a><em>&nbsp;“</em><a href="https://github.com/nim-lang/Nim/pull/16203">oids: switch from PRNG to random module #16203</a><em>“</em></figcaption></figure>

* * *

These situations also occur when there are some hidden assumptions that do not make sense to document in the code.

Wouldn’t it be nice if we could re-use the comments from past reviews into future PRs without having to maintain them?

### Enter Reviewpad

Reviewpad builds semantic diffs of changes in pull requests and associates metadata such as comments to these semantic objects. This is a big shift from the current behavior of existing code hosts such as Github, Gitlab, and Bitbucket. They associate comments to a line of a file in a particular git diff – as soon as the contents of the line change, this comment is marked as _outdated_.

We’ll showcase Reviewpad with the _[Web Crawler](https://tour.golang.org/concurrency/10)_ exercise of A Tour of Go using the linked [GitHub repository](https://github.com/reviewpad/dejavus). In this exercise of A Tour of Go, we are given a first implementation of a fake web crawler – the exercise is to parallelise it and prevent fetching the same URL twice.

#### Semantic git diffs

As a first step, we used [Reviewpad](https://beta.reviewpad.com/review/github.com/reviewpad/dejavus/pull/2) to create a pull request with this first implementation.

![Reviewpad screenshot](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/e226jgeda6o3ggq0bpqi.png)

<figcaption>Reviewpad interface for <a rel="noreferrer noopener" href="https://beta.reviewpad.com/review/github.com/reviewpad/dejavus/pull/2" target="_blank">pull request</a> after review and merge</figcaption>

Reviewpad performs a relational static analysis (in the style of [\[1\]](#1) and [\[2\]](#2)) over the git diff associated with the pull request to build semantic diffs. These semantic diffs form what we call the **Explore Tree** in Reviewpad.

![Reviewpad symbol tree](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/o7jzqmaws5rryw9ox9be.png)

<figcaption>Explore tree for <a href="https://github.com/reviewpad/dejavus/pull/2" data-type="URL" data-id="https://github.com/reviewpad/dejavus/pull/2" target="_blank" rel="noreferrer noopener">pull request</a></figcaption></figure>

An explore tree is an annotated file tree of the modified files. For each modified file, we present an annotated semantic tree of the changes. For example, the file `fakeFetcher.go` as presented on [GitHub](https://github.com/reviewpad/dejavus/pull/2/files#diff-242890664cf40fb88eaec89e6f477a923f63fec6fed10c42bb9e4fa2a95606f6) requires an entire scan to understand the main symbols added: 1) the type `fakeFetcher`, 2) the struct `fakeResult` with two fields `body` and `urls` and 3) the method `Fetch`. Note that the method `Fetch` is shown as a child of the type `fakeFetcher` as it is the receiver type. At the moment, our analysers focus on types and methods – this is why we don’t show variable `fetcher`. The color in the beginning of the symbol indicates the type of modification: green for added, red for deleted and yellow for modified.

We show the explore tree on the left of the description and general conversation so that developers can quickly match the description and discussions to code changes.

Each element of the explore tree is clickable to its representation in the git diff. For example, clicking on the `fakeFetcher` type directs the user to the [beginning of the symbol in the diff](https://beta.reviewpad.com/review/github.com/reviewpad/dejavus/pull/2?base=953653eb93f5a5f57811746e197eb19fd9bc8d74&head=e06496d88c2bb59c3cb17b497ef7caeb3c16594f&review-mode=true#diff-fakeFetcher.goR5):

![Reviewpad screenshot](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/vc8r6icp372563a0zdho.png)

<figcaption>Navigation from the explore tree to the code diff</figcaption>

Note: For those you noticed the coloured icon next to the file `go.mod` – this means that the file is automatically hidden from the review mode. Each Reviewpad user can tweak their file view in their review settings to exclude files.

In this PR, I did a [first review](https://beta.reviewpad.com/review/github.com/reviewpad/dejavus/pull/2?base=953653eb93f5a5f57811746e197eb19fd9bc8d74&head=e06496d88c2bb59c3cb17b497ef7caeb3c16594f#review-d11ab796-13e5-4735-8060-9b45499e0069) to document parts of the code. For example, while the original code for `Crawl` is:

    // Crawl uses fetcher to recursively crawl
    // pages starting with url, to a maximum of depth.
    func Crawl(url string, depth int, fetcher Fetcher) {
    	// TODO: Fetch URLs in parallel.
    	// TODO: Don't fetch the same URL twice.
    	// This implementation doesn't do either:
    ...
    }

I chose to [comment](https://beta.reviewpad.com/review/github.com/reviewpad/dejavus/pull/2?base=953653eb93f5a5f57811746e197eb19fd9bc8d74&head=e06496d88c2bb59c3cb17b497ef7caeb3c16594f&review-mode=true#comment-ea03c917-a66f-454d-a3b8-74b7b30e05d1) on the function as a code diff comment. These review comments are inserted into the knowledge base maintained by Reviewpad:

![Reviewpad screenshot](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/0zlnkoy7544cyurld5i9.png)

<figcaption>Diff comment in Crawl function</figcaption>

Note that the comment in the explore tree appears as a child of `Crawl` symbol. This means that the comment was made in the definition of the symbol (not just in its signature).

#### Comments from past reviews

After the PR with the initial code was squash and merged, I created a [new PR](https://beta.reviewpad.com/review/github.com/reviewpad/dejavus/pull/3) with a [solution](https://www.notion.so/6a4979aa7f8b90db6cbc74400d0beb49) presented by the GitHub user [harryhare](https://github.com/harryhare).

![Reviewpad screenshot](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/igqrf320iogzzf6f8yjx.png)
<figcaption>Reviewpad interface for <a rel="noreferrer noopener" href="https://beta.reviewpad.com/review/github.com/reviewpad/dejavus/pull/3" target="_blank">pull request</a> with first solution</figcaption>

The explore tree again shows the overview of the solution which involve the introduction of a `SafeCounter` struct with a map `v` to check if an URL was visited, a mutex `mux` and a method `checkvisited` that checks if URL was already visited or not.

The rest of the solution involves changes to the functions `Crawl` and `main` to fetch URLs in parallel. In the `Crawl` method, Reviewpad presents the comment from the past PR that documents the method. This way, developers can retrieve discussions from previous PRs that changed this method.

![Reviewpad screenshot](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/vzx14m55fyrq9m5njz0a.png)

<figcaption>Code diff view with comment from previous pull request</figcaption>

A user review raised an issue with the `checkvisited` which is fixed in the commit `[fix from review](https://github.com/reviewpad/dejavus/pull/3/commits/a629a38fddad2305a13f2ce07da6a001c263920b)`.

A [final review](https://beta.reviewpad.com/review/github.com/reviewpad/dejavus/pull/3?base=004717a707bb13b9b25cae49cb591c412ae7b8ff&head=a629a38fddad2305a13f2ce07da6a001c263920b#review-cb18ad6e-eb82-47f2-8626-d3dbb7f1e43c) approves the PR with the request to remove the TODOs in the code.

In the final [commit](https://beta.reviewpad.com/review/github.com/reviewpad/dejavus/pull/3?base=004717a707bb13b9b25cae49cb591c412ae7b8ff&head=17df68f8195eb427fd84d57fae7dc92663a8efd8&review-mode=true&hide_symbol_comments_history=false#diff-crawler.goR7) of this PR, we add the reference to the solution as a code comment in the `Crawl` method and remove the TODOs.

![Reviewpad screenshot](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/jmn4nqfepcu9brnhdm74.png)

<figcaption>Reviewpad interface for all changes in <a rel="noreferrer noopener" href="https://beta.reviewpad.com/review/github.com/reviewpad/dejavus/pull/3" target="_blank">pull request</a></figcaption>

Hovering on the function `Crawl` in the explore tree displays the code documentation associated with this function for readability.

#### Keeping track of comments and changes over time

At this point you might be wondering:

*   What happens if the comment was made in a PR that was not the exact previous one?
*   What happens if I start refactoring the code?

To answer these questions, let’s go over the two open PRs in our [project](https://beta.reviewpad.com/project/github.com/reviewpad/dejavus).

The first is an improvement proposed by another Github user that uses _wait groups_.

![Reviewpad screenshot](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/933mu23pptppv30rjt7g.png)

<figcaption><a rel="noreferrer noopener" href="https://beta.reviewpad.com/review/github.com/reviewpad/dejavus/pull/4?base=b8faf59d56b2635b94a3a87431b6a654a917f8dd&amp;head=e548593190974aa0327fb2d6b6ec516a6900eb61&amp;review-mode=true" target="_blank">Pull request</a> proposing an improvement using wait groups</figcaption>

The comment shown in this pull request was the one made in the first PR. Reviewpad tracks comments regardless of their time difference. For example in the following [PR](https://beta.reviewpad.com/review/github.com/google/error-prone/pull/1817?base=e7a6713dd4775efaece12a2facd2f29d5ba02c25&head=f57bdf7d67fb340abad6f3732f2b8e1cab12ea76&review-mode=true#diff-check_api/src/main/java/com/google/errorprone/fixes/SuggestedFixes.java) from the google/error-prone project:

![Reviewpad screenshot](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/8c13p63ke879gwv8o0xp.png)

<figcaption>Reviewpad review mode for <a rel="noreferrer noopener" href="https://beta.reviewpad.com/review/github.com/google/error-prone/pull/1817?base=e7a6713dd4775efaece12a2facd2f29d5ba02c25&amp;head=f57bdf7d67fb340abad6f3732f2b8e1cab12ea76&amp;review-mode=true#diff-check_api/src/main/java/com/google/errorprone/fixes/SuggestedFixes.java" data-type="URL" data-id="https://beta.reviewpad.com/review/github.com/google/error-prone/pull/1817?base=e7a6713dd4775efaece12a2facd2f29d5ba02c25&amp;head=f57bdf7d67fb340abad6f3732f2b8e1cab12ea76&amp;review-mode=true#diff-check_api/src/main/java/com/google/errorprone/fixes/SuggestedFixes.java" target="_blank">pull request</a> of the google/error-prone project. You can check it on Github <a href="https://github.com/google/error-prone/pull/1817" data-type="URL" data-id="https://github.com/google/error-prone/pull/1817">here</a>.</figcaption>

The PR was opened in September 2020 and it is changing class methods that were commented in January 2020.

The second open [pull request](https://beta.reviewpad.com/review/github.com/reviewpad/dejavus/pull/5) demonstrates a beta version of our refactoring analyser:

![Reviewpad screenshot](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/nzsb03bsy9qvawjf46ui.png)

<figcaption><a href="https://beta.reviewpad.com/review/github.com/reviewpad/dejavus/pull/5" data-type="URL" data-id="https://beta.reviewpad.com/review/github.com/reviewpad/dejavus/pull/5" target="_blank" rel="noreferrer noopener">Pull request</a> with a refactoring</figcaption>

In this PR, we introduced two commits, the first that changes the signature of the function `Crawl` by renaming the variable `url` to `reqUrl` and the second renames the file.

Reviewpad understands that it is the same function and presents the comment made in the first PR. Note that we are squash and merging these pull requests – the commit associated with the comment is not an ancestor of the commits in this PR.

### Cool. How do I check this out by myself?

We have a public beta version of Reviewpad available at [https://reviewpad.com/get-started/](https://reviewpad.com/get-started/). You will need to create a new account and once you log in for the first time, you will see the following page to connect to a code host:

![Reviewpad Screenshot](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/nn7yr1j3wungd53tqwr0.png)

<figcaption>Connect to code host page on Reviewpad</figcaption>

You can connect to GitHub through our OAuth app or manually [add a personal access token](https://docs.reviewpad.com/github). The OAuth requires minimal scopes to be able to read and comment on public repositories.

And that’s it – you will be able to give Reviewpad a spin on public repositories! We will be adding more and more public repositories in the upcoming days – feel free to reach us on our [community Slack](https://reviewpad.com/slack) with requests!

### References

\[1\]: [Cartesian hoare logic for verifying k-safety properties](https://dl.acm.org/doi/abs/10.1145/2908080.2908092).

\[2\]: [Verified three-way program merg](https://www.notion.so/18-Stop-losing-knowledge-when-you-merge-pull-requests-c9696eafba52432ab09f62ac1abce95b)[e](https://dl.acm.org/doi/10.1145/3276535).