{"thread":{"id":"12575","subject":"[RFC/PATCH] gitweb: Use list form of 'open \"-|\"' pipeline","startedAt":"2008-03-08T16:57:20Z","lastAt":"2008-03-12T02:09:03Z","messageCount":7,"participants":["Jakub Narebski","Charles Bailey","Frank Lichtenheld","Jay Soffian"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"71422","messageId":"20080308165245.15343.62914.stgit@localhost.localdomain","threadId":"12575","inReplyTo":null,"subject":"[RFC/PATCH] gitweb: Use list form of 'open \"-|\"' pipeline","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-03-08T16:57:20Z","receivedAt":"2008-03-08T16:57:20Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Add output_pipeline subroutine, which allows to use list form of\npipeline; instead of\n  open my $fh, \"-|\", \"cmd_1 option | cmd_2 argument\"\nwe can now write\n  my $fh = output_pipeline(['cmd_1', 'option'], ['cmd_2', 'argument']);\nwhich allows to avoid troubles with shell quoting, and avoid spawning\nshell.  Code is based on snippet http://www.perlmonks.org/?node_id=246397\nsimplified a bit.\n\nIt is then used in git_snapshot subroutine, where we sometimes open\npipeline from git-archive to compressor.\n\nWhile at it, ensure that snapshot saved as <basename>.<suffix>\nuncompresses to <basename>/\n\nNOTE: this commit prepares way for adding syntax highlighting support\nusing external filter (external tool), like GNU Source-highlight or\nAndre Simon's Highlight.\n\nSigned-off-by: Jakub Narebski <jnareb@gmail.com>\n---\nThis patch is an RFC, and doesn't really meant to be applied.\nI'd like opinion on this code from resident Perl experts.  It\nshould work, though; it was rudimentarly tested.\n\n gitweb/gitweb.perl |   52 ++++++++++++++++++++++++++++++++++++++++------------\n 1 files changed, 40 insertions(+), 12 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex a5df2fe..ba97a7b 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -1455,6 +1455,35 @@ sub git_cmd_str {\n \treturn join(' ', git_cmd());\n }\n \n+# my $fh = output_pipeline(['cmd_1', 'option'], ['cmd_2', 'argument']);\n+# is equivalent to (is the \"list form\" of) the following\n+# open my $fh, \"-|\", \"cmd_1 option | cmd_2 argument\"\n+#\n+# Based on http://www.perlmonks.org/?node_id=246397\n+#\n+sub output_pipeline {\n+\tmy @commands_list = @_;\n+\texit unless @commands_list;\n+\n+\tmy $pid = open(my $fh, \"-|\");\n+\t#die \"Couldn't fork: $!\" unless defined $pid;\n+\n+\tif ($pid) { # parent\n+\t\treturn $fh;\n+\t}\n+\n+\t# child\n+ COMMAND:\n+\twhile (my $command = pop @commands_list) {\n+\t\tmy $pid = @commands_list ? open(STDIN, \"-|\") : -1;\n+\t\t#die \"Couldn't fork: $!\" unless defined $pid;\n+\n+\t\tnext COMMAND unless ($pid); # parent\n+\t\texec @$command;             # child\n+\t\t#die \"Couldn't exec \\\"@$command\\\": $!\";\n+\t}\n+}\n+\n # get HEAD ref of given project as hash\n sub git_get_head_hash {\n \tmy $project = shift;\n@@ -4545,27 +4574,26 @@ sub git_snapshot {\n \t\t$hash = git_get_head_hash($project);\n \t}\n \n-\tmy $git_command = git_cmd_str();\n \tmy $name = $project;\n-\t$name =~ s,([^/])/*\\.git$,$1,;\n+\t$name =~ s,([^/])/*\\.git$,$1,; # strip '.git' or '/.git'\n \t$name = basename($name);\n-\tmy $filename = to_utf8($name);\n-\t$name =~ s/\\047/\\047\\\\\\047\\047/g;\n-\tmy $cmd;\n-\t$filename .= \"-$hash$known_snapshot_formats{$format}{'suffix'}\";\n-\t$cmd = \"$git_command archive \" .\n-\t\t\"--format=$known_snapshot_formats{$format}{'format'} \" .\n-\t\t\"--prefix=\\'$name\\'/ $hash\";\n+\t$name = to_utf8($name);  # or only for filename, not prefix?\n+\t$name .= \"-$hash\";\n+\n+\tmy @cmds = ([git_cmd(), \"archive\",\n+\t\t\"--format=$known_snapshot_formats{$format}{'format'}\",\n+\t\t\"--prefix=$name/\", $hash]);\n \tif (exists $known_snapshot_formats{$format}{'compressor'}) {\n-\t\t$cmd .= ' | ' . join ' ', @{$known_snapshot_formats{$format}{'compressor'}};\n+\t\tpush @cmds, $known_snapshot_formats{$format}{'compressor'};\n \t}\n \n \tprint $cgi->header(\n \t\t-type => $known_snapshot_formats{$format}{'type'},\n-\t\t-content_disposition => 'inline; filename=\"' . \"$filename\" . '\"',\n+\t\t-content_disposition => 'inline; filename=\"' .\n+\t\t\t\"$filename$known_snapshot_formats{$format}{'suffix'}\" . '\"',\n \t\t-status => '200 OK');\n \n-\topen my $fd, \"-|\", $cmd\n+\tmy $fd = output_pipeline(@cmds)\n \t\tor die_error(undef, \"Execute git-archive failed\");\n \tbinmode STDOUT, ':raw';\n \tprint <$fd>;\n\n"},{"id":"71424","messageId":"20080308175106.GA13855@hashpling.org","threadId":"12575","inReplyTo":"20080308165245.15343.62914.stgit@localhost.localdomain","subject":"Re: [RFC/PATCH] gitweb: Use list form of 'open \"-|\"' pipeline","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2008-03-08T17:51:06Z","receivedAt":"2008-03-08T17:51:06Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On Sat, Mar 08, 2008 at 05:57:20PM +0100, Jakub Narebski wrote:\n> Add output_pipeline subroutine, which allows to use list form of\n> pipeline; instead of\n>   open my $fh, \"-|\", \"cmd_1 option | cmd_2 argument\"\n> we can now write\n>   my $fh = output_pipeline(['cmd_1', 'option'], ['cmd_2', 'argument']);\n> which allows to avoid troubles with shell quoting, and avoid spawning\n> shell.  Code is based on snippet http://www.perlmonks.org/?node_id=246397\n> simplified a bit.\n> \n\nI'm not a perl open expert, and I don't know if gitweb has other\nissues on windows anyway, but the list time I experimented with it\nneither the open( FHAND, \"-|\" ) trick, not the list form of open in\ncombination with the \"-|\" mode worked at all on windows.\n\nCharles.\n"},{"id":"71427","messageId":"200803081929.46306.jnareb@gmail.com","threadId":"12575","inReplyTo":"20080308175106.GA13855@hashpling.org","subject":"Re: [RFC/PATCH] gitweb: Use list form of 'open \"-|\"' pipeline","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-03-08T18:29:45Z","receivedAt":"2008-03-08T18:29:45Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sat, 8 Mar 2008, Charles Bailey wrote:\n> On Sat, Mar 08, 2008 at 05:57:20PM +0100, Jakub Narebski wrote:\n\n>> Add output_pipeline subroutine, which allows to use list form of\n>> pipeline; instead of\n>>\n>>   open my $fh, \"-|\", \"cmd_1 option | cmd_2 argument\"\n>>\n>> we can now write\n>>\n>>   my $fh = output_pipeline(['cmd_1', 'option'], ['cmd_2', 'argument']);\n>>\n>> which allows to avoid troubles with shell quoting, and avoid spawning\n>> shell.  Code is based on snippet http://www.perlmonks.org/?node_id=246397\n>> simplified a bit.\n> \n> I'm not a Perl open expert, and I don't know if gitweb has other\n> issues on windows anyway, but the list time I experimented with it\n> neither the open( FHAND, \"-|\" ) trick, not the list form of open in\n> combination with the \"-|\" mode worked at all on windows.\n\nFirst, gitweb is git web interface; I don't know if anybody tried for\nexample to run gitweb on Windows, be it from Apache or from IIS.\n\nSecond, it probably depends on the flavour (distribution) of Perl\nused. ActiveState Perl requires some horrible workarounds[*1*], I don't\nknow what about MSys / MinGW Perl, Cygwin Perl, or Vanilla / Strawberry\nPerl.\n\n\n[*1*] Git.pm, Perl interface to the Git, includes workaround for\nActiveState Perl sheningans; gitweb currently doesn't use Git.pm\n(Git.pm would need some extensions), but perhaps it should. On the\nother hand it would make installation slightly more complicated.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"71669","messageId":"20080311090100.GN10103@mail-vs.djpig.de","threadId":"12575","inReplyTo":"20080308165245.15343.62914.stgit@localhost.localdomain","subject":"Re: [RFC/PATCH] gitweb: Use list form of 'open \"-|\"' pipeline","fromName":"Frank Lichtenheld","fromEmail":"frank@lichtenheld.de","sentAt":"2008-03-11T09:01:00Z","receivedAt":"2008-03-11T09:01:00Z","isPatch":true,"sender":{"key":"frank@lichtenheld.de","avatar":"https://gravatar.com/avatar/b9f1d4b120e138f157c9e480d0818197c474628923786adb98f30017cdb99c3c?d=mp&s=160"},"body":"On Sat, Mar 08, 2008 at 05:57:20PM +0100, Jakub Narebski wrote:\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index a5df2fe..ba97a7b 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -1455,6 +1455,35 @@ sub git_cmd_str {\n>  \treturn join(' ', git_cmd());\n>  }\n>  \n> +# my $fh = output_pipeline(['cmd_1', 'option'], ['cmd_2', 'argument']);\n> +# is equivalent to (is the \"list form\" of) the following\n> +# open my $fh, \"-|\", \"cmd_1 option | cmd_2 argument\"\n> +#\n> +# Based on http://www.perlmonks.org/?node_id=246397\n\nIt might be worthwile to look into how e.g. IPC::Run does this.\n\n> +sub output_pipeline {\n> +\tmy @commands_list = @_;\n> +\texit unless @commands_list;\n> +\n> +\tmy $pid = open(my $fh, \"-|\");\n> +\t#die \"Couldn't fork: $!\" unless defined $pid;\n\nWhy are all the die's commented out?\n\n> +\tif ($pid) { # parent\n> +\t\treturn $fh;\n> +\t}\n> +\n> +\t# child\n> + COMMAND:\n> +\twhile (my $command = pop @commands_list) {\n> +\t\tmy $pid = @commands_list ? open(STDIN, \"-|\") : -1;\n> +\t\t#die \"Couldn't fork: $!\" unless defined $pid;\n> +\n> +\t\tnext COMMAND unless ($pid); # parent\n> +\t\texec @$command;             # child\n\nThe \"parent\" and \"child\" comments here are wrong, which was really\nreally confusing...\n\nThis should probably be \"new child\" instead of \"parent\" and\n\"old child\" instead of \"child\".\n\nThw whole concept of processing the array backwards might be shorter,\nI personally find it somewhat confusing though.\n\nWhat happens to all these child processes anyway if one of them fails to\nexec?\n\n> +\t\t#die \"Couldn't exec \\\"@$command\\\": $!\";\n> +\t}\n> +}\n> +\n>  # get HEAD ref of given project as hash\n>  sub git_get_head_hash {\n>  \tmy $project = shift;\n> @@ -4545,27 +4574,26 @@ sub git_snapshot {\n>  \t\t$hash = git_get_head_hash($project);\n>  \t}\n>  \n> -\tmy $git_command = git_cmd_str();\n>  \tmy $name = $project;\n> -\t$name =~ s,([^/])/*\\.git$,$1,;\n> +\t$name =~ s,([^/])/*\\.git$,$1,; # strip '.git' or '/.git'\n>  \t$name = basename($name);\n> -\tmy $filename = to_utf8($name);\n> -\t$name =~ s/\\047/\\047\\\\\\047\\047/g;\n> -\tmy $cmd;\n> -\t$filename .= \"-$hash$known_snapshot_formats{$format}{'suffix'}\";\n> -\t$cmd = \"$git_command archive \" .\n> -\t\t\"--format=$known_snapshot_formats{$format}{'format'} \" .\n> -\t\t\"--prefix=\\'$name\\'/ $hash\";\n> +\t$name = to_utf8($name);  # or only for filename, not prefix?\n> +\t$name .= \"-$hash\";\n> +\n> +\tmy @cmds = ([git_cmd(), \"archive\",\n> +\t\t\"--format=$known_snapshot_formats{$format}{'format'}\",\n> +\t\t\"--prefix=$name/\", $hash]);\n>  \tif (exists $known_snapshot_formats{$format}{'compressor'}) {\n> -\t\t$cmd .= ' | ' . join ' ', @{$known_snapshot_formats{$format}{'compressor'}};\n> +\t\tpush @cmds, $known_snapshot_formats{$format}{'compressor'};\n>  \t}\n>  \n>  \tprint $cgi->header(\n>  \t\t-type => $known_snapshot_formats{$format}{'type'},\n> -\t\t-content_disposition => 'inline; filename=\"' . \"$filename\" . '\"',\n> +\t\t-content_disposition => 'inline; filename=\"' .\n> +\t\t\t\"$filename$known_snapshot_formats{$format}{'suffix'}\" . '\"',\n\nHuh, that compiles? Where is $filename defined now at all?\n\n>  \t\t-status => '200 OK');\n>  \n> -\topen my $fd, \"-|\", $cmd\n> +\tmy $fd = output_pipeline(@cmds)\n>  \t\tor die_error(undef, \"Execute git-archive failed\");\n>  \tbinmode STDOUT, ':raw';\n>  \tprint <$fd>;\n\nGruesse,\n-- \nFrank Lichtenheld <frank@lichtenheld.de>\nwww: http://www.djpig.de/\n"},{"id":"71739","messageId":"200803111830.58392.jnareb@gmail.com","threadId":"12575","inReplyTo":"20080311090100.GN10103@mail-vs.djpig.de","subject":"Re: [RFC/PATCH] gitweb: Use list form of 'open \"-|\"' pipeline","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-03-11T17:30:57Z","receivedAt":"2008-03-11T17:30:57Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Thanks a lot for reviewing this patch!\n\nOn Tue, 11 March 2008, Frank Lichtenheld wrote:\n> On Sat, Mar 08, 2008 at 05:57:20PM +0100, Jakub Narebski wrote:\n>>\n>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n>> index a5df2fe..ba97a7b 100755\n>> --- a/gitweb/gitweb.perl\n>> +++ b/gitweb/gitweb.perl\n>> @@ -1455,6 +1455,35 @@ sub git_cmd_str {\n>>  \treturn join(' ', git_cmd());\n>>  }\n>>  \n>> +# my $fh = output_pipeline(['cmd_1', 'option'], ['cmd_2', 'argument']);\n>> +# is equivalent to (is the \"list form\" of) the following\n>> +# open my $fh, \"-|\", \"cmd_1 option | cmd_2 argument\"\n>> +#\n>> +# Based on http://www.perlmonks.org/?node_id=246397\n\nNote that this patch is a bit \"cargo cult\" (copy'n'paste) programming...\n \n> It might be worthwile to look into how e.g. IPC::Run does this.\n\nThanks for the pointer. I look at it.\n\nIPC::Run isn't installed by default with Perl, like Encode, Fcntl,\nFile::Find and File::Basename, or reasonable to be required to be\ninstalled to run server side app like CGI, so we cannot use it\ndirectly, as we cannot require for gitweb user to have it installed.\n\nAdditionally while in some parts IPC::Run API is nice and well\nthought (composing pipeline), some of it would make it hard to use\nin gitweb. For example I could not find how to direct pipeline\noutput to filehandle instead of to scalar or opened file. While\nfor currently the only pipeline in gitweb, namely generating\n(externally) compressed snapshot IPC::Run API would be enough,\nfor syntax highlighting with its line-by-line pipeline output\nparsing it would be, I think, a bit cumbersome.\n\nIt would be nice if parts of IPC::Run made it into Git.pm...\nthen we could just use it in gitweb.\n\n>> +sub output_pipeline {\n>> +\tmy @commands_list = @_;\n>> +\texit unless @commands_list;\n>> +\n>> +\tmy $pid = open(my $fh, \"-|\");\n>> +\t#die \"Couldn't fork: $!\" unless defined $pid;\n> \n> Why are all the die's commented out?\n\nThe goal is to have gitweb deal with errors gracefully. It should\ngenerate some kind of '503 Server Error' page, instead of dieing\nwithout output, or what would be even worse, in the middle of output.\n\nI haven't examined how it should be writen for this RFC patch, so\nI have commented out 'die' just in case. In the final version (if it\nwill be decided to go this route) it should be cleaned out.\n\n>> +\tif ($pid) { # parent\n>> +\t\treturn $fh;\n>> +\t}\n>> +\n>> +\t# child\n>> + COMMAND:\n>> +\twhile (my $command = pop @commands_list) {\n>> +\t\tmy $pid = @commands_list ? open(STDIN, \"-|\") : -1;\n>> +\t\t#die \"Couldn't fork: $!\" unless defined $pid;\n>> +\n>> +\t\tnext COMMAND unless ($pid); # parent\n>> +\t\texec @$command;             # child\n> \n> The \"parent\" and \"child\" comments here are wrong, which was really\n> really confusing...\n\nI'm sorry. My mistake.\n\n> This should probably be \"new child\" instead of \"parent\" and\n> \"old child\" instead of \"child\".\n\nI think those comments could be simply removed, as they are, I guess,\nequivalent to\n\n   count++;  /* increment counter */\n\nin C/C++.\n\n> Thw whole concept of processing the array backwards might be shorter,\n> I personally find it somewhat confusing though.\n\nI'm not sure if it is not the only possible way, as the (first) parent,\nI think, has to return filehandle. But I might be mistaken.\n\n> What happens to all these child processes anyway if one of them fails to\n> exec?\n\nOriginal snippet returned in addition to filehandle also list of pids.\nPerhaps I have oversimplified this snipped... or it was to simple to\nbegin with.\n\n>> +\t\t#die \"Couldn't exec \\\"@$command\\\": $!\";\n>> +\t}\n>> +}\n>> +\n>>  # get HEAD ref of given project as hash\n>>  sub git_get_head_hash {\n>>  \tmy $project = shift;\n>> @@ -4545,27 +4574,26 @@ sub git_snapshot {\n>>  \t\t$hash = git_get_head_hash($project);\n>>  \t}\n>>  \n>> -\tmy $git_command = git_cmd_str();\n>>  \tmy $name = $project;\n>> -\t$name =~ s,([^/])/*\\.git$,$1,;\n>> +\t$name =~ s,([^/])/*\\.git$,$1,; # strip '.git' or '/.git'\n>>  \t$name = basename($name);\n>> -\tmy $filename = to_utf8($name);\n>> -\t$name =~ s/\\047/\\047\\\\\\047\\047/g;\n>> -\tmy $cmd;\n>> -\t$filename .= \"-$hash$known_snapshot_formats{$format}{'suffix'}\";\n>> -\t$cmd = \"$git_command archive \" .\n>> -\t\t\"--format=$known_snapshot_formats{$format}{'format'} \" .\n>> -\t\t\"--prefix=\\'$name\\'/ $hash\";\n>> +\t$name = to_utf8($name);  # or only for filename, not prefix?\n>> +\t$name .= \"-$hash\";\n>> +\n>> +\tmy @cmds = ([git_cmd(), \"archive\",\n>> +\t\t\"--format=$known_snapshot_formats{$format}{'format'}\",\n>> +\t\t\"--prefix=$name/\", $hash]);\n>>  \tif (exists $known_snapshot_formats{$format}{'compressor'}) {\n>> -\t\t$cmd .= ' | ' . join ' ', @{$known_snapshot_formats{$format}{'compressor'}};\n>> +\t\tpush @cmds, $known_snapshot_formats{$format}{'compressor'};\n>>  \t}\n>>  \n>>  \tprint $cgi->header(\n>>  \t\t-type => $known_snapshot_formats{$format}{'type'},\n>> -\t\t-content_disposition => 'inline; filename=\"' . \"$filename\" . '\"',\n>> +\t\t-content_disposition => 'inline; filename=\"' .\n>> +\t\t\t\"$filename$known_snapshot_formats{$format}{'suffix'}\" . '\"',\n> \n> Huh, that compiles? Where is $filename defined now at all?\n\nI'm very sorry. This was my quick improving code just before sending\nthis patch... forgetting to test after \"obvously correct\" (not!) changes.\n\nBy the way, this change to have <filename>.<suffix> snapshot extract to\n<filename>/ directory is independent on the \"list form of pipeline\"\nchanges and should probably be sent as separate patch.\n\n\nOnce again, thanks for reviewing and commenting on this RFC patch.\nI'm just not a Perl hacker...\n-- \nJakub Narebski\nPoland\n"},{"id":"71749","messageId":"20080311185916.GO10103@mail-vs.djpig.de","threadId":"12575","inReplyTo":"200803111830.58392.jnareb@gmail.com","subject":"Re: [RFC/PATCH] gitweb: Use list form of 'open \"-|\"' pipeline","fromName":"Frank Lichtenheld","fromEmail":"frank@lichtenheld.de","sentAt":"2008-03-11T18:59:16Z","receivedAt":"2008-03-11T18:59:16Z","isPatch":true,"sender":{"key":"frank@lichtenheld.de","avatar":"https://gravatar.com/avatar/b9f1d4b120e138f157c9e480d0818197c474628923786adb98f30017cdb99c3c?d=mp&s=160"},"body":"On Tue, Mar 11, 2008 at 06:30:57PM +0100, Jakub Narebski wrote:\n> On Tue, 11 March 2008, Frank Lichtenheld wrote:\n> > On Sat, Mar 08, 2008 at 05:57:20PM +0100, Jakub Narebski wrote:\n> >>\n> >> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> >> index a5df2fe..ba97a7b 100755\n> >> --- a/gitweb/gitweb.perl\n> >> +++ b/gitweb/gitweb.perl\n> >> @@ -1455,6 +1455,35 @@ sub git_cmd_str {\n> >>  \treturn join(' ', git_cmd());\n> >>  }\n> >>  \n> >> +# my $fh = output_pipeline(['cmd_1', 'option'], ['cmd_2', 'argument']);\n> >> +# is equivalent to (is the \"list form\" of) the following\n> >> +# open my $fh, \"-|\", \"cmd_1 option | cmd_2 argument\"\n> >> +#\n> >> +# Based on http://www.perlmonks.org/?node_id=246397\n> \n> Note that this patch is a bit \"cargo cult\" (copy'n'paste) programming...\n>  \n> > It might be worthwile to look into how e.g. IPC::Run does this.\n> \n> Thanks for the pointer. I look at it.\n\nAfter taking a look at the IPC::Run code myself I'm not really sure it\nis really \"worthwile\", as I put it, to try to understand that.\n\nCreating a less flexible solution that is readable might be better.\n\n> >> +sub output_pipeline {\n> >> +\tmy @commands_list = @_;\n> >> +\texit unless @commands_list;\n> >> +\n> >> +\tmy $pid = open(my $fh, \"-|\");\n> >> +\t#die \"Couldn't fork: $!\" unless defined $pid;\n> > \n> > Why are all the die's commented out?\n> \n> The goal is to have gitweb deal with errors gracefully. It should\n> generate some kind of '503 Server Error' page, instead of dieing\n> without output, or what would be even worse, in the middle of output.\n> \n> I haven't examined how it should be writen for this RFC patch, so\n> I have commented out 'die' just in case. In the final version (if it\n> will be decided to go this route) it should be cleaned out.\n\nOk, I guessed as much, but wanted to make sure ;)\n\n> > Thw whole concept of processing the array backwards might be shorter,\n> > I personally find it somewhat confusing though.\n> \n> I'm not sure if it is not the only possible way, as the (first) parent,\n> I think, has to return filehandle. But I might be mistaken.\n> \n> > What happens to all these child processes anyway if one of them fails to\n> > exec?\n> \n> Original snippet returned in addition to filehandle also list of pids.\n> Perhaps I have oversimplified this snipped... or it was to simple to\n> begin with.\n\nI'm not really convinced yet that dealing with a shell is much worse\nthan dealing with IPC ;)\n\nJokes aside, my idea for implementing something like this would be to\nuse explicit pipe()'s and fork()'s instead of the open() magic. With\nbetter control over the filehandles and pids you might be able to build\na more robust solution.\n\nGruesse,\n-- \nFrank Lichtenheld <frank@lichtenheld.de>\nwww: http://www.djpig.de/\n"},{"id":"71762","messageId":"76718490803111909p9684463j355c1f587bf31b13@mail.gmail.com","threadId":"12575","inReplyTo":"20080311185916.GO10103@mail-vs.djpig.de","subject":"Re: [RFC/PATCH] gitweb: Use list form of 'open \"-|\"' pipeline","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2008-03-12T02:09:03Z","receivedAt":"2008-03-12T02:09:03Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Tue, Mar 11, 2008 at 2:59 PM, Frank Lichtenheld <frank@lichtenheld.de> wrote:\n>  After taking a look at the IPC::Run code myself I'm not really sure it\n>  is really \"worthwile\", as I put it, to try to understand that.\n>\n>  Creating a less flexible solution that is readable might be better.\n\n\"man perlipc\" - search for \"Safe Pipe Opens\".\n\nj.\n"}]}