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

GitGraph - line routing and colour for adjacent merges #4912

Closed
ijmitch opened this issue Oct 5, 2023 · 19 comments · Fixed by #4927
Closed

GitGraph - line routing and colour for adjacent merges #4912

ijmitch opened this issue Oct 5, 2023 · 19 comments · Fixed by #4927
Labels
Contributor needed Good first issue! Graph: Git Status: Approved Is ready to be worked on Type: Bug / Error Something isn't working or is incorrect

Comments

@ijmitch
Copy link

ijmitch commented Oct 5, 2023

Description

If two branches are merged 'back-to-back' without any other intervening action, the route drawn and colour chosen for the second merge seems wrong. Adding a commit to the branch which is merged second results in a much better outcome.

With adjacent merges:
image

With the extra commit as a workaround:
image

Steps to reproduce

See the sample below...

Screenshots

See the images above

Code Sample

gitGraph
      commit
      commit
      branch feature-001
      commit
      commit
      checkout main
      branch feature-002
      commit
      checkout main
      merge feature-001
      merge feature-002

My workaround is:

gitGraph
      commit
      commit
      branch feature-001
      commit
      commit
      checkout main
      branch feature-002
      commit
      checkout main
      merge feature-001
      checkout feature-002
      commit
      checkout main
      merge feature-002
Loading

Coded as:

gitGraph
      commit
      commit
      branch feature-001
      commit
      commit
      checkout main
      branch feature-002
      commit
      checkout main
      merge feature-001
      %% --------------
      %% now make an extra commit on feature-002 as workaround to routing of its later merge
      %% --------------
      checkout feature-002
      commit
      checkout main
      merge feature-002

So the additional commit has to be after the merge of feature-001.

Setup

  • Mermaid version:
  • Browser and Version: [Chrome, Edge, Firefox]

Suggested Solutions

No response

Additional Context

No response

@ijmitch ijmitch added Status: Triage Needs to be verified, categorized, etc Type: Bug / Error Something isn't working or is incorrect labels Oct 5, 2023
@nirname nirname added Contributor needed Status: Approved Is ready to be worked on Good first issue! Graph: Git and removed Status: Triage Needs to be verified, categorized, etc labels Oct 5, 2023
@nirname
Copy link
Contributor

nirname commented Oct 5, 2023

Confirmed. If it renders exactly as on the screenshots provided, that means it is either not fixed yet, or GH has not updated it yet, which is to say it reproduces on GH

gitGraph
      commit
      commit
      branch feature-001
      commit
      commit
      checkout main
      branch feature-002
      commit
      checkout main
      merge feature-001
      merge feature-002
Loading

@programming-warrior
Copy link

can you please elaborate the problem statement? I am new here and want to contribute to this issue.

@ijmitch
Copy link
Author

ijmitch commented Oct 6, 2023

The line representing feature-002's merge into main is drawn in the colour chosen for main rather than that for feature-002. The line should also take a non-crossing route to join main.

@guypursey
Copy link
Contributor

I'm finding that even adding more than one commit to feature-002 does not correct the drawing of the branch.

Screenshot:

Second feature branch still overlaps

Code:

gitGraph
      commit
      commit
      branch feature-001
      commit
      commit
      checkout main
      branch feature-002
      commit
      commit
      checkout main
      merge feature-001
      merge feature-002

@ijmitch
Copy link
Author

ijmitch commented Oct 6, 2023

My workaround is:

gitGraph
      commit
      commit
      branch feature-001
      commit
      commit
      checkout main
      branch feature-002
      commit
      checkout main
      merge feature-001
      checkout feature-002
      commit
      checkout main
      merge feature-002
Loading

Coded as:

gitGraph
      commit
      commit
      branch feature-001
      commit
      commit
      checkout main
      branch feature-002
      commit
      checkout main
      merge feature-001
      %% --------------
      %% now make an extra commit on feature-002 as workaround to routing of its later merge
      %% --------------
      checkout feature-002
      commit
      checkout main
      merge feature-002

So the additional commit has to be after the merge of feature-001.

@guypursey
Copy link
Contributor

Ah, I see! As the workaround screenshot is already there, it might be worth putting this code under in the issue description too, perhaps under the Code Sample so there's a reference for the workaround.

@guypursey
Copy link
Contributor

Looks like this might be an issue even for simpler use cases, not just adjacent merges...

Here's the example diagram bundled within Mermaid for dev purposes:

image

The merge branch is coloured for main instead of new branch and still has that strange angle to it.

This despite the fact that there are comments in the code saying "Arrows going up take the color from the source branch" (and likewise "arrows going down take the color from the destination branch"). So it looks like something generally is not working as it should here.

@nirname
Copy link
Contributor

nirname commented Oct 6, 2023

@ijmitch Fair enough as a workaround but it is a compromise rather that an equal replacement because commits' order matters too.
@guypursey Could you copy and paste here the exact example of the code for that diagram you have provided? It is a simple one thus very representative

@ijmitch
Copy link
Author

ijmitch commented Oct 6, 2023

@nirname oh yes, I described it as a workaround since it's something acceptable for my current purpose which is purely illustrative for education about branching strategies.

I'm going to take a stab at what @guypursey 's diagram might be...

gitGraph
    commit
    branch newbranch
    commit
    commit
    checkout main
    commit
    commit
    merge newbranch
Loading

coded as

gitGraph
    commit
    branch newbranch
    commit
    commit
    checkout main
    commit
    commit
    merge newbranch

@guypursey
Copy link
Contributor

Pretty much 👍

It was the code below -- basically taken from https://github.com/mermaid-js/mermaid/blob/develop/demos/git.html, but I removed the line branch master and changed checkout master to checkout main as I wanted to test the hypothesis that the issue had something to do with having 3 branches (but it wasn't).

    gitGraph:
    options
    {
    "nodeSpacing": 50,
    "nodeRadius": 5
    }
    end
    commit
    branch newbranch
    checkout newbranch
    commit
    commit
    checkout main
    commit
    commit
    merge newbranch

@nirname
Copy link
Contributor

nirname commented Oct 6, 2023

Thank you all for detailed analysis. Could not have done it better

@guypursey
Copy link
Contributor

I've carried out some more analysis and I think I've almost fixed this... Colour seems to be working in my local branch and I'm almost there with the routing 🤞 I'll raise a pull request for review when I think I've done it

@guypursey
Copy link
Contributor

I think I've fixed this one now and have a PR (#4927) that is passing all tests, ready to be reviewed and approved/merged if all looks good

@guypursey
Copy link
Contributor

@ijmitch
Copy link
Author

ijmitch commented Oct 9, 2023

I just noticed that some of my examples where I want to demonstrate release branches by having them branch to lanes above main have some colour problems too.

%%{init: {'theme': 'base', 'gitGraph': {'mainBranchOrder': 2}} }%%
gitGraph
      commit
      commit
      branch release-1
      commit
      checkout main
      commit
      branch release-2
      commit
      checkout release-1
      commit
      checkout main
      commit
Loading

coded as:

%%{init: {'theme': 'base', 'gitGraph': {'mainBranchOrder': 2}} }%%
gitGraph
      commit
      commit
      branch release-1
      commit
      checkout main
      commit
      branch release-2
      commit
      checkout release-1
      commit
      checkout main
      commit

I checked both the current and preview live editors and they behaved the same, so if this is a bug then it looks different in nature to this one and I'd be happy to create a new issue for it.

@guypursey
Copy link
Contributor

@ijmitch Interesting... I think this might be separate bug personally.

I'd not only like to see the arrows coloured appropriately but also the branch curve flipped out the other way. For example, when you do the original branch to release-2, I'd expect the branch arrow to come straight out of main and then curve into release-2 branch. This is what happens when main is at the top, but the opposite happens when at the bottom.

Happy to work on it as a separate PR, given I've been digging around in that part of the code already.

@guypursey
Copy link
Contributor

@ijmitch Had a go at summarising my understanding of the bug as a separate issue. Hope looks correct: #4932

@guypursey
Copy link
Contributor

@nirname Not sure of etiquette re requesting approval but this one is ready for review now 🙂

@nirname
Copy link
Contributor

nirname commented Oct 22, 2023

@ijmitch Your coloring example is related to this issue for sure and seems that it is fixed in the upcoming request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Contributor needed Good first issue! Graph: Git Status: Approved Is ready to be worked on Type: Bug / Error Something isn't working or is incorrect
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants