Re: [PATCH v2] mergetools/p4merge: Honor $TMPDIR for the /dev/null placeholder
On Fri, Dec 21, 2012 at 8:08 AM, Junio C Hamano gits...@pobox.com wrote:
David Aguilar dav...@gmail.com writes:
Use $TMPDIR when creating the /dev/null placeholder for p4merge.
This keeps it out of the current directory.
The usual $REMOTE this is theirs and $LOCAL this is ours are
still created in the current directory, no? It is unclear why this
this side does not exist case wants to be outside of the current
directory in the first place.
I'll take this as a hint that I should improve the commit message.
As you alluded to in your earlier email, we are in feature freeze
so I will be holding off on sending it until things have settled.
I don't believe there is a strong reason why the file should be in one
place vs. another, and the explanation is different depending on who
is driving the scriptlet.
When difftool drives then $LOCAL and $REMOTE may point to
temporary files created by the GIT_EXTERNAL_DIFF machinery.
For that use case, this commit makes things consistent with
those location of paths.
When mergetool drives it creates $LOCAL and $REMOTE in
the current directory. They contain the different stages
of the index and can be helpful to the user when resolving merges.
The temporary /dev/null placeholder is a workaround for p4merge
and from the user's point of view this file is never interesting, so
writing it into the worktree is distracting to the user.
I could also say that it makes it consistent because /dev is
also not in the current directory, but that's a stretch ;-)
In other words, This keeps it out of the current directory only
explains what this patch changes, without explaining why it is a
good thing to do in the first place.
I'll try and write up a replacement patch but I'll hold off
on sending it out until after things have settled down.
FWIW I'm seeing a Bus Error when doing git update-index --refresh
in a repository with large files on a 32bit machine. I'm not sure if
that counts as a regression since the same error occurs in 1.7.10.4
(debian testing).
I'll start another thread on this topic in case anyone is interested.
+create_empty_file () {
+ empty_file=${TMPDIR:-/tmp}/git-difftool-p4merge-empty-file.$$
+ $empty_file
+
+ printf $empty_file
+}
Assuming that it makes sense to create only the this side doe not
exist, and here is a placeholder empty file in $TMPDIR, I think
this is probably sufficient.
Yup. I mulled over whether or not to embed LOCAL and REMOTE
in the name but eventually decided that it was not worth the effort.
It makes the code much simpler when they share the placeholder
since we no longer need to track them separately.
--
David
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] mergetools/p4merge: Honor $TMPDIR for the /dev/null placeholder
On Sat, Dec 22, 2012 at 1:56 PM, David Aguilar dav...@gmail.com wrote: FWIW I'm seeing a Bus Error when doing git update-index --refresh in a repository with large files on a 32bit machine. I'm not sure if that counts as a regression since the same error occurs in 1.7.10.4 (debian testing). I'll start another thread on this topic in case anyone is interested. Nevermind. The machine is an old salvaged laptop and its disk probably has some bad blocks. I wasn't able to copy the bad repo since 'cp' kept failing to copy the index file. I/O errors.. After deleting the borked index the problems went away, so I'll chalk this up to failing hardware. Sorry for the false alarm. -- David -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] mergetools/p4merge: Honor $TMPDIR for the /dev/null placeholder
David Aguilar dav...@gmail.com writes:
Use $TMPDIR when creating the /dev/null placeholder for p4merge.
This keeps it out of the current directory.
The usual $REMOTE this is theirs and $LOCAL this is ours are
still created in the current directory, no? It is unclear why this
this side does not exist case wants to be outside of the current
directory in the first place.
In other words, This keeps it out of the current directory only
explains what this patch changes, without explaining why it is a
good thing to do in the first place.
+create_empty_file () {
+ empty_file=${TMPDIR:-/tmp}/git-difftool-p4merge-empty-file.$$
+ $empty_file
+
+ printf $empty_file
+}
Assuming that it makes sense to create only the this side doe not
exist, and here is a placeholder empty file in $TMPDIR, I think
this is probably sufficient.
By the way, who is going to remove this temporary file once the
command is done?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] mergetools/p4merge: Honor $TMPDIR for the /dev/null placeholder
Junio C Hamano gits...@pobox.com writes: By the way, who is going to remove this temporary file once the command is done? Nevermind; I can see that once the backend returns it is removed in the same function. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] mergetools/p4merge: Honor $TMPDIR for the /dev/null placeholder
Use $TMPDIR when creating the /dev/null placeholder for p4merge.
This keeps it out of the current directory.
Reported-by: Jeremy Morton ad...@game-point.net
Signed-off-by: David Aguilar dav...@gmail.com
---
No mktemp usage in this round.
mergetools/p4merge | 27 +--
1 file changed, 13 insertions(+), 14 deletions(-)
diff --git a/mergetools/p4merge b/mergetools/p4merge
index 295361a..52f7c8f 100644
--- a/mergetools/p4merge
+++ b/mergetools/p4merge
@@ -1,29 +1,21 @@
diff_cmd () {
+ empty_file=
+
# p4merge does not like /dev/null
- rm_local=
- rm_remote=
if test /dev/null = $LOCAL
then
- LOCAL=./p4merge-dev-null.LOCAL.$$
- $LOCAL
- rm_local=true
+ LOCAL=$(create_empty_file)
fi
if test /dev/null = $REMOTE
then
- REMOTE=./p4merge-dev-null.REMOTE.$$
- $REMOTE
- rm_remote=true
+ REMOTE=$(create_empty_file)
fi
$merge_tool_path $LOCAL $REMOTE
- if test -n $rm_local
- then
- rm -f $LOCAL
- fi
- if test -n $rm_remote
+ if test -n $empty_file
then
- rm -f $REMOTE
+ rm -f $empty_file
fi
}
@@ -33,3 +25,10 @@ merge_cmd () {
$merge_tool_path $BASE $LOCAL $REMOTE $MERGED
check_unchanged
}
+
+create_empty_file () {
+ empty_file=${TMPDIR:-/tmp}/git-difftool-p4merge-empty-file.$$
+ $empty_file
+
+ printf $empty_file
+}
--
1.8.1.rc2.6.g18499ba
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
