merging-to-current enhancement

[phmarek]

I've got a remote branch that I want to merge (1 commit, ff possible).

After choosing the two lines and pressing 'm' I get asked 3 questions. This is especially bothersome if one of the points has a few branches defined on it - then I have to read the list at the bottom, find the one I want, and get the correct ID; in about 10 tests I've err'ed at least 3 times (mixed start and end point, choose wrong branch).

How about optimizing for the trivial case (merging onto the current branch), and allow some hotkey to try a merge without asking that much? Sadly both "M" and "m" are used for normal mode, although I think "m" could be reused -- or at least switch to that if only part of a single line is marked visually.

Ie. I'm standing on a (syntax colored) branchname "remote/branch" - pressing "m" (or, alternatively, visually marking that branch name and then pressing "m") should just call "git merge " and be done.

Thanks a lot,

Phil

[gregsexton]

Hi Phil,

Thanks for the suggestion. I agree that the merge UX isn't the best. I just threw it in as an added extra when developing the plugin. My goal was really for a repo viewer rather than any kind of workflow tool.

I do like your suggestion though. I'll add it to my backlog and have a look at it when I get some time to revisit this project. Of course, if you're in a hurry, I'll accept patches! ;)

Thanks,

Greg

[phmarek]

Oh, come on, it wasn't that hard!!

diff --git plugin/gitv.vim plugin/gitv.vim
index 1a01b27..ce68ce6 100644
--- plugin/gitv.vim
+++ plugin/gitv.vim
@@ -472,6 +472,7 @@ fu! s:SetupMappings() "{{{
     vnoremap <buffer> <silent> S :call <SID>StatGitvCommit()<cr>

     vnoremap <buffer> <silent> m :call <SID>MergeBranches()<cr>
+    nnoremap <buffer> <silent> m :call <SID>MergeToCurrent()<cr>

     "movement
     nnoremap <buffer> <silent> x :call <SID>JumpToBranch(0)<cr>
@@ -895,6 +896,16 @@ fu! s:DiffGitvCommit() range "{{{
     endif
     call s:MoveIntoPreviewAndExecute("Gdiff " . shalast, a:firstline != a:lastline)
 endf "}}}
+fu! s:MergeToCurrent() "{{{
+    let refs = s:GetGitvRefs(".")
+    call filter(refs, 'v:val !=? "HEAD"')
+    if len(refs) < 1
+        echom 'Missing a merge point.'
+        return
+    endif
+    let target = refs[0]
+    call s:PerformMerge("HEAD", target, 0)
+endfu "}}}
 fu! s:MergeBranches() range "{{{
     if a:firstline == a:lastline
         echom 'Already up to date.'

[gregsexton]

Ha ha! Fair enough. :)

Change the key binding to <leader>m and submit it as a pull request and I'll happily document and merge in. Or would you rather I just grabbed your changes from the diff?

Thanks,

Greg

[phmarek]

Please just take the diff.

Why not use "m", why do you want the here? Do you think it's likely that someone wants to mark a position here? Hmmm, perhaps, yes.

Thanks a lot!

[gregsexton]

Just incorporated this change and pushed it. Hopefully this solves your problem. I went with <leader>m as marks are occasionally useful.

[phmarek]

Hmmm, why asking for fast-forward? I thought that git would do that automatically, if possible, so I don't see any need for interrupting the workflow here.

[gregsexton]

git will fast-forward automatically if possible but you may not wish for it to do this. I often work on 'feature branches' and never wish for these to be merged using a fast-forward.

If this is an issue, you could probably override the mapping in ~/.vim/after/ftplugin/gitv.vim. I guess you can answer the question automatically. Or I could create a global var option to allow you to configure being prompted about merge strategy?

[phmarek]

Well, how about making the function take a "ff" argument, and ask the user if it's some special value?

[phmarek]

The feature is broken now, because s:GetGitvRefs() doesn't work anymore; it would only match HEAD and branch names in (), not a revision in [] at the end of the line.

Typically you want to cherry-pick single revisions that are not at the head of some branch.

[phmarek]

For reference, I'm using this:

fu! s:MergeToCurrent()
    let target = matchlist(getline("."), "\\[\\(\\w\\+\\)\\]$")

    exec 'Git cherry-pick ' . target[1]
endfu "}}}

[rbong]

@phmarek you should just be able to use cp right now if you're just cherry picking.

I'm not sure that merge works for everyone's use case right now. There may be users like @phmarek who always wish to fast-forward. There may be users like @gregsexton who wish to customize fast-forward. And what of merging with different strategies?

I'm not comfortable adding a bunch of different bindings for different merge use cases given that the bindings just blew up with bisect and rebase added. What happens when we start getting into adding more arguments for that? I think that just an option that says "do not ask me for --no-ff" is not enough.

I also don't want to break existing functionality.

Therefore, I am adding an option, g:Gitv_MergeArgs. If users wish to disable the default behaviour, they can set it to an empty string. If they want to use the default behaviour, they can simply not define it. If they wish to do different strategies they can. The only downside is if they wish to use some combination of the default behaviour and the MergeArgs behaviour, they must define it themselves. More information has been added to the documentation.