Bug/#4497 Unable to Cherry Pick Merge Commit Solved

[RounakJoshi09]

📑 Summary

Cherry-pick a merge commit, requires a special flag because merge commits have more than one parent. When you cherry-pick a regular commit, Git knows to take the changes from the parent commit to the chosen commit. In the case of a merge commit, there are multiple parent commits, so you have to specify which parent's context you want to use for the cherry-pick.
If you try to cherry-pick a merge commit without specifying a parent, Git won't know what changes to apply and will prompt you to specify a mainline parent.

Resolves #4497

📏 Design Decisions

The Pull Request addresses this issue by introducing additional syntax to the parser and modifying the cherryPick function to handle merge commit cherry-picking scenarios properly. Specifically:

A new identifier, PARENT_COMMIT, was introduced in the parser syntax to allow users to specify the parent commit when cherry-picking a merge commit.

The cherryPick function was enhanced to validate and handle the parentCommitId parameter when dealing with merge commits. It now checks if the specified parent commit is an immediate parent of the merge commit being cherry-picked and throws descriptive errors when incorrect usage is detected.
The function also handles tagging and branch checks to ensure the cherry-pick operation's correctness and to provide informative feedback to the user regarding any incorrect usage.

With these enhancements, users can now accurately represent cherry-picking merge commits within their Mermaid Git diagrams, improving the library's functionality and usability for illustrating complex Git workflows. The new syntax and function logic cover various cherry-picking scenarios, providing a robust solution to the initially reported problem.

📋 Tasks

Make sure you

• 📖 have read the contribution guidelines
• 💻 have added necessary unit/e2e tests.
• 📓 have added documentation. Make sure MERMAID_RELEASE_VERSION is used for all new features.
• 🔖 targeted develop branch

[netlify]

✅ Deploy Preview for mermaid-js ready!

Name	Link
🔨 Latest commit	d22ee8d
🔍 Latest deploy log	https://app.netlify.com/sites/mermaid-js/deploys/656d673643ec2900084320f9
😎 Deploy Preview	https://deploy-preview-4944--mermaid-js.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[codecov]

Codecov Report

Merging #4944 (d22ee8d) into develop (f81e4d4) will decrease coverage by 0.04%.
The diff coverage is 66.66%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4944      +/-   ##
===========================================
- Coverage    79.41%   79.38%   -0.04%     
===========================================
  Files          166      166              
  Lines        13871    13874       +3     
  Branches       705      705              
===========================================
- Hits         11016    11014       -2     
- Misses        2703     2708       +5     
  Partials       152      152              

Flag	Coverage Δ
e2e	85.19% <66.66%> (-0.05%)	⬇️
unit	43.03% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
packages/mermaid/src/diagrams/git/gitGraphAst.js	56.50% <66.66%> (+0.15%)	⬆️

... and 3 files with indirect coverage changes

[sidharthv96]

Hi @RounakJoshi09, nice work. You'll need to add some E2E tests also, by adding a test in cypress/integration/rendering/gitGraph.spec.js.
This will ensure correct rendering.

[RounakJoshi09]

Hi @sidharthv96 I have made all the suggested changes and also added E2E test for cherry picking merge Commit. Could you please check it once. Thanks

[jgreywolf]

@RounakJoshi09 Have you had a chance to review @sidharthv96 most recent change request?

[RounakJoshi09]

Hi @jgreywolf I am working on requested changes , It will be done by tommorow anyhow

[aloisklink]

Looks good to me!

I do have a potential improvement so that a bad parent: "xxxx" throws an error, even if we're not cherry-pick-ing a merge commit, but it's pretty minor, and this PR has been open for a while, so we can always make that change in another PR!

[sidharthv96]

@RounakJoshi09 was 31a1de1 reverted due to the failing CI? If so, it's unrelated and you can force push 31a1de1 again.

[RounakJoshi09]

Hello @sidharthv96 , I explicitly reverted the commit because after adding the changes by @aloisklink , Unit test and E2E are failing, which I will look today. Or if the issue mentioned by @aloisklink, is not major , and can be fixed in another PR, we can merge this PR, with the previous commit.

[sidharthv96]

Ahh, that error was because it was not checking if parentCommitId was present or not. The unit test errors have now been fixed. Waiting for E2E in CI.

[RounakJoshi09]

Thankyou @sidharthv96 for fixing this, E2E have also been passed, and condition requested by @aloisklink is also added. 😊

[aloisklink]

Awesome! It all looks great! Thanks for making those changes (and thanks @sidharthv96 for the help!)