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

Fix 149, commit append to the last line #150

Merged

Conversation

iaia
Copy link
Contributor

@iaia iaia commented Oct 31, 2017

ref. #149

When set commit at the end of default section, append to the last line.

@jreybert
Copy link
Owner

jreybert commented Nov 5, 2017

Just to let you know, I watched your PR the first day, but I did not found time to test it.

Seems quite good to me. I just wonder the w$ pattern, maybe the pattern end of filewould be better (i can't remember right now the exact syntax of this pattern).

Thanks for the PR anyway!

@iaia iaia force-pushed the fix_149_commit_at_the_end_of_default_sections branch from 02451a5 to dc18c49 Compare November 5, 2017 13:11
@iaia
Copy link
Contributor Author

iaia commented Nov 5, 2017

Thank you for reviewing.
What you are saying is maybe $EOF? I fixed. please review again.

@jreybert
Copy link
Owner

jreybert commented Nov 6, 2017

End of file vim pattern is \%$ (see :help %$). $EOF doe snot exists in vim.

@jreybert jreybert added this to In progress in vimagit dev board Nov 6, 2017
Copy link
Owner

@jreybert jreybert left a comment

Choose a reason for hiding this comment

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

you spotted the right codelines to fix the bug, but the fix is wrong.

The usage of line() makes no sense here, you don't want to check against the content of the last line (which is certainly empty line), because you could have empty line in your commit message. Instead, use the pattern \%$, which is regex pattern for end of file.

Others comments are mainly cosmetics, but I would appreciate them too.

plugin/magit.vim Outdated
@@ -293,7 +293,13 @@ endfunction
function! s:mg_get_commit_msg(...)
let commit_section_pat_start='^'.g:magit_sections.commit.'$'
" Get next section pattern with g:magit_default_sections order
let commit_section_pat_end='^'.g:magit_sections[g:magit_default_sections[match(g:magit_default_sections, 'commit')+1]].'$'
if ( match(g:magit_default_sections, 'commit')+1 >= len(g:magit_default_sections) )
Copy link
Owner

Choose a reason for hiding this comment

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

use intermediate variable to store match(g:magit_default_sections, 'commit') result
check if this position is equal to len + 1

plugin/magit.vim Outdated
@@ -293,7 +293,13 @@ endfunction
function! s:mg_get_commit_msg(...)
let commit_section_pat_start='^'.g:magit_sections.commit.'$'
" Get next section pattern with g:magit_default_sections order
let commit_section_pat_end='^'.g:magit_sections[g:magit_default_sections[match(g:magit_default_sections, 'commit')+1]].'$'
if ( match(g:magit_default_sections, 'commit')+1 >= len(g:magit_default_sections) )
let commit_position = line("$EOF")
Copy link
Owner

Choose a reason for hiding this comment

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

do not use intermediate variable for this, store directly in commit_section_pat_end

use \%$ pattern, without line()

plugin/magit.vim Outdated
if ( match(g:magit_default_sections, 'commit')+1 >= len(g:magit_default_sections) )
let commit_position = line("$EOF")
else
let commit_position = g:magit_sections[g:magit_default_sections[match(g:magit_default_sections, 'commit')+1]]
Copy link
Owner

Choose a reason for hiding this comment

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

use intermediate variable declared before

keep ^ and $ here

@iaia iaia force-pushed the fix_149_commit_at_the_end_of_default_sections branch from dc18c49 to 3c06923 Compare November 6, 2017 13:31
@iaia
Copy link
Contributor Author

iaia commented Nov 6, 2017

@jreybert sorry, i am not good at writing vimscript.
I rewrote it. please review again. thank you.

Copy link
Owner

@jreybert jreybert left a comment

Choose a reason for hiding this comment

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

Excellent! Just one last thing and you're good.

plugin/magit.vim Outdated
if ( commit_position + 1 == len(g:magit_default_sections) )
let commit_section_pat_end='\%$'
else
let commit_section_pat_end='^'.g:magit_sections[g:magit_default_sections[match(g:magit_default_sections, 'commit')+1]].'$'
Copy link
Owner

Choose a reason for hiding this comment

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

Reuse commit_position here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed!

@iaia iaia force-pushed the fix_149_commit_at_the_end_of_default_sections branch from 3c06923 to 9e3d348 Compare November 6, 2017 15:19
Copy link
Owner

@jreybert jreybert left a comment

Choose a reason for hiding this comment

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

Perfect.

I am sorry, two last things I did not noticed the first time:

@iaia iaia force-pushed the fix_149_commit_at_the_end_of_default_sections branch from 9e3d348 to 76350ab Compare November 6, 2017 22:38
@iaia
Copy link
Contributor Author

iaia commented Nov 6, 2017

oops! sorry, i deleted .development.vim. and change commit message.

@jreybert jreybert merged commit bb3c480 into jreybert:master Nov 7, 2017
vimagit dev board automation moved this from In progress to Done Nov 7, 2017
@jreybert
Copy link
Owner

jreybert commented Nov 7, 2017

Excellent, thanks for the PR and for all your changes through the review process!

@iaia
Copy link
Contributor Author

iaia commented Nov 7, 2017

Thanks for the merge!

@iaia iaia deleted the fix_149_commit_at_the_end_of_default_sections branch November 7, 2017 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants