Re: [PATCH v2] mergetools/p4merge: Honor $TMPDIR for the /dev/null placeholder
On Fri, Dec 21, 2012 at 8:08 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 9 quoted lines
> David Aguilar <davvid@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.
Show 10 quoted lines
>> +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
David Aguilar