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

Add map with branches and dimensions to diagram (fixes #155) #156

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

abitrolly
Copy link
Contributor

This adds a minimap with dimensions and branch names to diagram output to clarify what goes where,

Key:                                                    Map:
  * = merge done manually                                 o-0-1-2..-131  upstream/master
  . = merge done automatically                              1
  # = conflict that is currently blocking progress          ..
  @ = merge was blocked but has been resolved               11
  ? = no merge recorded                                    eliaszs/feature/individual_account
  |,-,+ = rectangles forming current merge frontier

@mhagger
Copy link
Owner

mhagger commented Jul 13, 2020

This would be helpful, but what would you think about adding these numbers around the boundaries of the diagram output itself? For example, maybe something like

                1    1    2    2 2
      0    5    0    5    0    5 7
      |    |    |    |    |    | |
  0 - **************************** - upstream/master
      *??.?????|??????????????????
      *??.-----+??????????????????
      *??|#???????????????????????
      *??|????????????????????????
  5 - *??|????????????????????????
      *--+????????????????????????
      *???????????????????????????
      *???????????????????????????
      *???????????????????????????
 10 - *???????????????????????????
      *???????????????????????????
      *???????????????????????????
 13 - *???????????????????????????
      |
 eliaszs/feature/individual_account

or

      0    5   10   15   20   25 27
      |    |    |    |    |    | |
  0 - **************************** - upstream/master
      *??.?????|??????????????????
      *??.-----+??????????????????
      *??|#???????????????????????
      *??|????????????????????????
  5 - *??|????????????????????????
      *--+????????????????????????
      *???????????????????????????
      *???????????????????????????
      *???????????????????????????
 10 - *???????????????????????????
      *???????????????????????????
      *???????????????????????????
 13 - *???????????????????????????
      |
 eliaszs/feature/individual_account

If every fifth commit, plus the last one in each direction, has its number displayed alongside the diagram, then there's plenty of room for 9999 commits on the horizontal axis. On the vertical axis, one could either reserve enough room for, say, 4-digit numbers, or dynamically determine the maximum number length needed and reserve that much space.

It's trickier to implement but I think it would be more intuitive and make it easy to read numbers off of the diagram.

@abitrolly
Copy link
Contributor Author

I am interested in rebases. I don't really get the "merge frontiers" graph. If I understand it right, the rebase process is pushing the branch from left to right towards master end. Because of simplicity the "frontier" can be omitted, and middle pieces of the diagram that don't touch the current merge can be cut out. The ideal diagram looks like this.

           5   20   25   30   35 37
           |    |    |    |    | |
    o-0123456..9012345678901234567- upstream/master
      .1?????..???????????????????
      .2?????..???????????????????
      .3?????..???????????????????
      .4?????..???????????????????
    5-.5?????..???????????????????
      .6?????..???????????????????
      7??????..???????????????????
      8??????..???????????????????
      ............................
      2??????..???????????????????
   23-3??????..???????????????????
      |
     eliaszs/feature/individual_account

And maybe replace question marks with spaces.

@mhagger
Copy link
Owner

mhagger commented Jul 14, 2020

I don't really get the "merge frontiers" graph.

Basically, any merge to the left and up from the "merge frontier" is assumed to be doable without conflicts (given the conflicts that have already been resolved by the user), whereas the merges below and to the right of it are still to be processed. I personally find the merge frontier useful even in the case of rebases.

If I understand it right, the rebase process is pushing the branch from left to right towards master end.

Kindof. It currently really follows the same algorithm as for merges, though that could be simplified, as alluded to here.

I suggest keeping the issues separate: (1) adding some kind of key or axis labels for the numbers (e.g., this PR), and (2) other proposed changes to the diagram output. If you'd like (2), I suggest that you open another issue and paste some mockups of what you propose.

@abitrolly
Copy link
Contributor Author

abitrolly commented Jul 14, 2020

I like graphs in #101. They clarify things a lot. The "merge frontier" shows how far a certain commit can be pushed left without any commits down attached to it. I would name it confict frontier.

Speaking of frontier.. although I moved the conditional block that prints the legend for it, there is really no way to make this option false.

git-imerge/gitimerge.py

Lines 3698 to 3699 in 4b6550d

if not (options.commits or options.frontier):
options.frontier = True


Back to PR, I see three threads here.

(0) adding "minimap" to the right of legend
(1) adding numbers and branch names to existing output
(3) simplifying diagram output for rebase

I would just add the minimap that gives information about numbers and branches. With the rebase over 131 commits in master the main diagram is already unreadable. because it doesn't fit the screen. As I can't really read the graph, the minimap becomes a substitution for missing status command.

@mhagger
Copy link
Owner

mhagger commented Jul 14, 2020

With the rebase over 131 commits in master the main diagram is already unreadable. because it doesn't fit the screen.

If you pipe it to less -S then you can pan left and right using the arrow keys (at least with GNU less). It's still not great, but at least semi-readable.

@abitrolly
Copy link
Contributor Author

The full command would be diagram --color | less -S.

Copy link
Owner

@mhagger mhagger left a comment

Choose a reason for hiding this comment

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

I think this would be an improvement, but I think axes would be even nicer. What do you think of #157?

gitimerge.py Outdated Show resolved Hide resolved
gitimerge.py Outdated
'Key: Map:\n'
)
sys.stdout.write(
' * = merge done manually o-0-1-2..-%(len1)d %(tip1)s\n'
Copy link
Owner

Choose a reason for hiding this comment

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

Hmm, this is going to look strange if there are fewer than 3 commits on the horizontal axis or fewer than 2 on the vertical axis; for example:

  o-0-1-2..2

Even this looks a little bit weird (though acceptable):

  o-0-1-2..3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. This needs to be calculated. I may be able to do this in a few hours. Although I may go offline for a week or two at any moment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mhagger fixed in the last commit.

       Map:
         o-0-1  single-dockerfile
           1
          ..
         235
          upstream/master

abitrolly and others added 2 commits March 31, 2021 15:52
Key:                                                    Map:
  * = merge done manually                                 o-0-1-2..-131  upstream/master
  . = merge done automatically                              1
  # = conflict that is currently blocking progress          ..
  @ = merge was blocked but has been resolved               11
  ? = no merge recorded                                    eliaszs/feature/individual_account
  |,-,+ = rectangles forming current merge frontier
Co-authored-by: Michael Haggerty <[email protected]>
@abitrolly
Copy link
Contributor Author

abitrolly commented Mar 31, 2021

Rebased only. Haven't fixed the label yet. Need some example repo to test with commands how to get it into the proper "in the process" state.

   Map:
     o-0-1  single-dockerfile
       1
      ..
     235
      upstream/master

And if there are more than 4.

    Map:
      o-0-1-2..4  single-dockerfile
        1
       ..
      235
       upstream/master
@abitrolly abitrolly requested a review from mhagger March 31, 2021 14:59
@abitrolly
Copy link
Contributor Author

Current render.

~/git-imerge/gitimerge.py diagram
       0    5   10   15   20   25   30 32
       |    |    |    |    |    |    | |
   0 - ********************************* - origin/master
       *??.???.*??????.????????????????|
       *??.???..??????.????????????????|
       *??......??????..---------------+
       *??.*.*..??????.*#???????????????
   5 - *---------------+????????????????
       |
     for-non-static

Key:                                                    Map:
  * = merge done manually                                 o-0-1-2..32  origin/master
  . = merge done automatically                              1
  # = conflict that is currently blocking progress         ..
  @ = merge was blocked but has been resolved               5
  ? = no merge recorded                                    for-non-static
  |,-,+ = rectangles forming current merge frontier

@waldyrious
Copy link
Contributor

Just a small style suggestion: how about "rotating" the .. in the vertical part of the map?

Map:
  o-0-1-2..32  origin/master
    1
    :
    5
   for-non-static

@abitrolly
Copy link
Contributor Author

@waldyrious yep, looks a little better now. )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants