[PATCH 6/9] difftool: replace system call with Git::command_noisy

The Git.pm module includes functions intended to standardize working
with Git repositories in Perl scripts. This commit teaches difftool
to use Git::command_noisy rather than a system call to run the diff
command.

Signed-off-by: Tim Henigan <redacted>
---
 git-difftool.perl |   10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index 9495f14..8498089 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl

@@ -72,12 +72,4 @@ elsif (defined($no_prompt)) {
 
 $ENV{GIT_PAGER} = '';
 $ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';
-my @command = ('git', 'diff', @ARGV);
-
-# ActiveState Perl for Win32 does not implement POSIX semantics of
-# exec* system call. It just spawns the given executable and finishes
-# the starting program, exiting with code 0.
-# system will at least catch the errors returned by git diff,
-# allowing the caller of git difftool better handling of failures.
-my $rc = system(@command);
-exit($rc | ($rc >> 8));
+git_cmd_try { Git::command_noisy(('diff', @ARGV)) } 'exit code %d';

-- 
1.7.9.1.290.gbd444

On Fri, Mar 16, 2012 at 6:59 PM, Tim Henigan [off-list ref] wrote:

The Git.pm module includes functions intended to standardize working
with Git repositories in Perl scripts. This commit teaches difftool
to use Git::command_noisy rather than a system call to run the diff
command.

Git::command_noisy() calls _cmd_exec() which calls _execv_git_cmd()
which does a fork() + exec('git', @_) + waitpid();

We were avoiding exec() for portability reasons, as Alex explained in
677fbff88f368ed6ac52438ddbb530166ec1d5d1:

# ActiveState Perl for Win32 does not implement POSIX semantics of
# exec* system call. It just spawns the given executable and finishes
# the starting program, exiting with code 0.
# system will at least catch the errors returned by git diff,
# allowing the caller of git difftool better handling of failures.

Is this no longer a concern?  Does Git.pm need a similar portability
caveat, or  does it avoid the problem altogether since it uses fork()
+ exec() + waitpid()?  (if this is true then it implies that this
change is fine).

I have not read the rest of this series yet, so apologies if these
questions were answered elsewhere.

In general, I am a little nervous about having difftool copy worktree
content somewhere temporary only to copy it back in later.  Is there
some way to make the diff machinery reuse the worktree?  I was under
the impression that we could do some GIT_INDEX tricks to do it, though
I will admit that I did not read that suggestion in depth, nor did I
grasp whether this was the problem it was meant to address.

Thoughts?

-- 
David

Resend for vger archives. Damn that Android GMail client.

On Sat, Mar 17, 2012 at 03:48, David Aguilar [off-list ref] wrote:

On Fri, Mar 16, 2012 at 6:59 PM, Tim Henigan [off-list ref] wrote:

quoted

The Git.pm module includes functions intended to standardize working
with Git repositories in Perl scripts. This commit teaches difftool
to use Git::command_noisy rather than a system call to run the diff
command.

Git::command_noisy() calls _cmd_exec() which calls _execv_git_cmd()
which does a fork() + exec('git', @_) + waitpid();

We were avoiding exec() for portability reasons, as Alex explained in
677fbff88f368ed6ac52438ddbb530166ec1d5d1:

# ActiveState Perl for Win32 does not implement POSIX semantics of
# exec* system call. It just spawns the given executable and finishes
# the starting program, exiting with code 0.
# system will at least catch the errors returned by git diff,
# allowing the caller of git difftool better handling of failures.

Is this no longer a concern?  Does Git.pm need a similar portability
caveat, or  does it avoid the problem altogether since it uses fork()
+ exec() + waitpid()?  (if this is true then it implies that this
change is fine).

It _might_ work. Cygwin kind of has fork(2), it even works (kind of:
it is a *very* expensive thing to do). There are also other ifs and
whens, but it is worth a test. It's a nice clean up to have.

On Sat, Mar 17, 2012 at 6:50 AM, Alex Riesen [off-list ref] wrote:

Resend for vger archives. Damn that Android GMail client.

On Sat, Mar 17, 2012 at 03:48, David Aguilar [off-list ref] wrote:

quoted

On Fri, Mar 16, 2012 at 6:59 PM, Tim Henigan [off-list ref] wrote:

quoted

The Git.pm module includes functions intended to standardize working
with Git repositories in Perl scripts. This commit teaches difftool
to use Git::command_noisy rather than a system call to run the diff
command.

Git::command_noisy() calls _cmd_exec() which calls _execv_git_cmd()
which does a fork() + exec('git', @_) + waitpid();

We were avoiding exec() for portability reasons, as Alex explained in
677fbff88f368ed6ac52438ddbb530166ec1d5d1:

# ActiveState Perl for Win32 does not implement POSIX semantics of
# exec* system call. It just spawns the given executable and finishes
# the starting program, exiting with code 0.
# system will at least catch the errors returned by git diff,
# allowing the caller of git difftool better handling of failures.

Is this no longer a concern?  Does Git.pm need a similar portability
caveat, or  does it avoid the problem altogether since it uses fork()
+ exec() + waitpid()?  (if this is true then it implies that this
change is fine).

I need to spend more time testing this.  On Windows, I have tested
with msysgit but not cygwin.  Was ActiveState Perl used with cygwin
git?

It _might_ work. Cygwin kind of has fork(2), it even works (kind of:
it is a *very* expensive thing to do). There are also other ifs and
whens, but it is worth a test. It's a nice clean up to have.

Even it fork(2) is expensive, in this case it seems reasonable. Given
the time needed to spawn the diff tool, the fork(2) time seems
negligible.

On Sat, Mar 17, 2012 at 15:48, Tim Henigan [off-list ref] wrote:

On Sat, Mar 17, 2012 at 6:50 AM, Alex Riesen [off-list ref] wrote:

quoted

On Sat, Mar 17, 2012 at 03:48, David Aguilar [off-list ref] wrote:

quoted

Is this no longer a concern?  Does Git.pm need a similar portability
caveat, or  does it avoid the problem altogether since it uses fork()
+ exec() + waitpid()?  (if this is true then it implies that this
change is fine).

I need to spend more time testing this.  On Windows, I have tested
with msysgit but not cygwin.  Was ActiveState Perl used with cygwin
git?

Yes, it is even stated in the commentary.

As far as I know, there is only one installation where a cygwin-compiled
Git is used with the ActiveState Perl (mine. I believe we would have
heard if there were others - it is an extremely annoying combination).

quoted

It _might_ work. Cygwin kind of has fork(2), it even works (kind of:
it is a *very* expensive thing to do). There are also other ifs and
whens, but it is worth a test. It's a nice clean up to have.

Even it fork(2) is expensive, in this case it seems reasonable. Given
the time needed to spawn the diff tool, the fork(2) time seems
negligible.

Not Cygwin's fork. They really do a deep copy of parent process.

But actually, I misunderstood. The Perl used was of ActiveState origin.
So the code in question is not affected by Cygwin at all.
I have no idea how usable fork(2) of ActiveState Perl is.

Even if it is bad, I won't be really affected, I very seldom use the
difftool, and I believe I never used it on that particular system.

I try test the patch in the next days. I'll tell if something is
completely broken.

On Fri, Mar 16, 2012 at 10:48 PM, David Aguilar [off-list ref] wrote:

On Fri, Mar 16, 2012 at 6:59 PM, Tim Henigan [off-list ref] wrote:

In general, I am a little nervous about having difftool copy worktree
content somewhere temporary only to copy it back in later.  Is there
some way to make the diff machinery reuse the worktree?  I was under
the impression that we could do some GIT_INDEX tricks to do it, though
I will admit that I did not read that suggestion in depth, nor did I
grasp whether this was the problem it was meant to address.

I have not been able to find any other way to do it.  The GIT_INDEX
trick allows the tmp directories to be built using 'git update-index'
and 'git checkout-index', but they offer no help for this problem.

If we use the working tree directory as one of the diff targets, then
all the files in the working directory would be included in the
diff...unless there was some way to remove the files that aren't part
of the diff from the working tree.  However at that point, I don't
think the solution would be any better (i.e. deleting files from the
working tree and then checking them back out is no better than copying
files to the tmp dir and back again).

The only other option I can think of is to build a complete copy of
the repo in the tmp directory for comparison against the working tree.
 However, this could obviously lead to resource/performance problems
on large repos.

I am open to suggestions, but I have not found any better solution.