{"thread":{"id":"39381","subject":"[PATCH v6 1/2] mergetool--lib: set IFS for difftool and mergetool","startedAt":"2015-05-20T09:07:22Z","lastAt":"2015-05-22T20:16:07Z","messageCount":11,"participants":["David Aguilar","SZEDER Gábor","Junio C Hamano","Sebastian Schuberth","Johannes Schindelin"],"isPatch":true,"patchVersion":6,"patchTotal":2},"messages":[{"id":"261650","messageId":"1432112843-973-1-git-send-email-davvid@gmail.com","threadId":"39381","inReplyTo":null,"subject":"[PATCH v6 1/2] mergetool--lib: set IFS for difftool and mergetool","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2015-05-20T09:07:22Z","receivedAt":"2015-05-20T09:07:22Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"git-sh-setup sets IFS but it is not used by git-difftool--helper.\nSet IFS in git-mergetool--lib so that the mergetool scriptlets,\ndifftool, and mergetool do not need to do so.\n\nSigned-off-by: David Aguilar \n---\nChanges since v5: fixed \"diftool\" typo in commit message\n\n git-mergetool--lib.sh | 3 +++\n git-mergetool.sh | 2 --\n 2 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex fe61e89..14b039d 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -2,6 +2,9 @@\n \n : ${MERGE_TOOLS_DIR=$(git --exec-path)/mergetools}\n \n+IFS='\n+'\n+\n mode_ok () {\n \tif diff_mode\n \tthen\ndiff --git a/git-mergetool.sh b/git-mergetool.sh\nindex d20581c..9f77e3a 100755\n--- a/git-mergetool.sh\n+++ b/git-mergetool.sh\n@@ -451,8 +451,6 @@ fi\n printf \"Merging:\\n\"\n printf \"%s\\n\" \"$files\"\n \n-IFS='\n-'\n rc=0\n for i in $files\n do\n-- \n2.4.1.218.gc09a0e5\n"},{"id":"261651","messageId":"1432112843-973-2-git-send-email-davvid@gmail.com","threadId":"39381","inReplyTo":"1432112843-973-1-git-send-email-davvid@gmail.com","subject":"[PATCH v6 2/2] mergetools: add winmerge as a builtin tool","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2015-05-20T09:07:23Z","receivedAt":"2015-05-20T09:07:23Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Add a winmerge scriptlet with the commands described in [1] so\nthat users can use winmerge without needing to perform any\nadditional configuration.\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/268631\n\nHelped-by: Philip Oakley \nHelped-by: Johannes Schindelin \nHelped-by: Sebastian Schuberth \nHelped-by: SZEDER Gábor \nSigned-off-by: David Aguilar \n---\nUnchanged since v5.\nChanges since v4: IFS is left as-is since it's done in 1/2\n\n mergetools/winmerge | 36 ++++++++++++++++++++++++++++++++++++\n 1 file changed, 36 insertions(+)\n create mode 100644 mergetools/winmerge\n\ndiff --git a/mergetools/winmerge b/mergetools/winmerge\nnew file mode 100644\nindex 0000000..74a66d4\n--- /dev/null\n+++ b/mergetools/winmerge\n@@ -0,0 +1,36 @@\n+diff_cmd () {\n+\t\"$merge_tool_path\" -u -e \"$LOCAL\" \"$REMOTE\"\n+\treturn 0\n+}\n+\n+merge_cmd () {\n+\t# mergetool.winmerge.trustExitCode is implicitly false.\n+\t# touch $BACKUP so that we can check_unchanged.\n+\ttouch \"$BACKUP\"\n+\t\"$merge_tool_path\" -u -e -dl Local -dr Remote \\\n+\t\t\"$LOCAL\" \"$REMOTE\" \"$MERGED\"\n+\tcheck_unchanged\n+}\n+\n+translate_merge_tool_path() {\n+\t# Use WinMergeU.exe if it exists in $PATH\n+\tif type -p WinMergeU.exe >/dev/null 2>&1\n+\tthen\n+\t\tprintf WinMergeU.exe\n+\t\treturn\n+\tfi\n+\n+\t# Look for WinMergeU.exe in the typical locations\n+\twinmerge_exe=\"WinMerge/WinMergeU.exe\"\n+\tfor directory in $(env | grep -Ei '^PROGRAM(FILES(\\(X86\\))?|W6432)=' |\n+\t\tcut -d '=' -f 2- | sort -u)\n+\tdo\n+\t\tif test -n \"$directory\" && test -x \"$directory/$winmerge_exe\"\n+\t\tthen\n+\t\t\tprintf '%s' \"$directory/$winmerge_exe\"\n+\t\t\treturn\n+\t\tfi\n+\tdone\n+\n+\tprintf WinMergeU.exe\n+}\n-- \n2.4.1.218.gc09a0e5\n"},{"id":"261653","messageId":"20150520130929.Horde.vYwOuIDRpi6hr15rOUbW1w7@webmail.informatik.kit.edu","threadId":"39381","inReplyTo":"1432112843-973-2-git-send-email-davvid@gmail.com","subject":"Re: [PATCH v6 2/2] mergetools: add winmerge as a builtin tool","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2015-05-20T11:09:29Z","receivedAt":"2015-05-20T11:09:29Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\nQuoting David Aguilar :\n\n> +translate_merge_tool_path() {\n> +\t# Use WinMergeU.exe if it exists in $PATH\n> +\tif type -p WinMergeU.exe >/dev/null 2>&1\n> +\tthen\n> +\t\tprintf WinMergeU.exe\n> +\t\treturn\n> +\tfi\n> +\n> +\t# Look for WinMergeU.exe in the typical locations\n> +\twinmerge_exe=\"WinMerge/WinMergeU.exe\"\n\nThis variable is not used elsewhere, right? Then you might want to \nmark it as local to make this clear.\n\n> +\tfor directory in $(env | grep -Ei '^PROGRAM(FILES(\\(X86\\))?|W6432)=' |\n> +\t\tcut -d '=' -f 2- | sort -u)\n> +\tdo\n> +\t\tif test -n \"$directory\" && test -x \"$directory/$winmerge_exe\"\n> +\t\tthen\n> +\t\t\tprintf '%s' \"$directory/$winmerge_exe\"\n> +\t\t\treturn\n> +\t\tfi\n> +\tdone\n> +\n> +\tprintf WinMergeU.exe\n\nPlease pardon my ignorance and curiosity, but what is the purpose of \nthis last printf?\nIt outputs the same as in the case when winmerge is in $PATH at the \nbeginning of the function. However, if we reach this printf, then \nwinmerge is not in $PATH, so what will be executed?\n\n\nGábor\n\n> +}\n> --\n> 2.4.1.218.gc09a0e5\n"},{"id":"261707","messageId":"xmqqtwv7m2hw.fsf@gitster.dls.corp.google.com","threadId":"39381","inReplyTo":"1432112843-973-2-git-send-email-davvid@gmail.com","subject":"Re: [PATCH v6 2/2] mergetools: add winmerge as a builtin tool","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-20T20:13:15Z","receivedAt":"2015-05-20T20:13:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar writes:\n\n> +\tfor directory in $(env | grep -Ei '^PROGRAM(FILES(\\(X86\\))?|W6432)=' |\n> +\t\tcut -d '=' -f 2- | sort -u)\n\nIs the final \"sort\" really desired? I am wondering if there are\nfixed precedence/preference order among variants of %PROGRAMFILES% \nenvironment variables that the users on the platform are expected\nto stick to, but the \"sort\" is sorting by the absolute pathnames of\nwhere these things are, which may not reflect that order.\n\nNot that I personally care too deeply, as I would expect that it is\nlikely any one of them found would just work fine ;-)\n\n> +\tdo\n> +\t\tif test -n \"$directory\" && test -x \"$directory/$winmerge_exe\"\n> +\t\tthen\n> +\t\t\tprintf '%s' \"$directory/$winmerge_exe\"\n> +\t\t\treturn\n> +\t\tfi\n> +\tdone\n> +\n> +\tprintf WinMergeU.exe\n> +}\n"},{"id":"261709","messageId":"CAHGBnuPyhG4y5ooR7KH0KrEhRYFu9BB+HKnnn+XhU5xL2TnL=w@mail.gmail.com","threadId":"39381","inReplyTo":"xmqqtwv7m2hw.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v6 2/2] mergetools: add winmerge as a builtin tool","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2015-05-20T20:20:09Z","receivedAt":"2015-05-20T20:20:09Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Wed, May 20, 2015 at 10:13 PM, Junio C Hamano wrote:\n\n> David Aguilar writes:\n>\n>> + for directory in $(env | grep -Ei '^PROGRAM(FILES(\\(X86\\))?|W6432)=' |\n>> + cut -d '=' -f 2- | sort -u)\n>\n> Is the final \"sort\" really desired? I am wondering if there are\n> fixed precedence/preference order among variants of %PROGRAMFILES%\n> environment variables that the users on the platform are expected\n> to stick to, but the \"sort\" is sorting by the absolute pathnames of\n> where these things are, which may not reflect that order.\n\nI did add the sort (and -u) by intention, to ensure that \"C:\\Program\nFiles\" (which is what %PROGRAMFILES% expands to by default) comes\nbefore \"C:\\Program Files (x86)\" (which is what %PROGRAMFILES(X86)%\nexpands to by default), so that programs of the OS-native bitness are\npreferred.\n\n-- \nSebastian Schuberth\n"},{"id":"261715","messageId":"xmqq8ucjm0bo.fsf@gitster.dls.corp.google.com","threadId":"39381","inReplyTo":"CAHGBnuPyhG4y5ooR7KH0KrEhRYFu9BB+HKnnn+XhU5xL2TnL=w@mail.gmail.com","subject":"Re: [PATCH v6 2/2] mergetools: add winmerge as a builtin tool","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-20T21:00:11Z","receivedAt":"2015-05-20T21:00:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sebastian Schuberth writes:\n\n> On Wed, May 20, 2015 at 10:13 PM, Junio C Hamano wrote:\n>\n>> David Aguilar writes:\n>>\n>>> + for directory in $(env | grep -Ei '^PROGRAM(FILES(\\(X86\\))?|W6432)=' |\n>>> + cut -d '=' -f 2- | sort -u)\n>>\n>> Is the final \"sort\" really desired? I am wondering if there are\n>> fixed precedence/preference order among variants of %PROGRAMFILES%\n>> environment variables that the users on the platform are expected\n>> to stick to, but the \"sort\" is sorting by the absolute pathnames of\n>> where these things are, which may not reflect that order.\n>\n> I did add the sort (and -u) by intention, to ensure that \"C:\\Program\n> Files\" (which is what %PROGRAMFILES% expands to by default) comes\n> before \"C:\\Program Files (x86)\" (which is what %PROGRAMFILES(X86)%\n> expands to by default), so that programs of the OS-native bitness are\n> preferred.\n\nYuck. So even though %PROGRAMFILES% and %PROGRAMFILES(X86)% look as\nif they are variables that can point at arbitrary places, they in\nreality don't? Otherwise %PROGRAMFILES% may point at D:\\Program\nwhile %PROGRAMFILES(X86)% may piont at C:\\X86 and the latter would\nsort before the former, which would defeat that \"logic\".\n\nBut of course if I view this not as a \"logic\" but as a \"heuristics\"\nthat happens to do the right thing in common environments, it is\nperfectly OK ;-).\n\nI've queued the patches as-is.\n\nThanks.\n"},{"id":"261721","messageId":"CAHGBnuMt82oawQnoPGNiXefui6k5A3DW1ktGWvMy0m8iq78oPg@mail.gmail.com","threadId":"39381","inReplyTo":"xmqq8ucjm0bo.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v6 2/2] mergetools: add winmerge as a builtin tool","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2015-05-20T21:20:26Z","receivedAt":"2015-05-20T21:20:26Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Wed, May 20, 2015 at 11:00 PM, Junio C Hamano wrote:\n\n> Yuck. So even though %PROGRAMFILES% and %PROGRAMFILES(X86)% look as\n> if they are variables that can point at arbitrary places, they in\n> reality don't? Otherwise %PROGRAMFILES% may point at D:\\Program\n\nCorrect. In the vast majority of WIndows installations these\nvariables contain the default values.\n\n> But of course if I view this not as a \"logic\" but as a \"heuristics\"\n> that happens to do the right thing in common environments, it is\n> perfectly OK ;-).\n\nExactly a heuristic is what it's supposed to be :-)\n\n-- \nSebastian Schuberth\n"},{"id":"261770","messageId":"86b89ca969ff967f600f955f1990cdca@www.dscho.org","threadId":"39381","inReplyTo":"xmqq8ucjm0bo.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v6 2/2] mergetools: add winmerge as a builtin tool","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-05-21T10:06:35Z","receivedAt":"2015-05-21T10:06:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn 2015-05-20 23:00, Junio C Hamano wrote:\n> Sebastian Schuberth writes:\n> \n>> On Wed, May 20, 2015 at 10:13 PM, Junio C Hamano wrote:\n>>\n>>> David Aguilar writes:\n>>>\n>>>> + for directory in $(env | grep -Ei '^PROGRAM(FILES(\\(X86\\))?|W6432)=' |\n>>>> + cut -d '=' -f 2- | sort -u)\n>>>\n>>> Is the final \"sort\" really desired? I am wondering if there are\n>>> fixed precedence/preference order among variants of %PROGRAMFILES%\n>>> environment variables that the users on the platform are expected\n>>> to stick to, but the \"sort\" is sorting by the absolute pathnames of\n>>> where these things are, which may not reflect that order.\n>>\n>> I did add the sort (and -u) by intention, to ensure that \"C:\\Program\n>> Files\" (which is what %PROGRAMFILES% expands to by default) comes\n>> before \"C:\\Program Files (x86)\" (which is what %PROGRAMFILES(X86)%\n>> expands to by default), so that programs of the OS-native bitness are\n>> preferred.\n> \n> Yuck. So even though %PROGRAMFILES% and %PROGRAMFILES(X86)% look as\n> if they are variables that can point at arbitrary places, they in\n> reality don't? Otherwise %PROGRAMFILES% may point at D:\\Program\n> while %PROGRAMFILES(X86)% may piont at C:\\X86 and the latter would\n> sort before the former, which would defeat that \"logic\".\n\nWell, you are correct, theoretically an administrator could set the registry values (which are the source of the environment variables in question) of the `ProgramFilesDir` key in both\n\nHKEY_LOCAL_MACHINE\\Software\\Microsoft\\Windows\\CurrentVersion\n\nand\n\nHKEY_LOCAL_MACHINE\\Software\\WOW64\\Microsoft\\Windows\\CurrentVersion\n\nto wildly different locations as you outlined. However, it is not supported by Microsoft to change those locations via the registry:\n\nhttps://support.microsoft.com/en-us/kb/933700\n\nCiao,\nDscho\n"},{"id":"261944","messageId":"20150522195802.GA26066@gmail.com","threadId":"39381","inReplyTo":"20150520130929.Horde.vYwOuIDRpi6hr15rOUbW1w7@webmail.informatik.kit.edu","subject":"Re: [PATCH v6 2/2] mergetools: add winmerge as a builtin tool","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2015-05-22T19:58:03Z","receivedAt":"2015-05-22T19:58:03Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"\n[just wrapping up the unaswered questions in this thread]\n\nOn Wed, May 20, 2015 at 01:09:29PM +0200, SZEDER Gábor wrote:\n> \n> Quoting David Aguilar :\n> \n> >+translate_merge_tool_path() {\n> >+\t# Use WinMergeU.exe if it exists in $PATH\n> >+\tif type -p WinMergeU.exe >/dev/null 2>&1\n> >+\tthen\n> >+\t\tprintf WinMergeU.exe\n> >+\t\treturn\n> >+\tfi\n> >+\n> >+\t# Look for WinMergeU.exe in the typical locations\n> >+\twinmerge_exe=\"WinMerge/WinMergeU.exe\"\n> \n> This variable is not used elsewhere, right? Then you might want to\n> mark it as local to make this clear.\n\n\n\"local\" is a bash-ism, otherwise that'd be a good idea.\n\n\n> >+\tfor directory in $(env | grep -Ei '^PROGRAM(FILES(\\(X86\\))?|W6432)=' |\n> >+\t\tcut -d '=' -f 2- | sort -u)\n> >+\tdo\n> >+\t\tif test -n \"$directory\" && test -x \"$directory/$winmerge_exe\"\n> >+\t\tthen\n> >+\t\t\tprintf '%s' \"$directory/$winmerge_exe\"\n> >+\t\t\treturn\n> >+\t\tfi\n> >+\tdone\n> >+\n> >+\tprintf WinMergeU.exe\n> \n> Please pardon my ignorance and curiosity, but what is the purpose of\n> this last printf?\n> It outputs the same as in the case when winmerge is in $PATH at the\n> beginning of the function. However, if we reach this printf, then\n> winmerge is not in $PATH, so what will be executed?\n\n\nThis function maps what we call the tool (winmerge) to the actual executable.\nThat last printf provides the following behavior:\n\n\t$ git difftool -t winmerge HEAD~\n\t\n\tViewing (1/1): 'mergetools/winmerge'\n\tLaunch 'winmerge' [Y/n]:\n\tThe diff tool winmerge is not available as 'WinMergeU.exe'\n\tfatal: external diff died, stopping at mergetools/winmerge\n\nIt ensures that the user sees 'WinMergeU.exe' in the error message.\nThat way the user can resolve the problem by e.g. adjusting their $PATH,\nor realizing that they don't have WinMergeU.exe installed.\n-- \nDavid\n"},{"id":"261948","messageId":"xmqqr3q8e5tj.fsf@gitster.dls.corp.google.com","threadId":"39381","inReplyTo":"20150522195802.GA26066@gmail.com","subject":"Re: [PATCH v6 2/2] mergetools: add winmerge as a builtin tool","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-22T20:05:28Z","receivedAt":"2015-05-22T20:05:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar writes:\n\n> [just wrapping up the unaswered questions in this thread]\n> ...\n> On Wed, May 20, 2015 at 01:09:29PM +0200, SZEDER Gábor wrote:\n\nThanks for clarifications. I think all is good now?\n"},{"id":"261949","messageId":"20150522201606.GB26066@gmail.com","threadId":"39381","inReplyTo":"xmqqr3q8e5tj.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v6 2/2] mergetools: add winmerge as a builtin tool","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2015-05-22T20:16:07Z","receivedAt":"2015-05-22T20:16:07Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Fri, May 22, 2015 at 01:05:28PM -0700, Junio C Hamano wrote:\n> David Aguilar writes:\n> \n> > [just wrapping up the unaswered questions in this thread]\n> > ...\n> > On Wed, May 20, 2015 at 01:09:29PM +0200, SZEDER Gábor wrote:\n> \n> Thanks for clarifications. I think all is good now?\n\nYes, I think so. I just looked at what you have queued in pu\nand it looks good.\n\nThanks all,\n-- \nDavid\n"}]}