sodapopcan / vim-twiggy

Git branch management for Vim
280 stars 15 forks source link

Error in branch parsing #27

Closed eglimi closed 5 years ago

eglimi commented 5 years ago

Thanks for making twiggy, which I find very useful!

Since 26225c (Refactor: Improve branch parsing), I get an error when invoking :Twiggy.

Error detected while processing function twiggy#Branch[14]..<SNR>85_Render[62]..<SNR>85_standard_view[10]..twiggy#get_branches[1]..<SNR>85__git_branch_vv[12]..<SNR>85_parse_branch:
line   13:
E684: list index out of range: 1
E116: Invalid arguments for function match
E15: Invalid expression: match(pieces[1], "HEAD detached at") >= 0
Error detected while processing function twiggy#Branch[14]..<SNR>85_Render:
line   62:
E712: Argument of extend() must be a List or Dictionary

My setup:

Please let me know if you need more information. I can also test changes here if that helps.

sodapopcan commented 5 years ago

Thanks for the report and your kind words :)

Unfortunately I am unable to produce this, though it seems the branch command isn't outputting as expected.

What version of git are you using?

What is the output of this command? (feel free to obfuscate branch names and commit messages):

git branch --list -vv --no-color --format=$'%(HEAD)\t\t%(refname:short)\t\t%(objectname:short)\t\t%(push:remotename)\t\t%(upstream:short)\t\t%(upstream:track)\t\t%(contents:subject)'

Do you have anything odd in your commit messages, like a \t or something?

eglimi commented 5 years ago

Thank you for the quick response!

The problem is the Git version. My version is 2.11.0 from Debian stable (Debian 9, stretch). This version does not include the --format option for the branch command. It seems this was implemented in Git 2.13.

An alternative command to consider would be git for-each-ref, which should work fine in older versions (although maybe not all field names are available). In any case, it works now for me. You can close the issue.

I can test the implementation in case you want to change it, just let me know.

sodapopcan commented 5 years ago

Ah that makes sense! I'd like to actually fix the issue so I'll leave the issue open.

I may take you up on the help offer, thanks a lot!

sodapopcan commented 5 years ago

I've pushed some changes that use for-each-ref instead of branch. I haven't actually tested on older versions but I generally like this much better. I really shouldn't be using porcelain commands anyway so this is a good step toward getting rid of them all.

Lemme know if you have any issues and thanks again!

eglimi commented 5 years ago

Thanks for the update. I agree that this looks much better.

Unfortunately, for the default Git version in Debian, it still doesn't work. The reason is that push:remotename is not available. For push, only :short, :track, and :trackshort are available, see man page for 2.12.

In addition, --no-color is also missing in this version.

I didn't look too closely into the code, but it seems that it still works when using push:short and removing --no-color. E.g.

function! s:_git_branch_vv(type) abort
  let branches = []
  let format = join([
        \ '%(HEAD)',
        \ '%(refname:short)',
        \ '%(objectname:short)',
        \ '%(push:short)',
        \ '%(upstream:short)',
        \ '%(upstream:track)',
        \ '%(contents:subject)',
        \ ], "\t\t")
  for branch in s:git_cmd('for-each-ref refs/' . a:type . " --format=$'".format."'", 0)
    call add(branches, s:parse_branch(branch, a:type))
  endfor

  return branches
endfunction

If you think that would work, I can open a PR with these changes.

sodapopcan commented 5 years ago

So as it turns out, the %(push) fragment isn't even used 😬 So it can just be be removed. As for --no-color, that can certainly be removed. I noticed it wasn't needed but left it there for "safety".

I'm happy to let you do the PR, but please just remove the %(push) part entirely. This means that any index greater than 3 of the oh-so-nicely named pieces array needs to be manually decremented. It's not a lot of places (I'm working on a refactoring for all this). Lemme know if you want me to just do it!

eglimi commented 5 years ago

It's fine if you do the changes, since you are working on a refactoring anyway.

sodapopcan commented 5 years ago

Done! Thanks again for your help with this :)

eglimi commented 5 years ago

That was quick :) Everything works now, many thanks!