{"thread":{"id":"32413","subject":"[PATCH] mergetools/p4merge: Honor $TMPDIR for the /dev/null placeholder","startedAt":"2012-12-20T04:45:54Z","lastAt":"2012-12-20T08:20:35Z","messageCount":5,"participants":["David Aguilar","Junio C Hamano","Joachim Schmitz"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"205244","messageId":"1355978754-7041-1-git-send-email-davvid@gmail.com","threadId":"32413","inReplyTo":null,"subject":"[PATCH] mergetools/p4merge: Honor $TMPDIR for the /dev/null placeholder","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-12-20T04:45:54Z","receivedAt":"2012-12-20T04:45:54Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Use mktemp to create 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---\nI consider this a final finishing touch on a new 1.8.1 feature,\nso hopefully we can get this in before 1.8.1.\n\n mergetools/p4merge | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/mergetools/p4merge b/mergetools/p4merge\nindex 295361a..090fa9b 100644\n--- a/mergetools/p4merge\n+++ b/mergetools/p4merge\n@@ -4,13 +4,13 @@ diff_cmd () {\n \trm_remote=\n \tif test \"/dev/null\" = \"$LOCAL\"\n \tthen\n-\t\tLOCAL=\"./p4merge-dev-null.LOCAL.$$\"\n+\t\tLOCAL=\"$(create_empty_file)\"\n \t\t>\"$LOCAL\"\n \t\trm_local=true\n \tfi\n \tif test \"/dev/null\" = \"$REMOTE\"\n \tthen\n-\t\tREMOTE=\"./p4merge-dev-null.REMOTE.$$\"\n+\t\tREMOTE=\"$(create_empty_file)\"\n \t\t>\"$REMOTE\"\n \t\trm_remote=true\n \tfi\n@@ -33,3 +33,7 @@ merge_cmd () {\n \t\"$merge_tool_path\" \"$BASE\" \"$LOCAL\" \"$REMOTE\" \"$MERGED\"\n \tcheck_unchanged\n }\n+\n+create_empty_file () {\n+\tmktemp -t git-difftool-p4merge-empty-file.XXXXXX\n+}\n-- \n1.8.1.rc2.6.g18499ba\n"},{"id":"205247","messageId":"7v623x6xvv.fsf@alter.siamese.dyndns.org","threadId":"32413","inReplyTo":"1355978754-7041-1-git-send-email-davvid@gmail.com","subject":"Re: [PATCH] mergetools/p4merge: Honor $TMPDIR for the /dev/null placeholder","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-12-20T06:28:04Z","receivedAt":"2012-12-20T06:28:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar writes:\n\n> Use mktemp to create the /dev/null placeholder for p4merge.\n> This keeps it out of the current directory.\n>\n> Reported-by: Jeremy Morton \n> Signed-off-by: David Aguilar \n> ---\n> I consider this a final finishing touch on a new 1.8.1 feature,\n> so hopefully we can get this in before 1.8.1.\n\nDoes everybody have mktemp(1), which is not even in POSIX.1?\n\nI'm a bit hesitant to apply this to the upcoming release without\ncooking it in 'next' for sufficiently long time to give it a chance\nto be tried by wider audience.\n\n> mergetools/p4merge | 8 ++++++--\n> 1 file changed, 6 insertions(+), 2 deletions(-)\n>\n> diff --git a/mergetools/p4merge b/mergetools/p4merge\n> index 295361a..090fa9b 100644\n> --- a/mergetools/p4merge\n> +++ b/mergetools/p4merge\n> @@ -4,13 +4,13 @@ diff_cmd () {\n> \trm_remote=\n> \tif test \"/dev/null\" = \"$LOCAL\"\n> \tthen\n> -\t\tLOCAL=\"./p4merge-dev-null.LOCAL.$$\"\n> +\t\tLOCAL=\"$(create_empty_file)\"\n> \t\t>\"$LOCAL\"\n> \t\trm_local=true\n> \tfi\n> \tif test \"/dev/null\" = \"$REMOTE\"\n> \tthen\n> -\t\tREMOTE=\"./p4merge-dev-null.REMOTE.$$\"\n> +\t\tREMOTE=\"$(create_empty_file)\"\n> \t\t>\"$REMOTE\"\n> \t\trm_remote=true\n> \tfi\n> @@ -33,3 +33,7 @@ merge_cmd () {\n> \t\"$merge_tool_path\" \"$BASE\" \"$LOCAL\" \"$REMOTE\" \"$MERGED\"\n> \tcheck_unchanged\n> }\n> +\n> +create_empty_file () {\n> +\tmktemp -t git-difftool-p4merge-empty-file.XXXXXX\n> +}\n"},{"id":"205248","messageId":"7v1uel6w1i.fsf@alter.siamese.dyndns.org","threadId":"32413","inReplyTo":"7v623x6xvv.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] mergetools/p4merge: Honor $TMPDIR for the /dev/null placeholder","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-12-20T07:07:53Z","receivedAt":"2012-12-20T07:07:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> David Aguilar writes:\n>\n>> Use mktemp to create the /dev/null placeholder for p4merge.\n>> This keeps it out of the current directory.\n>>\n>> Reported-by: Jeremy Morton \n>> Signed-off-by: David Aguilar \n>> ---\n>> I consider this a final finishing touch on a new 1.8.1 feature,\n>> so hopefully we can get this in before 1.8.1.\n>\n> Does everybody have mktemp(1), which is not even in POSIX.1?\n>\n> I'm a bit hesitant to apply this to the upcoming release without\n> cooking it in 'next' for sufficiently long time to give it a chance\n> to be tried by wider audience.\n\nOne approach may be to do something like this as a preparation step\n(current callers of unpack-file may want to do their temporary work\nin TMPDIR as well), and then use\n\n\tgit unpack-file -t e69de29bb2d1d6434b8b29ae775ad8c2e48c5391\n\nto create your empty temporary file.\n\n Documentation/git-unpack-file.txt | 10 +++++++---\n builtin/unpack-file.c | 28 +++++++++++++++++++++-------\n 2 files changed, 28 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/git-unpack-file.txt b/Documentation/git-unpack-file.txt\nindex e9f148a..56af328 100644\n--- a/Documentation/git-unpack-file.txt\n+++ b/Documentation/git-unpack-file.txt\n@@ -10,16 +10,20 @@ git-unpack-file - Creates a temporary file with a blob's contents\n SYNOPSIS\n --------\n [verse]\n-'git unpack-file' \n+'git unpack-file' [-t] \n \n DESCRIPTION\n -----------\n Creates a file holding the contents of the blob specified by sha1. It\n-returns the name of the temporary file in the following format:\n-\t.merge_file_XXXXX\n+returns the name of the temporary file (by default `.merge_file_XXXXX`).\n+\n \n OPTIONS\n -------\n+-t::\n+\tThe temporary file is created in `$TMPDIR` directory,\n+\tinstead of the current directory.\n+\n ::\n \tMust be a blob id\n \ndiff --git a/builtin/unpack-file.c b/builtin/unpack-file.c\nindex 1920029..de1f845 100644\n--- a/builtin/unpack-file.c\n+++ b/builtin/unpack-file.c\n@@ -1,8 +1,9 @@\n #include \"builtin.h\"\n \n-static char *create_temp_file(unsigned char *sha1)\n+static char *create_temp_file(unsigned char *sha1, int in_tempdir)\n {\n-\tstatic char path[50];\n+\tstatic char path[1024];\n+\tstatic const char template[] = \".merge_file_XXXXXX\";\n \tvoid *buf;\n \tenum object_type type;\n \tunsigned long size;\n@@ -12,8 +13,12 @@ static char *create_temp_file(unsigned char *sha1)\n \tif (!buf || type != OBJ_BLOB)\n \t\tdie(\"unable to read blob object %s\", sha1_to_hex(sha1));\n \n-\tstrcpy(path, \".merge_file_XXXXXX\");\n-\tfd = xmkstemp(path);\n+\tif (in_tempdir) {\n+\t\tfd = git_mkstemp(path, sizeof(path) - 1, template);\n+\t} else {\n+\t\tstrcpy(path, template);\n+\t\tfd = xmkstemp(path);\n+\t}\n \tif (write_in_full(fd, buf, size) != size)\n \t\tdie_errno(\"unable to write temp-file\");\n \tclose(fd);\n@@ -23,14 +28,23 @@ static char *create_temp_file(unsigned char *sha1)\n int cmd_unpack_file(int argc, const char **argv, const char *prefix)\n {\n \tunsigned char sha1[20];\n+\tint in_tempdir = 0;\n+\n+\tif (argc < 2 || 3 < argc || !strcmp(argv[1], \"-h\"))\n+\t\tusage(\"git unpack-file [-t] \");\n+\tif (argc == 3) {\n+\t\tif (strcmp(argv[1], \"-t\"))\n+\t\t\tusage(\"git unpack-file [-t] \");\n+\t\tin_tempdir = 1;\n+\t\targc--;\n+\t\targv++;\n+\t}\n \n-\tif (argc != 2 || !strcmp(argv[1], \"-h\"))\n-\t\tusage(\"git unpack-file \");\n \tif (get_sha1(argv[1], sha1))\n \t\tdie(\"Not a valid object name %s\", argv[1]);\n \n \tgit_config(git_default_config, NULL);\n \n-\tputs(create_temp_file(sha1));\n+\tputs(create_temp_file(sha1, in_tempdir));\n \treturn 0;\n }\n"},{"id":"205249","messageId":"CAJDDKr63Yf0-3yTppGKAW+19wHY_OOF0iiKZ_6XR+Vae-7Bx7w@mail.gmail.com","threadId":"32413","inReplyTo":"7v623x6xvv.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] mergetools/p4merge: Honor $TMPDIR for the /dev/null placeholder","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-12-20T08:03:35Z","receivedAt":"2012-12-20T08:03:35Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Wed, Dec 19, 2012 at 10:28 PM, Junio C Hamano wrote:\n> David Aguilar writes:\n>\n>> Use mktemp to create the /dev/null placeholder for p4merge.\n>> This keeps it out of the current directory.\n>>\n>> Reported-by: Jeremy Morton \n>> Signed-off-by: David Aguilar \n>> ---\n>> I consider this a final finishing touch on a new 1.8.1 feature,\n>> so hopefully we can get this in before 1.8.1.\n>\n> Does everybody have mktemp(1), which is not even in POSIX.1?\n>\n> I'm a bit hesitant to apply this to the upcoming release without\n> cooking it in 'next' for sufficiently long time to give it a chance\n> to be tried by wider audience.\n\nTrue. I only tried Linux and Mac OS X, so I would not be\nsurprised to find some exotic UNIX that does not have mktemp.\n\nI meant to write \"is this portable?\" in the section after\nthe double-dash; saying that it's polish for a fix\nis only true if it's portable.\n\nThe git-unpack thing looks interesting...\nis the SHA-1 in your example the special SHA-1 for an\nempty tree or blob?\n\nThe reason I ask is because in this code path we are\ncomparing an unknown blob, basically a blob that does\nnot exist in one of the trees, so I'm not sure if an\n'unpack' command would help for this case because we\nwould not have a blob SHA-1 to unpack.\n\n\nAs far as portability goes, the \"UNIX\" list\nfor p4merge is here:\n\nhttp://www.perforce.com/downloads/complete_list\n\nI do not have AIX, Solaris, or FreeBSD to test,\nso I agree that this can wait.\n\nDoes msysgit have mktemp?\n\np4merge is available on Windows too so I would\nneed to check there too unless someone happens\nto know off the top of their heads.\n\nMy other thought was to write a simple shell\nfunction that checks TMPDIR itself, and defaults\nto creating some file under /tmp when it is undefined.\nI can pursue this option if you think it's a safer choice.\n-- \nDavid\n"},{"id":"205250","messageId":"kauhoq$g9n$1@ger.gmane.org","threadId":"32413","inReplyTo":"7v623x6xvv.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] mergetools/p4merge: Honor $TMPDIR for the /dev/null placeholder","fromName":"Joachim Schmitz","fromEmail":"jojo@schmitz-digital.de","sentAt":"2012-12-20T08:20:35Z","receivedAt":"2012-12-20T08:20:35Z","isPatch":true,"sender":{"key":"jojo@schmitz-digital.de","avatar":"https://avatars.githubusercontent.com/u/1786669?v=4"},"body":"Junio C Hamano wrote:\n> David Aguilar writes:\n>\n>> Use mktemp to create the /dev/null placeholder for p4merge.\n>> This keeps it out of the current directory.\n>>\n>> Reported-by: Jeremy Morton \n>> Signed-off-by: David Aguilar \n>> ---\n>> I consider this a final finishing touch on a new 1.8.1 feature,\n>> so hopefully we can get this in before 1.8.1.\n>\n> Does everybody have mktemp(1), which is not even in POSIX.1?\n\nHP-NonStop doesn't have it, unless you're on the latest release, which added \nGNU coreutils.\n\nBye, Jojo \n"}]}