{"thread":{"id":"30087","subject":"[PATCH 1/2] mergetools: split config files for vim and gvim","startedAt":"2012-03-28T19:58:12Z","lastAt":"2012-03-28T21:02:58Z","messageCount":4,"participants":["Tim Henigan","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"187986","messageId":"1332964693-4058-1-git-send-email-tim.henigan@gmail.com","threadId":"30087","inReplyTo":null,"subject":"[PATCH 1/2] mergetools: split config files for vim and gvim","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-03-28T19:58:12Z","receivedAt":"2012-03-28T19:58:12Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"In ae69fd0 (mergetool-lib: combine vimdiff and gvimdiff run blocks),\nthe config files for these two tools were combined since they were\nnearly identical.  This remains true, but having a single config\nfile for both makes it difficult to test that each is installed\nand capable of running.\n\nThis commit splits the two config files. Now 'vim' and 'gvim'\nfollow the pattern used by all the other diff/merge tools:\n  - There is a single file per tool\n  - To use the tool, 'diff.tool' (or other similar option) may be\n    set to the exact name of the file.\n\nSigned-off-by: Tim Henigan <tim.henigan@gmail.com>\n---\n\nThis series should apply cleanly on the current master, but it was\ndeveloped on top of the series that implements the 'difftool --dir-diff'\noption (currently th/difftool-diffall branched from pu).\n\nThis patch does some cleanup needed to be compatible with the new\n'--dir-diff' option for 'difftool'.\n\nOne side-effect of this change which may be a problem is that we\nlose support for the 'vimdiff2' and 'gvimdiff2' tools that were\ncreated in 0008669 (mergetool-lib: make the three-way diff the\ndefault for vim/gvim).  The 2-panel options were not advertised in\nany way, so I don't know if it is important to keep them.\n\n\n git-mergetool--lib.sh |    9 +--------\n mergetools/gvim       |   21 +++++++++++++++++++++\n mergetools/vim        |   41 +++++++++--------------------------------\n 3 files changed, 31 insertions(+), 40 deletions(-)\n create mode 100644 mergetools/gvim\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex ed630b2..89b16dc 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -44,14 +44,7 @@ valid_tool () {\n }\n \n setup_tool () {\n-\tcase \"$1\" in\n-\tvim*|gvim*)\n-\t\ttool=vim\n-\t\t;;\n-\t*)\n-\t\ttool=\"$1\"\n-\t\t;;\n-\tesac\n+\ttool=\"$1\"\n \tmergetools=\"$(git --exec-path)/mergetools\"\n \n \t# Load the default definitions\ndiff --git a/mergetools/gvim b/mergetools/gvim\nnew file mode 100644\nindex 0000000..b746e6f\n--- /dev/null\n+++ b/mergetools/gvim\n@@ -0,0 +1,21 @@\n+diff_cmd () {\n+\t\"$merge_tool_path\" -R -f -d \\\n+\t\t-c 'wincmd l' -c 'cd $GIT_PREFIX' \"$LOCAL\" \"$REMOTE\"\n+}\n+\n+merge_cmd () {\n+\ttouch \"$BACKUP\"\n+\tif $base_present\n+\tthen\n+\t\t\"$merge_tool_path\" -f -d -c 'wincmd J' \\\n+\t\t\t\"$MERGED\" \"$LOCAL\" \"$BASE\" \"$REMOTE\"\n+\telse\n+\t\t\"$merge_tool_path\" -f -d -c 'wincmd l' \\\n+\t\t\t\"$LOCAL\" \"$MERGED\" \"$REMOTE\"\n+\tfi\n+\tcheck_unchanged\n+}\n+\n+translate_merge_tool_path() {\n+\techo gvim\n+}\ndiff --git a/mergetools/vim b/mergetools/vim\nindex 619594a..6817708 100644\n--- a/mergetools/vim\n+++ b/mergetools/vim\n@@ -1,44 +1,21 @@\n diff_cmd () {\n-\tcase \"$1\" in\n-\tgvimdiff|vimdiff)\n-\t\t\"$merge_tool_path\" -R -f -d \\\n-\t\t\t-c 'wincmd l' -c 'cd $GIT_PREFIX' \"$LOCAL\" \"$REMOTE\"\n-\t\t;;\n-\tgvimdiff2|vimdiff2)\n-\t\t\"$merge_tool_path\" -R -f -d \\\n-\t\t\t-c 'wincmd l' -c 'cd $GIT_PREFIX' \"$LOCAL\" \"$REMOTE\"\n-\t\t;;\n-\tesac\n+\t\"$merge_tool_path\" -R -f -d \\\n+\t\t-c 'wincmd l' -c 'cd $GIT_PREFIX' \"$LOCAL\" \"$REMOTE\"\n }\n \n merge_cmd () {\n \ttouch \"$BACKUP\"\n-\tcase \"$1\" in\n-\tgvimdiff|vimdiff)\n-\t\tif $base_present\n-\t\tthen\n-\t\t\t\"$merge_tool_path\" -f -d -c 'wincmd J' \\\n-\t\t\t\t\"$MERGED\" \"$LOCAL\" \"$BASE\" \"$REMOTE\"\n-\t\telse\n-\t\t\t\"$merge_tool_path\" -f -d -c 'wincmd l' \\\n-\t\t\t\t\"$LOCAL\" \"$MERGED\" \"$REMOTE\"\n-\t\tfi\n-\t\t;;\n-\tgvimdiff2|vimdiff2)\n+\tif $base_present\n+\tthen\n+\t\t\"$merge_tool_path\" -f -d -c 'wincmd J' \\\n+\t\t\t\"$MERGED\" \"$LOCAL\" \"$BASE\" \"$REMOTE\"\n+\telse\n \t\t\"$merge_tool_path\" -f -d -c 'wincmd l' \\\n \t\t\t\"$LOCAL\" \"$MERGED\" \"$REMOTE\"\n-\t\t;;\n-\tesac\n+\tfi\n \tcheck_unchanged\n }\n \n translate_merge_tool_path() {\n-\tcase \"$1\" in\n-\tgvimdiff|gvimdiff2)\n-\t\techo gvim\n-\t\t;;\n-\tvimdiff|vimdiff2)\n-\t\techo vim\n-\t\t;;\n-\tesac\n+\techo vim\n }\n-- \n1.7.10.rc2.21.g8cb1a\n"},{"id":"187987","messageId":"1332964693-4058-2-git-send-email-tim.henigan@gmail.com","threadId":"30087","inReplyTo":"1332964693-4058-1-git-send-email-tim.henigan@gmail.com","subject":"[PATCH 2/2] mergetools: fail if display needed but not present","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-03-28T19:58:13Z","receivedAt":"2012-03-28T19:58:13Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"Prior to this commit, if 'git mergetool' or 'git difftool' were run in a\nterminal-only session, they might still try to open a tool that required\na windowed environment.\n\nThis commit teaches 'git-mergetool--lib.sh' to test for the presence of\na display prior to opening tools that require one.\n\nNOTE: The DISPLAY variable is not set in msysgit or cygwin Git.  In the\nabsence of that variable, the script assumes that a window environment\nis always available when running msysgit or cygwin.\n\nSigned-off-by: Tim Henigan <tim.henigan@gmail.com>\n---\n\nOnly 2 of the current diff/merge tools (vim and emerge) can operate\nwithout a display.\n\nThe assumption that a display is always available on msys or cygwin is\nprobably not optimal.  However, I was not able to find a better work-\naround.  It also matches the previous behavior that did not test for\nthe DISPLAY prior to opening the tool.\n\n\n git-mergetool--lib.sh |   23 +++++++++++++++++++++++\n mergetools/defaults   |    4 ++++\n mergetools/emerge     |    4 ++++\n mergetools/vim        |    4 ++++\n 4 files changed, 35 insertions(+)\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 89b16dc..75c8b11 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -12,6 +12,19 @@ translate_merge_tool_path () {\n \techo \"$1\"\n }\n \n+display_available() {\n+\tif test -n \"$DISPLAY\"\n+\tthen\n+\t\treturn 0\n+\tfi\n+\n+\tcase \"$(uname -s)\" in\n+\tMINGW*|CYGWIN*) return 0; break;;\n+\tesac\n+\n+\treturn 1\n+}\n+\n check_unchanged () {\n \tif test \"$MERGED\" -nt \"$BACKUP\"\n \tthen\n@@ -190,6 +203,16 @@ get_merge_tool_path () {\n \t\t\t \"'$merge_tool_path'\"\n \t\texit 1\n \tfi\n+\tif ! display_available\n+\tthen\n+\t\tif tool_requires_display\n+\t\tthen\n+\t\t\techo >&2 \"The $TOOL_MODE tool $merge_tool cannot\"\\\n+\t\t\t\"be run from a terminal-only session.  A window\"\\\n+\t\t\t\"environment is required.\"\n+\t\t\texit 1\n+\t\tfi\n+\tfi\n \techo \"$merge_tool_path\"\n }\n \ndiff --git a/mergetools/defaults b/mergetools/defaults\nindex 1d8f2a3..f150227 100644\n--- a/mergetools/defaults\n+++ b/mergetools/defaults\n@@ -7,6 +7,10 @@ can_diff () {\n \treturn 0\n }\n \n+tool_requires_display() {\n+\treturn 0\n+}\n+\n diff_cmd () {\n \tmerge_tool_cmd=\"$(get_merge_tool_cmd \"$1\")\"\n \tif test -z \"$merge_tool_cmd\"\ndiff --git a/mergetools/emerge b/mergetools/emerge\nindex f96d9e5..c7eb80b 100644\n--- a/mergetools/emerge\n+++ b/mergetools/emerge\n@@ -1,3 +1,7 @@\n+tool_requires_display () {\n+\treturn 1\n+}\n+\n diff_cmd () {\n \t\"$merge_tool_path\" -f emerge-files-command \"$LOCAL\" \"$REMOTE\"\n }\ndiff --git a/mergetools/vim b/mergetools/vim\nindex 6817708..ffd9fd6 100644\n--- a/mergetools/vim\n+++ b/mergetools/vim\n@@ -1,3 +1,7 @@\n+tool_requires_display () {\n+\treturn 1\n+}\n+\n diff_cmd () {\n \t\"$merge_tool_path\" -R -f -d \\\n \t\t-c 'wincmd l' -c 'cd $GIT_PREFIX' \"$LOCAL\" \"$REMOTE\"\n-- \n1.7.10.rc2.21.g8cb1a\n"},{"id":"188003","messageId":"7vr4wctpl7.fsf@alter.siamese.dyndns.org","threadId":"30087","inReplyTo":"1332964693-4058-1-git-send-email-tim.henigan@gmail.com","subject":"Re: [PATCH 1/2] mergetools: split config files for vim and gvim","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-28T20:53:08Z","receivedAt":"2012-03-28T20:53:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tim Henigan <tim.henigan@gmail.com> writes:\n\n> One side-effect of this change which may be a problem is that we\n> lose support for the 'vimdiff2' and 'gvimdiff2' tools that were\n> created in 0008669 (mergetool-lib: make the three-way diff the\n> default for vim/gvim).  The 2-panel options were not advertised in\n> any way, so I don't know if it is important to keep them.\n\nBy default, anything we support is important unless there is a sound\njustification to say otherwise.  I think they were kept for people who\nwere already used to the 2-pane version when 3-pane one was introduced.\n\nBut I will not be a good judge for this particular case, as I do not use\nvim nor gvim for merge resolution.  Davidd?\n"},{"id":"188005","messageId":"7vmx70tp4t.fsf@alter.siamese.dyndns.org","threadId":"30087","inReplyTo":"1332964693-4058-2-git-send-email-tim.henigan@gmail.com","subject":"Re: [PATCH 2/2] mergetools: fail if display needed but not present","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-28T21:02:58Z","receivedAt":"2012-03-28T21:02:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tim Henigan <tim.henigan@gmail.com> writes:\n\n> Prior to this commit, if 'git mergetool' or 'git difftool' were run in a\n> terminal-only session, they might still try to open a tool that required\n> a windowed environment.\n\nWhen you say \"The command does X.  X is bad for such and such reasons.\nMake it do Y instead, because it is nicer for such and such reasons.\",\neverybody would understand that the command does X without your patch.\nMaybe it is just me, but I find the phrases like \"Prior to this commit\" or\n\"Currently\" somewhat irritating.\n\n> This commit teaches 'git-mergetool--lib.sh' to test for the presence of\n> a display prior to opening tools that require one.\n\nHrm, why not make it more general, so that mergetool--lib does not have to\nknow anything about DISPLAY but allow the tool scripts to inspect their\nenvironment and make their own decision, i.e. after loading the tool\nscriptlet, the caller can call \"can_run [--quiet]\" and the scriptlet can\neither return 0/1 and optionally issue the help message?\n"}]}