{"thread":{"id":"17292","subject":"[PATCH] git mergetool: Don't repeat merge tool candidates","startedAt":"2009-01-21T20:24:02Z","lastAt":"2009-01-23T23:12:45Z","messageCount":5,"participants":["Johannes Gilger","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"101446","messageId":"1232569442-12480-1-git-send-email-heipei@hackvalue.de","threadId":"17292","inReplyTo":null,"subject":"[PATCH] git mergetool: Don't repeat merge tool candidates","fromName":"Johannes Gilger","fromEmail":"heipei@hackvalue.de","sentAt":"2009-01-21T20:24:02Z","receivedAt":"2009-01-21T20:24:02Z","isPatch":true,"sender":{"key":"heipei@hackvalue.de","avatar":"https://avatars.githubusercontent.com/u/6072?v=4"},"body":"git mergetool listed some candidates for mergetools twice, depending on\nthe environment.\n\nSigned-off-by: Johannes Gilger \n---\n git-mergetool.sh | 13 +++++--------\n 1 files changed, 5 insertions(+), 8 deletions(-)\n\ndiff --git a/git-mergetool.sh b/git-mergetool.sh\nindex 00e1337..8f09e4a 100755\n--- a/git-mergetool.sh\n+++ b/git-mergetool.sh\n@@ -390,21 +390,18 @@ fi\n \n if test -z \"$merge_tool\" ; then\n if test -n \"$DISPLAY\"; then\n- merge_tool_candidates=\"kdiff3 tkdiff xxdiff meld gvimdiff\"\n if test -n \"$GNOME_DESKTOP_SESSION_ID\" ; then\n- merge_tool_candidates=\"meld $merge_tool_candidates\"\n- fi\n- if test \"$KDE_FULL_SESSION\" = \"true\"; then\n- merge_tool_candidates=\"kdiff3 $merge_tool_candidates\"\n+ merge_tool_candidates=\"meld kdiff3 tkdiff xxdiff gvimdiff\"\n+ else\n+ merge_tool_candidates=\"kdiff3 tkdiff xxdiff meld gvimdiff\"\n fi\n fi\n if echo \"${VISUAL:-$EDITOR}\" | grep 'emacs' > /dev/null 2>&1; then\n- merge_tool_candidates=\"$merge_tool_candidates emerge\"\n+ merge_tool_candidates=\"$merge_tool_candidates emerge opendiff vimdiff\"\n fi\n if echo \"${VISUAL:-$EDITOR}\" | grep 'vim' > /dev/null 2>&1; then\n- merge_tool_candidates=\"$merge_tool_candidates vimdiff\"\n+ merge_tool_candidates=\"$merge_tool_candidates vimdiff opendiff emerge\"\n fi\n- merge_tool_candidates=\"$merge_tool_candidates opendiff emerge vimdiff\"\n echo \"merge tool candidates: $merge_tool_candidates\"\n for i in $merge_tool_candidates; do\n init_merge_tool_path $i\n-- \n1.6.1.40.g8ea6a\n"},{"id":"101631","messageId":"7v7i4m1lq4.fsf@gitster.siamese.dyndns.org","threadId":"17292","inReplyTo":"1232569442-12480-1-git-send-email-heipei@hackvalue.de","subject":"Re: [PATCH] git mergetool: Don't repeat merge tool candidates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-23T08:16:03Z","receivedAt":"2009-01-23T08:16:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Gilger writes:\n\n> git mergetool listed some candidates for mergetools twice, depending on\n> the environment.\n>\n> Signed-off-by: Johannes Gilger \n> ---\n> git-mergetool.sh | 13 +++++--------\n> 1 files changed, 5 insertions(+), 8 deletions(-)\n>\n> diff --git a/git-mergetool.sh b/git-mergetool.sh\n> index 00e1337..8f09e4a 100755\n> --- a/git-mergetool.sh\n> +++ b/git-mergetool.sh\n> @@ -390,21 +390,18 @@ fi\n> \n> if test -z \"$merge_tool\" ; then\n> if test -n \"$DISPLAY\"; then\n> - merge_tool_candidates=\"kdiff3 tkdiff xxdiff meld gvimdiff\"\n> if test -n \"$GNOME_DESKTOP_SESSION_ID\" ; then\n> - merge_tool_candidates=\"meld $merge_tool_candidates\"\n> - fi\n> - if test \"$KDE_FULL_SESSION\" = \"true\"; then\n> - merge_tool_candidates=\"kdiff3 $merge_tool_candidates\"\n> + merge_tool_candidates=\"meld kdiff3 tkdiff xxdiff gvimdiff\"\n> + else\n> + merge_tool_candidates=\"kdiff3 tkdiff xxdiff meld gvimdiff\"\n> fi\n> fi\n> if echo \"${VISUAL:-$EDITOR}\" | grep 'emacs' > /dev/null 2>&1; then\n> - merge_tool_candidates=\"$merge_tool_candidates emerge\"\n> + merge_tool_candidates=\"$merge_tool_candidates emerge opendiff vimdiff\"\n> fi\n> if echo \"${VISUAL:-$EDITOR}\" | grep 'vim' > /dev/null 2>&1; then\n> - merge_tool_candidates=\"$merge_tool_candidates vimdiff\"\n> + merge_tool_candidates=\"$merge_tool_candidates vimdiff opendiff emerge\"\n> fi\n> - merge_tool_candidates=\"$merge_tool_candidates opendiff emerge vimdiff\"\n> echo \"merge tool candidates: $merge_tool_candidates\"\n> for i in $merge_tool_candidates; do\n> init_merge_tool_path $i\n\nDoesn't this change the order of the tools listed in the variable,\naffecting which one ends up being used? I think that is a worse\nregression than repeating the same name twice in an otherwise no-op\ninformational message.\n\nPlease spend a few minutes to see if there are active developers who are\nfamiliar with the area of code you are touching and Cc them to ask their\ninput.\n\n git blame -L390,+20 git-mergetool.sh\n\ntells me that most of this came from 301ac38 (git-mergetool: Make default\nselection of merge-tool more intelligent, 2007-06-10), so I am Cc'ing Ted.\n"},{"id":"101643","messageId":"1232702093-24313-1-git-send-email-heipei@hackvalue.de","threadId":"17292","inReplyTo":"7v7i4m1lq4.fsf@gitster.siamese.dyndns.org","subject":"[PATCHv2] git mergetool: Don't repeat merge tool candidates","fromName":"Johannes Gilger","fromEmail":"heipei@hackvalue.de","sentAt":"2009-01-23T09:14:53Z","receivedAt":"2009-01-23T09:14:53Z","isPatch":false,"sender":{"key":"heipei@hackvalue.de","avatar":"https://avatars.githubusercontent.com/u/6072?v=4"},"body":"git mergetool listed some candidates for mergetools twice, depending on\nthe environment.\n\nSigned-off-by: Johannes Gilger \n---\nThe first patch had the fatal flaw that it listed nothing when DISPLAY \nand EDITOR/VISUAL were unset, we fixed that.\nThe order in which merge-candidates appear is still exactly the same, \nonly duplicates have been stripped. The check for KDE_FULL_SESSION was \nremoved since kdiff3 was added as long as DISPLAY was set and we weren't \nrunning gnome.\n\n git-mergetool.sh | 16 ++++++++--------\n 1 files changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/git-mergetool.sh b/git-mergetool.sh\nindex 00e1337..acdcffb 100755\n--- a/git-mergetool.sh\n+++ b/git-mergetool.sh\n@@ -390,21 +390,21 @@ fi\n \n if test -z \"$merge_tool\" ; then\n if test -n \"$DISPLAY\"; then\n- merge_tool_candidates=\"kdiff3 tkdiff xxdiff meld gvimdiff\"\n if test -n \"$GNOME_DESKTOP_SESSION_ID\" ; then\n- merge_tool_candidates=\"meld $merge_tool_candidates\"\n- fi\n- if test \"$KDE_FULL_SESSION\" = \"true\"; then\n- merge_tool_candidates=\"kdiff3 $merge_tool_candidates\"\n+ merge_tool_candidates=\"meld kdiff3 tkdiff xxdiff gvimdiff\"\n+ else\n+ merge_tool_candidates=\"kdiff3 tkdiff xxdiff meld gvimdiff\"\n fi\n fi\n if echo \"${VISUAL:-$EDITOR}\" | grep 'emacs' > /dev/null 2>&1; then\n- merge_tool_candidates=\"$merge_tool_candidates emerge\"\n+ merge_tool_candidates=\"$merge_tool_candidates emerge opendiff vimdiff\"\n fi\n if echo \"${VISUAL:-$EDITOR}\" | grep 'vim' > /dev/null 2>&1; then\n- merge_tool_candidates=\"$merge_tool_candidates vimdiff\"\n+ merge_tool_candidates=\"$merge_tool_candidates vimdiff opendiff emerge\"\n+ fi\n+ if test -z \"$merge_tool_candidates\" ; then\n+ merge_tool_candidates=\"opendiff emerge vimdiff\"\n fi\n- merge_tool_candidates=\"$merge_tool_candidates opendiff emerge vimdiff\"\n echo \"merge tool candidates: $merge_tool_candidates\"\n for i in $merge_tool_candidates; do\n init_merge_tool_path $i\n-- \n1.6.1.40.g8ea6a\n"},{"id":"101698","messageId":"7vpridr7vb.fsf@gitster.siamese.dyndns.org","threadId":"17292","inReplyTo":"1232702093-24313-1-git-send-email-heipei@hackvalue.de","subject":"Re: [PATCHv2] git mergetool: Don't repeat merge tool candidates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-23T22:10:48Z","receivedAt":"2009-01-23T22:10:48Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Gilger writes:\n\n> git mergetool listed some candidates for mergetools twice, depending on\n> the environment.\n>\n> Signed-off-by: Johannes Gilger \n> ---\n> The first patch had the fatal flaw that it listed nothing when DISPLAY \n> and EDITOR/VISUAL were unset, we fixed that.\n> The order in which merge-candidates appear is still exactly the same, \n> only duplicates have been stripped. The check for KDE_FULL_SESSION was \n> removed since kdiff3 was added as long as DISPLAY was set and we weren't \n> running gnome.\n\nThe old code produced this:\n\n DISPLAY set\n | GNOME_DESKTOP_SESSION_ID set\n | | KDE_FULL_SESSION true\n | | |\n - - - (editor)\n - - + (editor)\n - + - (editor)\n - + + (editor)\n + - - kdiff3 tkdiff xxdiff meld gvimdiff (editor)\n + - + kdiff3 kdiff3 tkdiff xxdiff meld gvimdiff (editor)\n + + - meld kdiff3 tkdiff xxdiff meld gvimdiff (editor)\n + + + kdiff3 meld kdiff3 tkdiff xxdiff meld gvimdiff (editor)\n\nwhere (editor) lists emerge or vimdiff if the preferred editor was emacs\nor vim, and then opendiff, and then emerge and vimdiff as fallback\nduplicates.\n\nLooking at what the new code does for the (editor) fallback part first:\n\n if echo \"${VISUAL:-$EDITOR}\" | grep 'emacs' > /dev/null 2>&1; then\n merge_tool_candidates=\"$merge_tool_candidates emerge opendiff vimdiff\"\n fi\n if echo \"${VISUAL:-$EDITOR}\" | grep 'vim' > /dev/null 2>&1; then\n merge_tool_candidates=\"$merge_tool_candidates vimdiff opendiff emerge\"\n fi\n if test -z \"$merge_tool_candidates\" ; then\n merge_tool_candidates=\"opendiff emerge vimdiff\"\n fi\n\nI think it is better to rewrite this part for clarity:\n\n if EDITOR is emacs?\n then\n \tappend emerge opendiff vimdiff in this order\n elif EDITOR is vim?\n then\n \tappend vimdiff opendiff emerge in this order\n else\n \tappend opendiff emerge vimdiff in this order\n fi\n\nbecause emacs and vim cannot be set to EDITOR at the same time\n(note that I think this also fixes a bug; see below).\n\n> if test -n \"$DISPLAY\"; then\n> if test -n \"$GNOME_DESKTOP_SESSION_ID\" ; then\n> + merge_tool_candidates=\"meld kdiff3 tkdiff xxdiff gvimdiff\"\n> + else\n> + merge_tool_candidates=\"kdiff3 tkdiff xxdiff meld gvimdiff\"\n> fi\n> fi\n\nThis one produces:\n\n DISPLAY set\n | GNOME_DESKTOP_SESSION_ID set\n | | KDE_FULL_SESSION true\n | | |\n - - - (editor)\n - - + (editor)\n - + - (editor)\n - + + (editor)\n + - - kdiff3 tkdiff xxdiff meld gvimdiff (editor')\n + - + kdiff3 tkdiff xxdiff meld gvimdiff (editor')\n + + - meld kdiff3 tkdiff xxdiff gvimdiff (editor')\n + + + meld kdiff3 tkdiff xxdiff gvimdiff (editor')\n\nwhere \"(editor')\" is empty if your EDITOR is not emacs nor vim.\n\nThe original list with the duplicates removed is:\n\n DISPLAY set\n | GNOME_DESKTOP_SESSION_ID set\n | | KDE_FULL_SESSION true\n | | |\n - - - (editor)\n - - + (editor)\n - + - (editor)\n - + + (editor)\n + - - kdiff3 tkdiff xxdiff meld gvimdiff (editor)\n + - + kdiff3 tkdiff xxdiff meld gvimdiff (editor)\n + + - meld kdiff3 tkdiff xxdiff gvimdiff (editor)\n + + + kdiff3 meld tkdiff xxdiff gvimdiff (editor)\n\nAside from the \"(editor') is empty when DISPLAY is set\" difference, the\nresult is also different iff GNOME_DESKTOP_SESSION_ID and KDE_FULL_SESSION\nare both set. I am guessing that that does not happen in a sane\nenvironment, though.\n"},{"id":"101706","messageId":"1232752365-23614-1-git-send-email-heipei@hackvalue.de","threadId":"17292","inReplyTo":"7vpridr7vb.fsf@gitster.siamese.dyndns.org","subject":"[PATCHv3] git mergetool: Don't repeat merge tool candidates","fromName":"Johannes Gilger","fromEmail":"heipei@hackvalue.de","sentAt":"2009-01-23T23:12:45Z","receivedAt":"2009-01-23T23:12:45Z","isPatch":false,"sender":{"key":"heipei@hackvalue.de","avatar":"https://avatars.githubusercontent.com/u/6072?v=4"},"body":"git mergetool listed some candidates for mergetools twice, depending on\nthe environment.\n\nSigned-off-by: Johannes Gilger \n---\nThis improves on v2 of this patch as it appends non-gui merge-tools even \nif $DISPLAY is set. It still makes the assumption that KDE_FULL_SESSION \nand GNOME_DESKTOP_SESSION_ID are never set at the same time. If this \nwere to happen the tool would simply prefer meld over kdiff3.\n\n git-mergetool.sh | 18 ++++++++----------\n 1 files changed, 8 insertions(+), 10 deletions(-)\n\ndiff --git a/git-mergetool.sh b/git-mergetool.sh\nindex 00e1337..09f3a10 100755\n--- a/git-mergetool.sh\n+++ b/git-mergetool.sh\n@@ -390,21 +390,19 @@ fi\n \n if test -z \"$merge_tool\" ; then\n if test -n \"$DISPLAY\"; then\n- merge_tool_candidates=\"kdiff3 tkdiff xxdiff meld gvimdiff\"\n if test -n \"$GNOME_DESKTOP_SESSION_ID\" ; then\n- merge_tool_candidates=\"meld $merge_tool_candidates\"\n- fi\n- if test \"$KDE_FULL_SESSION\" = \"true\"; then\n- merge_tool_candidates=\"kdiff3 $merge_tool_candidates\"\n+ merge_tool_candidates=\"meld kdiff3 tkdiff xxdiff gvimdiff\"\n+ else\n+ merge_tool_candidates=\"kdiff3 tkdiff xxdiff meld gvimdiff\"\n fi\n fi\n if echo \"${VISUAL:-$EDITOR}\" | grep 'emacs' > /dev/null 2>&1; then\n- merge_tool_candidates=\"$merge_tool_candidates emerge\"\n- fi\n- if echo \"${VISUAL:-$EDITOR}\" | grep 'vim' > /dev/null 2>&1; then\n- merge_tool_candidates=\"$merge_tool_candidates vimdiff\"\n+ merge_tool_candidates=\"$merge_tool_candidates emerge opendiff vimdiff\"\n+ elif echo \"${VISUAL:-$EDITOR}\" | grep 'vim' > /dev/null 2>&1; then\n+ merge_tool_candidates=\"$merge_tool_candidates vimdiff opendiff emerge\"\n+ else\n+ merge_tool_candidates=\"$merge_tool_candidates opendiff emerge vimdiff\"\n fi\n- merge_tool_candidates=\"$merge_tool_candidates opendiff emerge vimdiff\"\n echo \"merge tool candidates: $merge_tool_candidates\"\n for i in $merge_tool_candidates; do\n init_merge_tool_path $i\n-- \n1.6.1.40.g8ea6a\n"}]}