Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#1877] Show merge commits in the report #1882

Merged
merged 25 commits into from
Mar 7, 2023

Conversation

ckcherry23
Copy link
Member

@ckcherry23 ckcherry23 commented Feb 2, 2023

Fixes #1877

Proposed commit message

Currently, merge commits are completely ignored during report generation
and analysis. This misrepresents the number of commits the user has made
and misses out on important information included in the merge commits,
such as tags.

Let's include merge commits in the report so that important information
in such commits is not missed out.

Implementation details

  • To include merge commits in the reports generated by the backend, I have modified the git log --no-merges to be git log --full-history.
  • The git log command now also returns the parent hashes of the commit, and I determine whether it is an isMergeCommit based on whether it has more than 1 parent hash.
  • The Ramp Chart shows merge commits now as very thin ramps.
  • The Commits Panel shows all commits including merge commits when all file types are selected. When only some file types are selected, empty merge commits are not shown (since they do not come under any file category). We could extend this to explicitly show/hide merge commits from the commits panel.

Other changes made:

  • Backend unit tests: Created test branch 1882-CommitInfoAnalyzerTest-analyzeCommits_mergeCommits_success in testrepo-Alpha
  • Backend system tests: Regenerated all commits.json files
  • Frontend cypress tests: Made 2 commits with Eugene's credentials in the cypress test branch

@damithc
Copy link
Collaborator

damithc commented Feb 2, 2023

@ckcherry23 thanks for taking this up. When ready, also post a screenshot.

@ckcherry23 ckcherry23 requested a review from a team February 2, 2023 05:39
@yhtMinceraft1010X
Copy link
Contributor

yhtMinceraft1010X commented Feb 2, 2023

  • The Ramp Chart shows merge commits now as very thin ramps. Ramp sizes are now calculated based on insertions + deletions instead of just insertions.
  • File changes are now shown as +60 -12 lines instead of 60 lines.

With regards to these points, I'm of the opinion that changes to the ramp size calculation and file changes display should be done in a separate PR (which I think can solve #1042), and once that is merged, continue working on this PR.

What does everyone else @reposense/active-reviewers @reposense/devs think?

@ckcherry23
Copy link
Member Author

ckcherry23 commented Feb 2, 2023

@ckcherry23 thanks for taking this up. When ready, also post a screenshot.

Here's a screenshot of the behavior now. Will update again based on my peer reviews.

This is the repo that was used to report the bug. The light blue ramps are for 2023-01-29, and have 3 merge commit ramps between 2 normal ramps. The Commits Panel has the merge commits and the previously missing tags as well.

Screenshot 2023-02-02 at 5 04 38 PM

With regards to these points, I'm of the opinion that changes to the ramp size calculation and file changes display should be done in a separate PR (which I think can solve #1042), and once that is merged, continue working on this PR.

Okay, I think I will make the changes to ramp size calculation and file changes separately to limit the scope of this PR.

@damithc
Copy link
Collaborator

damithc commented Feb 2, 2023

With regards to these points, I'm of the opinion that changes to the ramp size calculation and file changes display should be done in a separate PR (which I think can solve #1042), and once that is merged, continue working on this PR.

Yes, this requires more deliberations.

Copy link
Contributor

@chan-j-d chan-j-d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The backend changes look pretty good. I think it might be necessary to add some new unit tests to check if merge commits are correctly identified

@HCY123902
Copy link
Contributor

HCY123902 commented Feb 20, 2023

The current changes look great to me. You can request a separate review from me as soon as you complete the cypress changes.

I only have 1 more doubt. This is again related to the case where a merge commit contains changes for conflict resolution.

According to this post and this post, git log --numstat does not actually diff a merge commit against either of its parent by default, because it does not know which parent to compare with. To actually get the number of additions and deletions, there has to be an additional option such as -c, --cc, or -m to either combine or pick the difference.

According to the posts, -c and --cc will only compute differences for a merge commit that is different from both of its parents, meaning that the merge commit contains additional changes for conflict resolution, and other merge commits will again be skipped. The -m option compares the commit with each of its parents separately.

I am not sure about the current behavior of the wrapper class in GitLog.java. I do not think what I mentioned above is significant either at the moment, because it may introduce additional complications to the logic. Instead, this can be handled in the future in a separate pull request

@ckcherry23 ckcherry23 marked this pull request as ready for review March 3, 2023 17:42
@ckcherry23 ckcherry23 requested review from a team and removed request for a team March 3, 2023 17:43
Copy link
Contributor

@HCY123902 HCY123902 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that there are 2 cypress cases regarding zoom view that did not pass. Can you take a look

@HCY123902 HCY123902 requested a review from a team March 4, 2023 06:33
Copy link
Contributor

@zhoukerrr zhoukerrr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am only reviewing the frontend side of this PR. LGTM! The cypress tests looks good to me as well.

@zhoukerrr zhoukerrr requested a review from chan-j-d March 5, 2023 10:03
Copy link
Contributor

@chan-j-d chan-j-d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Unit tests look good and the frontend changes seem to work well. Great work!

@dcshzj dcshzj merged commit d9b1f7b into reposense:master Mar 7, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Mar 7, 2023

The following links are for previewing this pull request:

vvidday added a commit to vvidday/RepoSense that referenced this pull request Mar 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show merge commits in the report
7 participants