{"thread":{"id":"32425","subject":"[PATCH v2] mergetools/p4merge: Honor $TMPDIR for the /dev/null placeholder","startedAt":"2012-12-21T06:57:03Z","lastAt":"2012-12-23T01:31:12Z","messageCount":5,"participants":["David Aguilar","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"205317","messageId":"1356073023-15376-1-git-send-email-davvid@gmail.com","threadId":"32425","inReplyTo":null,"subject":"[PATCH v2] mergetools/p4merge: Honor $TMPDIR for the /dev/null placeholder","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-12-21T06:57:03Z","receivedAt":"2012-12-21T06:57:03Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Use $TMPDIR when creating the /dev/null placeholder for p4merge.\nThis keeps it out of the current directory.\n\nReported-by: Jeremy Morton \nSigned-off-by: David Aguilar \n---\nNo mktemp usage in this round.\n\n mergetools/p4merge | 27 +++++++++++++--------------\n 1 file changed, 13 insertions(+), 14 deletions(-)\n\ndiff --git a/mergetools/p4merge b/mergetools/p4merge\nindex 295361a..52f7c8f 100644\n--- a/mergetools/p4merge\n+++ b/mergetools/p4merge\n@@ -1,29 +1,21 @@\n diff_cmd () {\n+\tempty_file=\n+\n \t# p4merge does not like /dev/null\n-\trm_local=\n-\trm_remote=\n \tif test \"/dev/null\" = \"$LOCAL\"\n \tthen\n-\t\tLOCAL=\"./p4merge-dev-null.LOCAL.$$\"\n-\t\t>\"$LOCAL\"\n-\t\trm_local=true\n+\t\tLOCAL=\"$(create_empty_file)\"\n \tfi\n \tif test \"/dev/null\" = \"$REMOTE\"\n \tthen\n-\t\tREMOTE=\"./p4merge-dev-null.REMOTE.$$\"\n-\t\t>\"$REMOTE\"\n-\t\trm_remote=true\n+\t\tREMOTE=\"$(create_empty_file)\"\n \tfi\n \n \t\"$merge_tool_path\" \"$LOCAL\" \"$REMOTE\"\n \n-\tif test -n \"$rm_local\"\n-\tthen\n-\t\trm -f \"$LOCAL\"\n-\tfi\n-\tif test -n \"$rm_remote\"\n+\tif test -n \"$empty_file\"\n \tthen\n-\t\trm -f \"$REMOTE\"\n+\t\trm -f \"$empty_file\"\n \tfi\n }\n \n@@ -33,3 +25,10 @@ merge_cmd () {\n \t\"$merge_tool_path\" \"$BASE\" \"$LOCAL\" \"$REMOTE\" \"$MERGED\"\n \tcheck_unchanged\n }\n+\n+create_empty_file () {\n+\tempty_file=\"${TMPDIR:-/tmp}/git-difftool-p4merge-empty-file.$$\"\n+\t>\"$empty_file\"\n+\n+\tprintf \"$empty_file\"\n+}\n-- \n1.8.1.rc2.6.g18499ba\n"},{"id":"205342","messageId":"7v4njf2xrk.fsf@alter.siamese.dyndns.org","threadId":"32425","inReplyTo":"1356073023-15376-1-git-send-email-davvid@gmail.com","subject":"Re: [PATCH v2] mergetools/p4merge: Honor $TMPDIR for the /dev/null placeholder","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-12-21T16:08:47Z","receivedAt":"2012-12-21T16:08:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar writes:\n\n> Use $TMPDIR when creating the /dev/null placeholder for p4merge.\n> This keeps it out of the current directory.\n\nThe usual $REMOTE \"this is theirs\" and $LOCAL \"this is ours\" are\nstill created in the current directory, no? It is unclear why this\n\"this side does not exist\" case wants to be outside of the current\ndirectory in the first place.\n\nIn other words, \"This keeps it out of the current directory\" only\nexplains what this patch changes, without explaining why it is a\ngood thing to do in the first place.\n\n> +create_empty_file () {\n> +\tempty_file=\"${TMPDIR:-/tmp}/git-difftool-p4merge-empty-file.$$\"\n> +\t>\"$empty_file\"\n> +\n> +\tprintf \"$empty_file\"\n> +}\n\nAssuming that it makes sense to create only the \"this side doe not\nexist, and here is a placeholder empty file\" in $TMPDIR, I think\nthis is probably sufficient.\n\nBy the way, who is going to remove this temporary file once the\ncommand is done?\n"},{"id":"205385","messageId":"7v38yysvhb.fsf@alter.siamese.dyndns.org","threadId":"32425","inReplyTo":"7v4njf2xrk.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] mergetools/p4merge: Honor $TMPDIR for the /dev/null placeholder","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-12-22T01:53:36Z","receivedAt":"2012-12-22T01:53:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> By the way, who is going to remove this temporary file once the\n> command is done?\n\nNevermind; I can see that once the backend returns it is removed in\nthe same function.\n"},{"id":"205433","messageId":"CAJDDKr54XBtU4J-A8T9LwXAPL6E8M2_0kaTxH_-LZCAR0fwN+g@mail.gmail.com","threadId":"32425","inReplyTo":"7v4njf2xrk.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] mergetools/p4merge: Honor $TMPDIR for the /dev/null placeholder","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-12-22T21:56:53Z","receivedAt":"2012-12-22T21:56:53Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Fri, Dec 21, 2012 at 8:08 AM, Junio C Hamano wrote:\n> David Aguilar writes:\n>\n>> Use $TMPDIR when creating the /dev/null placeholder for p4merge.\n>> This keeps it out of the current directory.\n>\n> The usual $REMOTE \"this is theirs\" and $LOCAL \"this is ours\" are\n> still created in the current directory, no? It is unclear why this\n> \"this side does not exist\" case wants to be outside of the current\n> directory in the first place.\n\nI'll take this as a hint that I should improve the commit message.\nAs you alluded to in your earlier email, we are in feature freeze\nso I will be holding off on sending it until things have settled.\n\nI don't believe there is a strong reason why the file should be in one\nplace vs. another, and the explanation is different depending on who\nis driving the scriptlet.\n\nWhen difftool drives then $LOCAL and $REMOTE may point to\ntemporary files created by the GIT_EXTERNAL_DIFF machinery.\nFor that use case, this commit makes things consistent with\nthose location of paths.\n\nWhen mergetool drives it creates $LOCAL and $REMOTE in\nthe current directory. They contain the different stages\nof the index and can be helpful to the user when resolving merges.\n\nThe temporary /dev/null placeholder is a workaround for p4merge\nand from the user's point of view this file is never interesting, so\nwriting it into the worktree is distracting to the user.\nI could also say that it makes it consistent because \"/dev\" is\nalso not in the current directory, but that's a stretch ;-)\n\n> In other words, \"This keeps it out of the current directory\" only\n> explains what this patch changes, without explaining why it is a\n> good thing to do in the first place.\n\nI'll try and write up a replacement patch but I'll hold off\non sending it out until after things have settled down.\n\nFWIW I'm seeing a \"Bus Error\" when doing \"git update-index --refresh\"\nin a repository with large files on a 32bit machine. I'm not sure if\nthat counts as a regression since the same error occurs in 1.7.10.4\n(debian testing).\n\nI'll start another thread on this topic in case anyone is interested.\n\n>> +create_empty_file () {\n>> + empty_file=\"${TMPDIR:-/tmp}/git-difftool-p4merge-empty-file.$$\"\n>> + >\"$empty_file\"\n>> +\n>> + printf \"$empty_file\"\n>> +}\n>\n> Assuming that it makes sense to create only the \"this side doe not\n> exist, and here is a placeholder empty file\" in $TMPDIR, I think\n> this is probably sufficient.\n\nYup. I mulled over whether or not to embed \"LOCAL\" and \"REMOTE\"\nin the name but eventually decided that it was not worth the effort.\nIt makes the code much simpler when they share the placeholder\nsince we no longer need to track them separately.\n-- \nDavid\n"},{"id":"205438","messageId":"CAJDDKr7fcz=GUs649c4pZDgV=4P_dSRtMHcczzVDvWz_L3_7uw@mail.gmail.com","threadId":"32425","inReplyTo":"CAJDDKr54XBtU4J-A8T9LwXAPL6E8M2_0kaTxH_-LZCAR0fwN+g@mail.gmail.com","subject":"Re: [PATCH v2] mergetools/p4merge: Honor $TMPDIR for the /dev/null placeholder","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-12-23T01:31:12Z","receivedAt":"2012-12-23T01:31:12Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Sat, Dec 22, 2012 at 1:56 PM, David Aguilar wrote:\n> FWIW I'm seeing a \"Bus Error\" when doing \"git update-index --refresh\"\n> in a repository with large files on a 32bit machine. I'm not sure if\n> that counts as a regression since the same error occurs in 1.7.10.4\n> (debian testing).\n>\n> I'll start another thread on this topic in case anyone is interested.\n\nNevermind. The machine is an old salvaged laptop and its disk\nprobably has some bad blocks.\n\nI wasn't able to copy the bad repo since 'cp' kept failing to\ncopy the index file. I/O errors.. After deleting the borked\nindex the problems went away, so I'll chalk this up to\nfailing hardware. Sorry for the false alarm.\n-- \nDavid\n"}]}