issue to fix

• Sometimes a bookmark name is detected as if it was commit_id #228

root cause of bug

• current CommitId is identify with below logic
• which leads to any hex like string bookmarks like ace to be mistaken as CommitId
  

  jjui/internal/parser/row_line.go

  Lines 31 to 39 in 360d612

  	func (gr *GraphRowLine) FindPossibleCommitIdIdx(after int) int {
  	for i := after; i < len(gr.Segments); i++ {
  	segment := gr.Segments[i]
  	if isHexLike(segment.Text) {
  	return i
  	}
  	}
  	return -1
  	}

  
  

  jjui/internal/parser/row.go

  Lines 199 to 206 in 360d612

  	func isHexLike(s string) bool {
  	for _, r := range s {
  	if !unicode.Is(unicode.Hex_Digit, r) {
  	return false
  	}
  	}
  	return true
  	}

What are you planning to implement?

• A different approach to get CommitId for jj log output

Proposed Approach

approach 1

• draft implementation see here

• Simplify logic to get CommitId

  • from my own experiments, CommitId is always the second last element in GraphRowLine.Segments
  • hence we could simply take the second last element of GraphRowLine.Segments as CommitId

• this is failing these two tests (see the tests removed in the commit):

  • Revision with extremely long bookmarks (according to the test, CommitId would be on a new line)
  • Revision is single line with description (unsure about this test)

• however, i wasn't able to recreate these two tests with jjui

approach 2

• use templated jj log to get CommitId
  • with existing jj template, we could get CommitId and ChangeId without any complex string operation
• below command works decently well to serve this purpose

jj log -T 'builtin_log_compact ++ change_id.shortest() ++ " " ++ commit_id.shortest()'

• which gives something like:

@  rwzrmrvk yc@yingchaoooo.com 13 minutes ago a3754fb0
│  (empty) (no description set)
│  r a    # this is the new line generated by the additional 
○  nlyyqkyr yc@yingchaoooo.com 13 minutes ago git_head() b1deea63
│  (empty) (no description set)
│  nl b

• note that: │ r a is the new line generated by the additional change_id.shortest() ++ " " ++ commit_id.shortest() template input
• we can easily get ChangeId and CommitId and not trim out the new line
• this approach a good strategy to get the IDs natively from jj, which is definitely very nice as it's error free!
• it takes a bigger refactor, but i think it's worth the work and will pay off long term if we have more functionalities depending on the IDs

Breaking Changes

Would this implementation introduce any breaking changes? If so, describe them and any migration strategy.

• for now, only ace jump depends on CommitId

Questions for Discussion

• approach 1 is a quick fix targeting the issue Sometimes a bookmark name is detected as if it was commit_id #228
• approach 2 is a better way to address the root cause, use jj's native response which is more consistent and error free than the original isHexLike string operation or getting second last element in approach 1

@idursun what do you think?
@vic would love to hear your thoughts too!