Re: [FEATURE] difftool to provide meaningful names for temporary files

Re: [FEATURE] difftool to provide meaningful names for temporary files

[Eugene Sajine]

Hi,

I noticed many times already that it is at times confusing to
determine for new users which file versions are shown where in the
graphical told like gvimdiff or kompare.

The problem is that when difftool machinery creates a temporary files
to show in the editor it is creating them with some random hashes in
the beginning of the file name.

Is it possible to make the difftool and mergetool smarter to have them
to create those temporary files with meaningful names, f.e.:

If I specify:
Git difftool master dev
It will create the files with master and dev prefixes correspondingly,
it will take the two points names and use them in temp file names.
Master_file.txt vs dev_file.txt

If I specify:
Git difftool
It could take HEAD and local as name prefixes

Same for mergetool where it could use HEAD and incoming branch name as
prefixes or something like that.

Does it make sense?

Thanks,
Eugene

Re: [FEATURE] difftool to provide meaningful names for temporary files

[David Aguilar]

On Wed, May 18, 2011 at 05:54:02PM -0400, Eugene Sajine wrote:
> Hi,
> 
> I noticed many times already that it is at times confusing to
> determine for new users which file versions are shown where in the
> graphical told like gvimdiff or kompare.
> 
> The problem is that when difftool machinery creates a temporary files
> to show in the editor it is creating them with some random hashes in
> the beginning of the file name.
> 
> Is it possible to make the difftool and mergetool smarter to have them
> to create those temporary files with meaningful names, f.e.:
> 
> If I specify:
> Git difftool master dev
> It will create the files with master and dev prefixes correspondingly,
> it will take the two points names and use them in temp file names.
> Master_file.txt vs dev_file.txt
> 
> If I specify:
> Git difftool
> It could take HEAD and local as name prefixes
> 
> Same for mergetool where it could use HEAD and incoming branch name as
> prefixes or something like that.
> 
> Does it make sense?

That makes sense, but part of the reason why we use
mkstemp() is to make the paths random.

This is better then what it was like before, though --
they used to be completely random! ;-)

I think the randomness is a little bit of a security feature.
I wouldn't want to discourage you from trying to make it better,
though.  Here's how I first improved it.  Maybe you can take
this as a starting point for making it better?

# david@lustrous:~/src/git (master)
% git show 003b33a8ad686ee4a0d0b36635bfd6aba940b24a
commit 003b33a8ad686ee4a0d0b36635bfd6aba940b24a
Author: David Aguilar <davvid@gmail.com>
Date:   Sun May 31 01:35:52 2009 -0700

    diff: generate pretty filenames in prep_temp_blob()
    
    Naturally, prep_temp_blob() did not care about filenames.
    As a result, GIT_EXTERNAL_DIFF and textconv generated
    filenames such as ".diff_XXXXXX".
    
    This modifies prep_temp_blob() to generate user-friendly
    filenames when creating temporary files.
    
    Diffing "name.ext" now generates "XXXXXX_name.ext".
    
    Signed-off-by: David Aguilar <davvid@gmail.com>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/cache.h b/cache.h
index b8503ad..871c984 100644
--- a/cache.h
+++ b/cache.h
@@ -614,6 +614,8 @@ extern int is_empty_blob_sha1(const unsigned char *sha1);
 
 int git_mkstemp(char *path, size_t n, const char *template);
 
+int git_mkstemps(char *path, size_t n, const char *template, int suffix_len);
+
 /*
  * NOTE NOTE NOTE!!
  *
diff --git a/diff.c b/diff.c
index dcfbcb0..4d0a5b9 100644
--- a/diff.c
+++ b/diff.c
@@ -1964,8 +1964,16 @@ static void prep_temp_blob(const char *path, struct diff_tempfile *temp,
 {
 	int fd;
 	struct strbuf buf = STRBUF_INIT;
+	struct strbuf template = STRBUF_INIT;
+	char *path_dup = xstrdup(path);
+	const char *base = basename(path_dup);
 
-	fd = git_mkstemp(temp->tmp_path, PATH_MAX, ".diff_XXXXXX");
+	/* Generate "XXXXXX_basename.ext" */
+	strbuf_addstr(&template, "XXXXXX_");
+	strbuf_addstr(&template, base);
+
+	fd = git_mkstemps(temp->tmp_path, PATH_MAX, template.buf,
+			strlen(base) + 1);
 	if (fd < 0)
 		die("unable to create temp-file: %s", strerror(errno));
 	if (convert_to_working_tree(path,
@@ -1981,6 +1989,8 @@ static void prep_temp_blob(const char *path, struct diff_tempfile *temp,
 	temp->hex[40] = 0;
 	sprintf(temp->mode, "%06o", mode);
 	strbuf_release(&buf);
+	strbuf_release(&template);
+	free(path_dup);
 }
 
 static struct diff_tempfile *prepare_temp_file(const char *name,
diff --git a/path.c b/path.c
index 8a0a674..047fdb0 100644
--- a/path.c
+++ b/path.c
@@ -139,6 +139,22 @@ int git_mkstemp(char *path, size_t len, const char *template)
 	return mkstemp(path);
 }
 
+/* git_mkstemps() - create tmp file with suffix honoring TMPDIR variable. */
+int git_mkstemps(char *path, size_t len, const char *template, int suffix_len)
+{
+	const char *tmp;
+	size_t n;
+
+	tmp = getenv("TMPDIR");
+	if (!tmp)
+		tmp = "/tmp";
+	n = snprintf(path, len, "%s/%s", tmp, template);
+	if (len <= n) {
+		errno = ENAMETOOLONG;
+		return -1;
+	}
+	return mkstemps(path, suffix_len);
+}
 
 int validate_headref(const char *path)
 {
diff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh
index 0720001..4ea42e0 100755
--- a/t/t4020-diff-external.sh
+++ b/t/t4020-diff-external.sh
@@ -136,6 +136,15 @@ test_expect_success 'GIT_EXTERNAL_DIFF with more than one changed files' '
 	GIT_EXTERNAL_DIFF=echo git diff
 '
 
+test_expect_success 'GIT_EXTERNAL_DIFF generates pretty paths' '
+	touch file.ext &&
+	git add file.ext &&
+	echo with extension > file.ext &&
+	GIT_EXTERNAL_DIFF=echo git diff file.ext | grep ......_file\.ext &&
+	git update-index --force-remove file.ext &&
+	rm file.ext
+'
+
 echo "#!$SHELL_PATH" >fake-diff.sh
 cat >> fake-diff.sh <<\EOF
 cat $2 >> crlfed.txt

-- 
					David