{"thread":{"id":"29968","subject":"[PATCH 6/9] difftool: replace system call with Git::command_noisy","startedAt":"2012-03-17T01:59:17Z","lastAt":"2012-03-18T01:21:47Z","messageCount":6,"participants":["Tim Henigan","David Aguilar","Alex Riesen"],"isPatch":true,"patchVersion":1,"patchTotal":9},"messages":[{"id":"187122","messageId":"1331949557-15146-1-git-send-email-tim.henigan@gmail.com","threadId":"29968","inReplyTo":null,"subject":"[PATCH 6/9] difftool: replace system call with Git::command_noisy","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-03-17T01:59:17Z","receivedAt":"2012-03-17T01:59:17Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"The Git.pm module includes functions intended to standardize working\nwith Git repositories in Perl scripts. This commit teaches difftool\nto use Git::command_noisy rather than a system call to run the diff\ncommand.\n\nSigned-off-by: Tim Henigan \n---\n git-difftool.perl | 10 +---------\n 1 file changed, 1 insertion(+), 9 deletions(-)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex 9495f14..8498089 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -72,12 +72,4 @@ elsif (defined($no_prompt)) {\n \n $ENV{GIT_PAGER} = '';\n $ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';\n-my @command = ('git', 'diff', @ARGV);\n-\n-# ActiveState Perl for Win32 does not implement POSIX semantics of\n-# exec* system call. It just spawns the given executable and finishes\n-# the starting program, exiting with code 0.\n-# system will at least catch the errors returned by git diff,\n-# allowing the caller of git difftool better handling of failures.\n-my $rc = system(@command);\n-exit($rc | ($rc >> 8));\n+git_cmd_try { Git::command_noisy(('diff', @ARGV)) } 'exit code %d';\n-- \n1.7.9.1.290.gbd444\n"},{"id":"187128","messageId":"CAJDDKr4+0iWoZhxo6kMVa0YUtDzmrH=XTZnDqQdbnM6TJ41UDg@mail.gmail.com","threadId":"29968","inReplyTo":"1331949557-15146-1-git-send-email-tim.henigan@gmail.com","subject":"Re: [PATCH 6/9] difftool: replace system call with Git::command_noisy","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-03-17T02:48:56Z","receivedAt":"2012-03-17T02:48:56Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Fri, Mar 16, 2012 at 6:59 PM, Tim Henigan wrote:\n> The Git.pm module includes functions intended to standardize working\n> with Git repositories in Perl scripts. This commit teaches difftool\n> to use Git::command_noisy rather than a system call to run the diff\n> command.\n\nGit::command_noisy() calls _cmd_exec() which calls _execv_git_cmd()\nwhich does a fork() + exec('git', @_) + waitpid();\n\nWe were avoiding exec() for portability reasons, as Alex explained in\n677fbff88f368ed6ac52438ddbb530166ec1d5d1:\n\n# ActiveState Perl for Win32 does not implement POSIX semantics of\n# exec* system call. It just spawns the given executable and finishes\n# the starting program, exiting with code 0.\n# system will at least catch the errors returned by git diff,\n# allowing the caller of git difftool better handling of failures.\n\nIs this no longer a concern? Does Git.pm need a similar portability\ncaveat, or does it avoid the problem altogether since it uses fork()\n+ exec() + waitpid()? (if this is true then it implies that this\nchange is fine).\n\nI have not read the rest of this series yet, so apologies if these\nquestions were answered elsewhere.\n\nIn general, I am a little nervous about having difftool copy worktree\ncontent somewhere temporary only to copy it back in later. Is there\nsome way to make the diff machinery reuse the worktree? I was under\nthe impression that we could do some GIT_INDEX tricks to do it, though\nI will admit that I did not read that suggestion in depth, nor did I\ngrasp whether this was the problem it was meant to address.\n\nThoughts?\n\n\n>\n> Signed-off-by: Tim Henigan \n> ---\n>  git-difftool.perl |   10 +---------\n>  1 file changed, 1 insertion(+), 9 deletions(-)\n>\n> diff --git a/git-difftool.perl b/git-difftool.perl\n> index 9495f14..8498089 100755\n> --- a/git-difftool.perl\n> +++ b/git-difftool.perl\n> @@ -72,12 +72,4 @@ elsif (defined($no_prompt)) {\n>\n>  $ENV{GIT_PAGER} = '';\n>  $ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';\n> -my @command = ('git', 'diff', @ARGV);\n> -\n> -# ActiveState Perl for Win32 does not implement POSIX semantics of\n> -# exec* system call. It just spawns the given executable and finishes\n> -# the starting program, exiting with code 0.\n> -# system will at least catch the errors returned by git diff,\n> -# allowing the caller of git difftool better handling of failures.\n> -my $rc = system(@command);\n> -exit($rc | ($rc >> 8));\n> +git_cmd_try { Git::command_noisy(('diff', @ARGV)) } 'exit code %d';\n> --\n> 1.7.9.1.290.gbd444\n>\n\n\n\n-- \nDavid\n"},{"id":"187144","messageId":"CALxABCYiOpQavW3qz+Xx-qjadaF3sAQ3DHAAwzRRBDXm6MAnOw@mail.gmail.com","threadId":"29968","inReplyTo":"CAJDDKr4+0iWoZhxo6kMVa0YUtDzmrH=XTZnDqQdbnM6TJ41UDg@mail.gmail.com","subject":"Re: [PATCH 6/9] difftool: replace system call with Git::command_noisy","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2012-03-17T10:50:57Z","receivedAt":"2012-03-17T10:50:57Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Resend for vger archives. Damn that Android GMail client.\n\nOn Sat, Mar 17, 2012 at 03:48, David Aguilar wrote:\n> On Fri, Mar 16, 2012 at 6:59 PM, Tim Henigan wrote:\n>> The Git.pm module includes functions intended to standardize working\n>> with Git repositories in Perl scripts. This commit teaches difftool\n>> to use Git::command_noisy rather than a system call to run the diff\n>> command.\n>\n> Git::command_noisy() calls _cmd_exec() which calls _execv_git_cmd()\n> which does a fork() + exec('git', @_) + waitpid();\n>\n> We were avoiding exec() for portability reasons, as Alex explained in\n> 677fbff88f368ed6ac52438ddbb530166ec1d5d1:\n>\n> # ActiveState Perl for Win32 does not implement POSIX semantics of\n> # exec* system call. It just spawns the given executable and finishes\n> # the starting program, exiting with code 0.\n> # system will at least catch the errors returned by git diff,\n> # allowing the caller of git difftool better handling of failures.\n>\n> Is this no longer a concern?  Does Git.pm need a similar portability\n> caveat, or  does it avoid the problem altogether since it uses fork()\n> + exec() + waitpid()?  (if this is true then it implies that this\n> change is fine).\n\nIt _might_ work. Cygwin kind of has fork(2), it even works (kind of:\nit is a *very* expensive thing to do). There are also other ifs and\nwhens, but it is worth a test. It's a nice clean up to have.\n"},{"id":"187157","messageId":"CAFouethChs_2ZhYDjOqRTSoDMZ60DeMkS1A=Ke4G5G_pKPKrYA@mail.gmail.com","threadId":"29968","inReplyTo":"CALxABCYiOpQavW3qz+Xx-qjadaF3sAQ3DHAAwzRRBDXm6MAnOw@mail.gmail.com","subject":"Re: [PATCH 6/9] difftool: replace system call with Git::command_noisy","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-03-17T14:48:42Z","receivedAt":"2012-03-17T14:48:42Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"On Sat, Mar 17, 2012 at 6:50 AM, Alex Riesen wrote:\n> Resend for vger archives. Damn that Android GMail client.\n>\n> On Sat, Mar 17, 2012 at 03:48, David Aguilar wrote:\n>> On Fri, Mar 16, 2012 at 6:59 PM, Tim Henigan wrote:\n>>> The Git.pm module includes functions intended to standardize working\n>>> with Git repositories in Perl scripts. This commit teaches difftool\n>>> to use Git::command_noisy rather than a system call to run the diff\n>>> command.\n>>\n>> Git::command_noisy() calls _cmd_exec() which calls _execv_git_cmd()\n>> which does a fork() + exec('git', @_) + waitpid();\n>>\n>> We were avoiding exec() for portability reasons, as Alex explained in\n>> 677fbff88f368ed6ac52438ddbb530166ec1d5d1:\n>>\n>> # ActiveState Perl for Win32 does not implement POSIX semantics of\n>> # exec* system call. It just spawns the given executable and finishes\n>> # the starting program, exiting with code 0.\n>> # system will at least catch the errors returned by git diff,\n>> # allowing the caller of git difftool better handling of failures.\n>>\n>> Is this no longer a concern?  Does Git.pm need a similar portability\n>> caveat, or  does it avoid the problem altogether since it uses fork()\n>> + exec() + waitpid()?  (if this is true then it implies that this\n>> change is fine).\n\nI need to spend more time testing this. On Windows, I have tested\nwith msysgit but not cygwin. Was ActiveState Perl used with cygwin\ngit?\n\n\n> It _might_ work. Cygwin kind of has fork(2), it even works (kind of:\n> it is a *very* expensive thing to do). There are also other ifs and\n> whens, but it is worth a test. It's a nice clean up to have.\n\nEven it fork(2) is expensive, in this case it seems reasonable. Given\nthe time needed to spawn the diff tool, the fork(2) time seems\nnegligible.\n"},{"id":"187174","messageId":"CALxABCaGOhsTdRRtbVDbS37FHZ32yf4URGw34P-nq-RFm5sSYA@mail.gmail.com","threadId":"29968","inReplyTo":"CAFouethChs_2ZhYDjOqRTSoDMZ60DeMkS1A=Ke4G5G_pKPKrYA@mail.gmail.com","subject":"Re: [PATCH 6/9] difftool: replace system call with Git::command_noisy","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2012-03-17T19:54:14Z","receivedAt":"2012-03-17T19:54:14Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On Sat, Mar 17, 2012 at 15:48, Tim Henigan wrote:\n> On Sat, Mar 17, 2012 at 6:50 AM, Alex Riesen wrote:\n>> On Sat, Mar 17, 2012 at 03:48, David Aguilar wrote:\n>>> Is this no longer a concern?  Does Git.pm need a similar portability\n>>> caveat, or  does it avoid the problem altogether since it uses fork()\n>>> + exec() + waitpid()?  (if this is true then it implies that this\n>>> change is fine).\n>\n> I need to spend more time testing this.  On Windows, I have tested\n> with msysgit but not cygwin.  Was ActiveState Perl used with cygwin\n> git?\n\nYes, it is even stated in the commentary.\n\nAs far as I know, there is only one installation where a cygwin-compiled\nGit is used with the ActiveState Perl (mine. I believe we would have\nheard if there were others - it is an extremely annoying combination).\n\n>> It _might_ work. Cygwin kind of has fork(2), it even works (kind of:\n>> it is a *very* expensive thing to do). There are also other ifs and\n>> whens, but it is worth a test. It's a nice clean up to have.\n>\n> Even it fork(2) is expensive, in this case it seems reasonable. Given\n> the time needed to spawn the diff tool, the fork(2) time seems\n> negligible.\n\nNot Cygwin's fork. They really do a deep copy of parent process.\n\nBut actually, I misunderstood. The Perl used was of ActiveState origin.\nSo the code in question is not affected by Cygwin at all.\nI have no idea how usable fork(2) of ActiveState Perl is.\n\nEven if it is bad, I won't be really affected, I very seldom use the\ndifftool, and I believe I never used it on that particular system.\n\nI try test the patch in the next days. I'll tell if something is\ncompletely broken.\n"},{"id":"187180","messageId":"CAFouetgJsHD9UoVPE6V16vAsJ4Q1neHzH7jrwGma6e2ZHELgFA@mail.gmail.com","threadId":"29968","inReplyTo":"CAJDDKr4+0iWoZhxo6kMVa0YUtDzmrH=XTZnDqQdbnM6TJ41UDg@mail.gmail.com","subject":"Re: [PATCH 6/9] difftool: replace system call with Git::command_noisy","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-03-18T01:21:47Z","receivedAt":"2012-03-18T01:21:47Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"On Fri, Mar 16, 2012 at 10:48 PM, David Aguilar wrote:\n> On Fri, Mar 16, 2012 at 6:59 PM, Tim Henigan wrote:\n>\n> In general, I am a little nervous about having difftool copy worktree\n> content somewhere temporary only to copy it back in later.  Is there\n> some way to make the diff machinery reuse the worktree?  I was under\n> the impression that we could do some GIT_INDEX tricks to do it, though\n> I will admit that I did not read that suggestion in depth, nor did I\n> grasp whether this was the problem it was meant to address.\n\nI have not been able to find any other way to do it. The GIT_INDEX\ntrick allows the tmp directories to be built using 'git update-index'\nand 'git checkout-index', but they offer no help for this problem.\n\nIf we use the working tree directory as one of the diff targets, then\nall the files in the working directory would be included in the\ndiff...unless there was some way to remove the files that aren't part\nof the diff from the working tree. However at that point, I don't\nthink the solution would be any better (i.e. deleting files from the\nworking tree and then checking them back out is no better than copying\nfiles to the tmp dir and back again).\n\nThe only other option I can think of is to build a complete copy of\nthe repo in the tmp directory for comparison against the working tree.\n However, this could obviously lead to resource/performance problems\non large repos.\n\nI am open to suggestions, but I have not found any better solution.\n"}]}