git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* Fixing the p4merge launch shell script
@ 2012-10-02 23:38 Jeremy Morton
  2012-10-03  0:44 ` Junio C Hamano
  0 siblings, 1 reply; 4+ messages in thread
From: Jeremy Morton @ 2012-10-02 23:38 UTC (permalink / raw)
  To: git

Hi,

I've noticed that the p4merge shell script could do with some 
improvement when it comes to merging.  Because p4merge throws up an 
error when one of the files it's given to diff is "/dev/null", git needs 
to create a temporary empty file and pass that to p4merge when diffing a 
file that has been created/deleted (eg. create file, git add ., git diff 
--cached).

At the moment, the hack I'm using, which works, is to create a 
c:/blank.txt file which is an empty file, and modify 
"libexec/git-core/mergetools/p4merge" to start with this:

diff_cmd () {
	if [ ! -f "$LOCAL" ]
	then
		LOCAL="C:/blank.txt"
	fi
	if [ ! -f "$REMOTE" ]
	then
		REMOTE="C:/blank.txt"
	fi
	"$merge_tool_path" "$LOCAL" "$REMOTE"
}

Obviously, this isn't good enough because c:/blank.txt probably won't 
exist on the system.  It would be nice for the temporary blank file to 
have the same extension as the one that's been added, so we could diff 
(say) c:/users/jez/Temp/abc123_newFile.foo 
c:/development/bar/newfile.foo.  However, I can't see a way to do this 
purely from within the shell script (even `mktemp` doesn't exist on all 
systems so that isn't good enough).  I believe git needs to create this 
temporary blank file itself, much like it creates a temporary file for 
the previous version of a file that's been modified when it's being 
diff'd.  It then needs to expose the location of this temporary file as 
a variable; say $LOCALBLANK.  So, we would be able to modify the shell 
script to this:


diff_cmd () {
	if [ ! -f "$LOCAL" ]
	then
		LOCAL=$LOCALBLANK
	fi
	if [ ! -f "$REMOTE" ]
	then
		REMOTE=$REMOTEBLANK
	fi
	"$merge_tool_path" "$LOCAL" "$REMOTE"
}

Thoughts?  Is there an easier way to do this?

-- 
Best regards,
Jeremy Morton (Jez)

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: Fixing the p4merge launch shell script
  2012-10-02 23:38 Fixing the p4merge launch shell script Jeremy Morton
@ 2012-10-03  0:44 ` Junio C Hamano
  2012-10-03  8:34   ` Jeremy Morton
  0 siblings, 1 reply; 4+ messages in thread
From: Junio C Hamano @ 2012-10-03  0:44 UTC (permalink / raw)
  To: Jeremy Morton; +Cc: git

Jeremy Morton <admin@game-point.net> writes:

> I've noticed that the p4merge shell script could do with some
> improvement when it comes to merging.  Because p4merge throws up an
> error when one of the files it's given to diff is "/dev/null", git
> needs to create a temporary empty file and pass that to p4merge when
> diffing a file that has been created/deleted (eg. create file, git add
> ., git diff --cached).
> ...
> Thoughts?  Is there an easier way to do this?

Which version of git?  Perhaps you do not have ec245ba (mergetool:
Provide an empty file when needed, 2012-01-19) yet?

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: Fixing the p4merge launch shell script
  2012-10-03  0:44 ` Junio C Hamano
@ 2012-10-03  8:34   ` Jeremy Morton
  2012-10-04  6:59     ` David Aguilar
  0 siblings, 1 reply; 4+ messages in thread
From: Jeremy Morton @ 2012-10-03  8:34 UTC (permalink / raw)
  To: git

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

> 
> Jeremy Morton <admin <at> game-point.net> writes:
> 
> > I've noticed that the p4merge shell script could do with some
> > improvement when it comes to merging.  Because p4merge throws up an
> > error when one of the files it's given to diff is "/dev/null", git
> > needs to create a temporary empty file and pass that to p4merge when
> > diffing a file that has been created/deleted (eg. create file, git add
> > ., git diff --cached).
> > ...
> > Thoughts?  Is there an easier way to do this?
> 
> Which version of git?  Perhaps you do not have ec245ba (mergetool:
> Provide an empty file when needed, 2012-01-19) yet?
> 

That patch fixes the mergetool part, but the part I was referring to was the
difftool part, which still has this problem.

Best regards,
Jeremy Morton (Jez)

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: Fixing the p4merge launch shell script
  2012-10-03  8:34   ` Jeremy Morton
@ 2012-10-04  6:59     ` David Aguilar
  0 siblings, 0 replies; 4+ messages in thread
From: David Aguilar @ 2012-10-04  6:59 UTC (permalink / raw)
  To: Jeremy Morton; +Cc: git, Junio C Hamano

On Wed, Oct 3, 2012 at 1:34 AM, Jeremy Morton <admin@game-point.net> wrote:
> Junio C Hamano <gitster <at> pobox.com> writes:
>
>>
>> Jeremy Morton <admin <at> game-point.net> writes:
>>
>> > I've noticed that the p4merge shell script could do with some
>> > improvement when it comes to merging.  Because p4merge throws up an
>> > error when one of the files it's given to diff is "/dev/null", git
>> > needs to create a temporary empty file and pass that to p4merge when
>> > diffing a file that has been created/deleted (eg. create file, git add
>> > ., git diff --cached).
>> > ...
>> > Thoughts?  Is there an easier way to do this?
>>
>> Which version of git?  Perhaps you do not have ec245ba (mergetool:
>> Provide an empty file when needed, 2012-01-19) yet?
>>
>
> That patch fixes the mergetool part, but the part I was referring to was the
> difftool part, which still has this problem.
>
> Best regards,
> Jeremy Morton (Jez)

Thanks for the heads-up, Jez.

I should have some time to take a look this weekend.
If someone beats me to it I'll be happy to review the patches before
then (Jeremy? ;-)

Is `git diff` giving us /dev/null as the path here,
and p4merge doesn't like that, or is something else
going on?

If that's indeed happening, then I would imagine
a sensible thing for git-difftool--helper to do
would be to check for /dev/null and create some
temporary empty file.

Does that sound like the right way to go?
Another option would be for git-diff's external diff
code to handle it at a much earlier point, perhaps
guarded behind an option that git-difftool knows to
pass (changing existing behavior is off the table).

These both sound like pretty big changes given
that we're getting ready for 1.8.0.

If it's just p4merge that doesn't like this then
perhaps the right decision is to paper over it
in p4merge's mergetools/ scriptlet.

It's certainly the least-impactful option.  Thoughts?
-- 
David

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2012-10-04 22:01 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-10-02 23:38 Fixing the p4merge launch shell script Jeremy Morton
2012-10-03  0:44 ` Junio C Hamano
2012-10-03  8:34   ` Jeremy Morton
2012-10-04  6:59     ` David Aguilar

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).