[PATCH] mergetools/p4merge: Honor $TMPDIR for the /dev/null placeholder

[PATCH] mergetools/p4merge: Honor $TMPDIR for the /dev/null placeholder

[David Aguilar]

Use mktemp to create the /dev/null placeholder for p4merge.
This keeps it out of the current directory.

Reported-by: Jeremy Morton <admin@game-point.net>
Signed-off-by: David Aguilar <davvid@gmail.com>
---
I consider this a final finishing touch on a new 1.8.1 feature,
so hopefully we can get this in before 1.8.1.

 mergetools/p4merge | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/mergetools/p4merge b/mergetools/p4merge
index 295361a..090fa9b 100644
--- a/mergetools/p4merge
+++ b/mergetools/p4merge
@@ -4,13 +4,13 @@ diff_cmd () {
 	rm_remote=
 	if test "/dev/null" = "$LOCAL"
 	then
-		LOCAL="./p4merge-dev-null.LOCAL.$$"
+		LOCAL="$(create_empty_file)"
 		>"$LOCAL"
 		rm_local=true
 	fi
 	if test "/dev/null" = "$REMOTE"
 	then
-		REMOTE="./p4merge-dev-null.REMOTE.$$"
+		REMOTE="$(create_empty_file)"
 		>"$REMOTE"
 		rm_remote=true
 	fi
@@ -33,3 +33,7 @@ merge_cmd () {
 	"$merge_tool_path" "$BASE" "$LOCAL" "$REMOTE" "$MERGED"
 	check_unchanged
 }
+
+create_empty_file () {
+	mktemp -t git-difftool-p4merge-empty-file.XXXXXX
+}
-- 
1.8.1.rc2.6.g18499ba

Re: [PATCH] mergetools/p4merge: Honor $TMPDIR for the /dev/null placeholder

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> Use mktemp to create the /dev/null placeholder for p4merge.
> This keeps it out of the current directory.
>
> Reported-by: Jeremy Morton <admin@game-point.net>
> Signed-off-by: David Aguilar <davvid@gmail.com>
> ---
> I consider this a final finishing touch on a new 1.8.1 feature,
> so hopefully we can get this in before 1.8.1.

Does everybody have mktemp(1), which is not even in POSIX.1?

I'm a bit hesitant to apply this to the upcoming release without
cooking it in 'next' for sufficiently long time to give it a chance
to be tried by wider audience.

>  mergetools/p4merge | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/mergetools/p4merge b/mergetools/p4merge
> index 295361a..090fa9b 100644
> --- a/mergetools/p4merge
> +++ b/mergetools/p4merge
> @@ -4,13 +4,13 @@ diff_cmd () {
>  	rm_remote=
>  	if test "/dev/null" = "$LOCAL"
>  	then
> -		LOCAL="./p4merge-dev-null.LOCAL.$$"
> +		LOCAL="$(create_empty_file)"
>  		>"$LOCAL"
>  		rm_local=true
>  	fi
>  	if test "/dev/null" = "$REMOTE"
>  	then
> -		REMOTE="./p4merge-dev-null.REMOTE.$$"
> +		REMOTE="$(create_empty_file)"
>  		>"$REMOTE"
>  		rm_remote=true
>  	fi
> @@ -33,3 +33,7 @@ merge_cmd () {
>  	"$merge_tool_path" "$BASE" "$LOCAL" "$REMOTE" "$MERGED"
>  	check_unchanged
>  }
> +
> +create_empty_file () {
> +	mktemp -t git-difftool-p4merge-empty-file.XXXXXX
> +}

Re: [PATCH] mergetools/p4merge: Honor $TMPDIR for the /dev/null placeholder

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> David Aguilar <davvid@gmail.com> writes:
>
>> Use mktemp to create the /dev/null placeholder for p4merge.
>> This keeps it out of the current directory.
>>
>> Reported-by: Jeremy Morton <admin@game-point.net>
>> Signed-off-by: David Aguilar <davvid@gmail.com>
>> ---
>> I consider this a final finishing touch on a new 1.8.1 feature,
>> so hopefully we can get this in before 1.8.1.
>
> Does everybody have mktemp(1), which is not even in POSIX.1?
>
> I'm a bit hesitant to apply this to the upcoming release without
> cooking it in 'next' for sufficiently long time to give it a chance
> to be tried by wider audience.

One approach may be to do something like this as a preparation step
(current callers of unpack-file may want to do their temporary work
in TMPDIR as well), and then use

	git unpack-file -t e69de29bb2d1d6434b8b29ae775ad8c2e48c5391

to create your empty temporary file.

 Documentation/git-unpack-file.txt | 10 +++++++---
 builtin/unpack-file.c             | 28 +++++++++++++++++++++-------
 2 files changed, 28 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-unpack-file.txt b/Documentation/git-unpack-file.txt
index e9f148a..56af328 100644
--- a/Documentation/git-unpack-file.txt
+++ b/Documentation/git-unpack-file.txt
@@ -10,16 +10,20 @@ git-unpack-file - Creates a temporary file with a blob's contents
 SYNOPSIS
 --------
 [verse]
-'git unpack-file' <blob>
+'git unpack-file' [-t] <blob>
 
 DESCRIPTION
 -----------
 Creates a file holding the contents of the blob specified by sha1. It
-returns the name of the temporary file in the following format:
-	.merge_file_XXXXX
+returns the name of the temporary file (by default `.merge_file_XXXXX`).
+
 
 OPTIONS
 -------
+-t::
+	The temporary file is created in `$TMPDIR` directory,
+	instead of the current directory.
+
 <blob>::
 	Must be a blob id
 
diff --git a/builtin/unpack-file.c b/builtin/unpack-file.c
index 1920029..de1f845 100644
--- a/builtin/unpack-file.c
+++ b/builtin/unpack-file.c
@@ -1,8 +1,9 @@
 #include "builtin.h"
 
-static char *create_temp_file(unsigned char *sha1)
+static char *create_temp_file(unsigned char *sha1, int in_tempdir)
 {
-	static char path[50];
+	static char path[1024];
+	static const char template[] = ".merge_file_XXXXXX";
 	void *buf;
 	enum object_type type;
 	unsigned long size;
@@ -12,8 +13,12 @@ static char *create_temp_file(unsigned char *sha1)
 	if (!buf || type != OBJ_BLOB)
 		die("unable to read blob object %s", sha1_to_hex(sha1));
 
-	strcpy(path, ".merge_file_XXXXXX");
-	fd = xmkstemp(path);
+	if (in_tempdir) {
+		fd = git_mkstemp(path, sizeof(path) - 1, template);
+	} else {
+		strcpy(path, template);
+		fd = xmkstemp(path);
+	}
 	if (write_in_full(fd, buf, size) != size)
 		die_errno("unable to write temp-file");
 	close(fd);
@@ -23,14 +28,23 @@ static char *create_temp_file(unsigned char *sha1)
 int cmd_unpack_file(int argc, const char **argv, const char *prefix)
 {
 	unsigned char sha1[20];
+	int in_tempdir = 0;
+
+	if (argc < 2 || 3 < argc || !strcmp(argv[1], "-h"))
+		usage("git unpack-file [-t] <sha1>");
+	if (argc == 3) {
+		if (strcmp(argv[1], "-t"))
+			usage("git unpack-file [-t] <sha1>");
+		in_tempdir = 1;
+		argc--;
+		argv++;
+	}
 
-	if (argc != 2 || !strcmp(argv[1], "-h"))
-		usage("git unpack-file <sha1>");
 	if (get_sha1(argv[1], sha1))
 		die("Not a valid object name %s", argv[1]);
 
 	git_config(git_default_config, NULL);
 
-	puts(create_temp_file(sha1));
+	puts(create_temp_file(sha1, in_tempdir));
 	return 0;
 }

Re: [PATCH] mergetools/p4merge: Honor $TMPDIR for the /dev/null placeholder

[David Aguilar]

On Wed, Dec 19, 2012 at 10:28 PM, Junio C Hamano <gitster@pobox.com> wrote:
> David Aguilar <davvid@gmail.com> writes:
>
>> Use mktemp to create the /dev/null placeholder for p4merge.
>> This keeps it out of the current directory.
>>
>> Reported-by: Jeremy Morton <admin@game-point.net>
>> Signed-off-by: David Aguilar <davvid@gmail.com>
>> ---
>> I consider this a final finishing touch on a new 1.8.1 feature,
>> so hopefully we can get this in before 1.8.1.
>
> Does everybody have mktemp(1), which is not even in POSIX.1?
>
> I'm a bit hesitant to apply this to the upcoming release without
> cooking it in 'next' for sufficiently long time to give it a chance
> to be tried by wider audience.

True.  I only tried Linux and Mac OS X, so I would not be
surprised to find some exotic UNIX that does not have mktemp.

I meant to write "is this portable?" in the section after
the double-dash; saying that it's polish for a fix
is only true if it's portable.

The git-unpack thing looks interesting...
is the SHA-1 in your example the special SHA-1 for an
empty tree or blob?

The reason I ask is because in this code path we are
comparing an unknown blob, basically a blob that does
not exist in one of the trees, so I'm not sure if an
'unpack' command would help for this case because we
would not have a blob SHA-1 to unpack.


As far as portability goes, the "UNIX" list
for p4merge is here:

http://www.perforce.com/downloads/complete_list

I do not have AIX, Solaris, or FreeBSD to test,
so I agree that this can wait.

Does msysgit have mktemp?

p4merge is available on Windows too so I would
need to check there too unless someone happens
to know off the top of their heads.

My other thought was to write a simple shell
function that checks TMPDIR itself, and defaults
to creating some file under /tmp when it is undefined.
I can pursue this option if you think it's a safer choice.
-- 
David

Re: [PATCH] mergetools/p4merge: Honor $TMPDIR for the /dev/null placeholder

[Joachim Schmitz]

Junio C Hamano wrote:
> David Aguilar <davvid@gmail.com> writes:
>
>> Use mktemp to create the /dev/null placeholder for p4merge.
>> This keeps it out of the current directory.
>>
>> Reported-by: Jeremy Morton <admin@game-point.net>
>> Signed-off-by: David Aguilar <davvid@gmail.com>
>> ---
>> I consider this a final finishing touch on a new 1.8.1 feature,
>> so hopefully we can get this in before 1.8.1.
>
> Does everybody have mktemp(1), which is not even in POSIX.1?

HP-NonStop doesn't have it, unless you're on the latest release, which added 
GNU coreutils.

Bye, Jojo