{"thread":{"id":"8892","subject":"[PATCH] git-cvsserver: detect/diagnose write failure, etc.","startedAt":"2007-07-06T12:18:42Z","lastAt":"2007-07-14T18:48:42Z","messageCount":3,"participants":["Jim Meyering","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"46616","messageId":"87vecx4tel.fsf@rho.meyering.net","threadId":"8892","inReplyTo":null,"subject":"[PATCH] git-cvsserver: detect/diagnose write failure, etc.","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2007-07-06T12:18:42Z","receivedAt":"2007-07-06T12:18:42Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Currently I maintain a CVS repository that mirrors\nthe primary git repository for the GNU coreutils.\nIt is kept in sync via a relatively fragile \"update\" (push) hook\nthat runs git-cvsexportcommit every time I push to the git repository.\n\nSince it'd be nice to keep cvs support (so as not to *require*\neveryone to switch to git right away), without the actual CVS\nrepository, I'm considering using git-cvsserver.\n\nHowever, before I switch, I'd like to have a little more\nconfidence that it is reliable. I've included a patch below\nthat makes it more likely to report if/when something goes wrong,\nusually just write errors.\n\nBeware: in some contexts (when running as server), one must\nnot \"die\", but rather return an \"error\" indicator to the client.\nI did that in the second hunk. However, I haven't carefully\naudited the others, and so, some of the \"die\" calls I added may\nend up killing the server, when a gentler failure is required.\n\nI've just looked, and can confirm that my change to req_Modified\n(first hunk) is wrong. It should not die. Rather, it should probably\ndo something like the \"print \"E ... in hunk#2. Ideally, someone\nwould write a test to exercise code like this, to make sure it works.\n\nJim\n\ndiff --git a/git-cvsserver.perl b/git-cvsserver.perl\nindex 5cbf27e..8d8d6f5 100755\n--- a/git-cvsserver.perl\n+++ b/git-cvsserver.perl\n@@ -644,7 +644,7 @@ sub req_Modified\n $bytesleft -= $blocksize;\n }\n \n- close $fh;\n+ close $fh or die \"Failed to write temporary, $filename: $!\";\n \n # Ensure we have something sensible for the file mode\n if ( $mode =~ /u=(\\w+)/ )\n@@ -901,8 +901,13 @@ sub req_update\n # projects (heads in this case) to checkout.\n #\n if ($state->{module} eq '') {\n+\tmy $heads_dir = $state->{CVSROOT} . '/refs/heads';\n+\tif (!opendir HEADS, $heads_dir) {\n+\t print \"E [server aborted]: Failed to open directory, $heads_dir: $!\\n\"\n+\t . \"error\\n\";\n+\t return 0;\n+\t}\n print \"E cvs update: Updating .\\n\";\n-\topendir HEADS, $state->{CVSROOT} . '/refs/heads';\n \twhile (my $head = readdir(HEADS)) {\n \t if (-f $state->{CVSROOT} . '/refs/heads/' . $head) {\n \t print \"E cvs update: New directory `$head'\\n\";\n@@ -1754,7 +1759,9 @@ sub req_annotate\n # git-jsannotate telling us about commits we are hiding\n # from the client.\n \n- open(ANNOTATEHINTS, \">$tmpdir/.annotate_hints\") or die \"Error opening > $tmpdir/.annotate_hints $!\";\n+ my $a_hints = \"$tmpdir/.annotate_hints\";\n+ open(ANNOTATEHINTS, '>', $a_hints)\n+\t or die \"Failed to open '$a_hints' for writing: $!\";\n for (my $i=0; $i < @$revisions; $i++)\n {\n print ANNOTATEHINTS $revisions->[$i][2];\n@@ -1765,11 +1772,11 @@ sub req_annotate\n }\n \n print ANNOTATEHINTS \"\\n\";\n- close ANNOTATEHINTS;\n+ close ANNOTATEHINTS or die \"Failed to write $a_hints: $!\";\n \n- my $annotatecmd = 'git-annotate';\n- open(ANNOTATE, \"-|\", $annotatecmd, '-l', '-S', \"$tmpdir/.annotate_hints\", $filename)\n-\t or die \"Error invoking $annotatecmd -l -S $tmpdir/.annotate_hints $filename : $!\";\n+ my @cmd = (qw(git-annotate -l -S), $a_hints, $filename);\n+ open(ANNOTATE, \"-|\", @cmd)\n+\t or die \"Error invoking \". join(' ',@cmd) .\": $!\";\n my $metadata = {};\n print \"E Annotations for $filename\\n\";\n print \"E ***************\\n\";\n@@ -1996,12 +2003,12 @@ sub transmitfile\n {\n open NEWFILE, \">\", $targetfile or die(\"Couldn't open '$targetfile' for writing : $!\");\n print NEWFILE $_ while ( <$fh> );\n- close NEWFILE;\n+ close NEWFILE or die(\"Failed to write '$targetfile': $!\");\n } else {\n print \"$size\\n\";\n print while ( <$fh> );\n }\n- close $fh or die (\"Couldn't close filehandle for transmitfile()\");\n+ close $fh or die (\"Couldn't close filehandle for transmitfile(): $!\");\n } else {\n die(\"Couldn't execute git-cat-file\");\n }\n@@ -2501,17 +2508,14 @@ sub update\n if ($parent eq $lastpicked) {\n next;\n }\n- open my $p, 'git-merge-base '. $lastpicked . ' '\n- . $parent . '|';\n- my @output = (<$p>);\n- close $p;\n- my $base = join('', @output);\n+ my $base = safe_pipe_capture('git-merge-base',\n+\t\t\t\t\t\t $lastpicked, $parent);\n chomp $base;\n if ($base) {\n my @merged;\n # print \"want to log between $base $parent \\n\";\n open(GITLOG, '-|', 'git-log', \"$base..$parent\")\n- or die \"Cannot call git-log: $!\";\n+\t\t\t or die \"Cannot call git-log: $!\";\n my $mergedhash;\n while () {\n chomp;\n-- \n1.5.3.rc0.11.ge2b1a-dirty\n"},{"id":"47086","messageId":"7v1wfe38xp.fsf@assigned-by-dhcp.cox.net","threadId":"8892","inReplyTo":"87vecx4tel.fsf@rho.meyering.net","subject":"Re: [PATCH] git-cvsserver: detect/diagnose write failure, etc.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-07-11T21:52:02Z","receivedAt":"2007-07-11T21:52:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jim Meyering writes:\n\n> Beware: in some contexts (when running as server), one must\n> not \"die\", but rather return an \"error\" indicator to the client.\n> I did that in the second hunk. However, I haven't carefully\n> audited the others, and so, some of the \"die\" calls I added may\n> end up killing the server, when a gentler failure is required.\n>\n> I've just looked, and can confirm that my change to req_Modified\n> (first hunk) is wrong. It should not die. Rather, it should probably\n> do something like the \"print \"E ... in hunk#2. Ideally, someone\n> would write a test to exercise code like this, to make sure it works.\n>\n> Jim\n\n> diff --git a/git-cvsserver.perl b/git-cvsserver.perl\n> index 5cbf27e..8d8d6f5 100755\n> --- a/git-cvsserver.perl\n> +++ b/git-cvsserver.perl\n> @@ -644,7 +644,7 @@ sub req_Modified\n> $bytesleft -= $blocksize;\n> }\n> \n> - close $fh;\n> + close $fh or die \"Failed to write temporary, $filename: $!\";\n\nTrue; dying is not appropriate here.\n\n> @@ -901,8 +901,13 @@ sub req_update\n> # projects (heads in this case) to checkout.\n> #\n> if ($state->{module} eq '') {\n> +\tmy $heads_dir = $state->{CVSROOT} . '/refs/heads';\n> +\tif (!opendir HEADS, $heads_dir) {\n> +\t print \"E [server aborted]: Failed to open directory, $heads_dir: $!\\n\"\n> +\t . \"error\\n\";\n> +\t return 0;\n> +\t}\n> print \"E cvs update: Updating .\\n\";\n> -\topendir HEADS, $state->{CVSROOT} . '/refs/heads';\n> \twhile (my $head = readdir(HEADS)) {\n> \t if (-f $state->{CVSROOT} . '/refs/heads/' . $head) {\n> \t print \"E cvs update: New directory `$head'\\n\";\n\nHmph. The clients get no entries in the listing either way, but\nI would say this is an improvement.\n\n> @@ -1754,7 +1759,9 @@ sub req_annotate\n> # git-jsannotate telling us about commits we are hiding\n> # from the client.\n> \n> - open(ANNOTATEHINTS, \">$tmpdir/.annotate_hints\") or die \"Error opening > $tmpdir/.annotate_hints $!\";\n> + my $a_hints = \"$tmpdir/.annotate_hints\";\n> + open(ANNOTATEHINTS, '>', $a_hints)\n> +\t or die \"Failed to open '$a_hints' for writing: $!\";\n> for (my $i=0; $i < @$revisions; $i++)\n> {\n> print ANNOTATEHINTS $revisions->[$i][2];\n\nHmph, we seem to have:\n\n $log->warn(\"Error in annotate output! LINE: $_\");\n print \"E Annotate error \\n\";\n\nwhich suggest me that throwing \"E error\" at the client and\nreturning might be better?\n\n> @@ -1765,11 +1772,11 @@ sub req_annotate\n> }\n> \n> print ANNOTATEHINTS \"\\n\";\n> - close ANNOTATEHINTS;\n> + close ANNOTATEHINTS or die \"Failed to write $a_hints: $!\";\n> \n> - my $annotatecmd = 'git-annotate';\n> - open(ANNOTATE, \"-|\", $annotatecmd, '-l', '-S', \"$tmpdir/.annotate_hints\", $filename)\n> -\t or die \"Error invoking $annotatecmd -l -S $tmpdir/.annotate_hints $filename : $!\";\n> + my @cmd = (qw(git-annotate -l -S), $a_hints, $filename);\n> + open(ANNOTATE, \"-|\", @cmd)\n> +\t or die \"Error invoking \". join(' ',@cmd) .\": $!\";\n> my $metadata = {};\n> print \"E Annotations for $filename\\n\";\n> print \"E ***************\\n\";\n\nLikewise...\n\n> @@ -1996,12 +2003,12 @@ sub transmitfile\n> {\n> open NEWFILE, \">\", $targetfile or die(\"Couldn't open '$targetfile' for writing : $!\");\n> print NEWFILE $_ while ( <$fh> );\n> - close NEWFILE;\n> + close NEWFILE or die(\"Failed to write '$targetfile': $!\");\n> } else {\n> print \"$size\\n\";\n> print while ( <$fh> );\n> }\n> - close $fh or die (\"Couldn't close filehandle for transmitfile()\");\n> + close $fh or die (\"Couldn't close filehandle for transmitfile(): $!\");\n> } else {\n> die(\"Couldn't execute git-cat-file\");\n> }\n\nOk.\n\n> @@ -2501,17 +2508,14 @@ sub update\n> if ($parent eq $lastpicked) {\n> next;\n> }\n> - open my $p, 'git-merge-base '. $lastpicked . ' '\n> - . $parent . '|';\n> - my @output = (<$p>);\n> - close $p;\n> - my $base = join('', @output);\n> + my $base = safe_pipe_capture('git-merge-base',\n> +\t\t\t\t\t\t $lastpicked, $parent);\n> chomp $base;\n> if ($base) {\n> my @merged;\n> # print \"want to log between $base $parent \\n\";\n> open(GITLOG, '-|', 'git-log', \"$base..$parent\")\n> - or die \"Cannot call git-log: $!\";\n> +\t\t\t or die \"Cannot call git-log: $!\";\n> my $mergedhash;\n> while () {\n> chomp;\n\nOk.\n"},{"id":"47366","messageId":"87abtyn7n9.fsf@rho.meyering.net","threadId":"8892","inReplyTo":"7v1wfe38xp.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] git-cvsserver: detect/diagnose write failure, etc.","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2007-07-14T18:48:42Z","receivedAt":"2007-07-14T18:48:42Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Junio C Hamano wrote:\n> Jim Meyering writes:\n>\n>> Beware: in some contexts (when running as server), one must\n>> not \"die\", but rather return an \"error\" indicator to the client.\n>> I did that in the second hunk. However, I haven't carefully\n>> audited the others, and so, some of the \"die\" calls I added may\n>> end up killing the server, when a gentler failure is required.\n>>\n>> I've just looked, and can confirm that my change to req_Modified\n>> (first hunk) is wrong. It should not die. Rather, it should probably\n>> do something like the \"print \"E ... in hunk#2. Ideally, someone\n>> would write a test to exercise code like this, to make sure it works.\n>>\n>> Jim\n>\n>> diff --git a/git-cvsserver.perl b/git-cvsserver.perl\n>> index 5cbf27e..8d8d6f5 100755\n>> --- a/git-cvsserver.perl\n>> +++ b/git-cvsserver.perl\n>> @@ -644,7 +644,7 @@ sub req_Modified\n>> $bytesleft -= $blocksize;\n>> }\n>>\n>> - close $fh;\n>> + close $fh or die \"Failed to write temporary, $filename: $!\";\n>\n> True; dying is not appropriate here.\n...\n\nHi Jun,\n\nThanks for the feedback.\nSince no one else stepped forward, I've gone ahead and addressed the\nproblems -- though I haven't tested any of these server-side diagnostics.\nI've added two new checks in req_Modified, to better diagnose an\nincomplete request. Also, I converted each of the \"die\" stmts in\nreq_annotate to print \"E...\" + return.\n\nSigned-off-by: Jim Meyering \n---\n git-cvsserver.perl | 50 +++++++++++++++++++++++++++++++++-----------------\n 1 files changed, 33 insertions(+), 17 deletions(-)\n\ndiff --git a/git-cvsserver.perl b/git-cvsserver.perl\nindex 10aba50..ae7d511 100755\n--- a/git-cvsserver.perl\n+++ b/git-cvsserver.perl\n@@ -623,8 +623,12 @@ sub req_Modified\n my ( $cmd, $data ) = @_;\n \n my $mode = ;\n+ defined $mode\n+ or (print \"E end of file reading mode for $data\\n\"), return;\n chomp $mode;\n my $size = ;\n+ defined $size\n+ or (print \"E end of file reading size of $data\\n\"), return;\n chomp $size;\n \n # Grab config information\n@@ -644,7 +648,8 @@ sub req_Modified\n $bytesleft -= $blocksize;\n }\n \n- close $fh;\n+ close $fh\n+ or (print \"E failed to write temporary, $filename: $!\\n\"), return;\n \n # Ensure we have something sensible for the file mode\n if ( $mode =~ /u=(\\w+)/ )\n@@ -901,8 +906,13 @@ sub req_update\n # projects (heads in this case) to checkout.\n #\n if ($state->{module} eq '') {\n+\tmy $heads_dir = $state->{CVSROOT} . '/refs/heads';\n+\tif (!opendir HEADS, $heads_dir) {\n+\t print \"E [server aborted]: Failed to open directory, \"\n+\t . \"$heads_dir: $!\\nerror\\n\";\n+\t return 0;\n+\t}\n print \"E cvs update: Updating .\\n\";\n-\topendir HEADS, $state->{CVSROOT} . '/refs/heads';\n \twhile (my $head = readdir(HEADS)) {\n \t if (-f $state->{CVSROOT} . '/refs/heads/' . $head) {\n \t print \"E cvs update: New directory `$head'\\n\";\n@@ -1737,14 +1747,16 @@ sub req_annotate\n \tsystem(\"git-read-tree\", $lastseenin);\n \tunless ($? == 0)\n \t{\n-\t die \"Error running git-read-tree $lastseenin $file_index $!\";\n+\t print \"E error running git-read-tree $lastseenin $file_index $!\\n\";\n+\t return;\n \t}\n \t$log->info(\"Created index '$file_index' with commit $lastseenin - exit status $?\");\n \n # do a checkout of the file\n system('git-checkout-index', '-f', '-u', $filename);\n unless ($? == 0) {\n- die \"Error running git-checkout-index -f -u $filename : $!\";\n+ print \"E error running git-checkout-index -f -u $filename : $!\\n\";\n+ return;\n }\n \n $log->info(\"Annotate $filename\");\n@@ -1754,7 +1766,11 @@ sub req_annotate\n # git-jsannotate telling us about commits we are hiding\n # from the client.\n \n- open(ANNOTATEHINTS, \">$tmpdir/.annotate_hints\") or die \"Error opening > $tmpdir/.annotate_hints $!\";\n+ my $a_hints = \"$tmpdir/.annotate_hints\";\n+ if (!open(ANNOTATEHINTS, '>', $a_hints)) {\n+ print \"E failed to open '$a_hints' for writing: $!\\n\";\n+ return;\n+ }\n for (my $i=0; $i < @$revisions; $i++)\n {\n print ANNOTATEHINTS $revisions->[$i][2];\n@@ -1765,11 +1781,14 @@ sub req_annotate\n }\n \n print ANNOTATEHINTS \"\\n\";\n- close ANNOTATEHINTS;\n+ close ANNOTATEHINTS\n+ or (print \"E failed to write $a_hints: $!\\n\"), return;\n \n- my $annotatecmd = 'git-annotate';\n- open(ANNOTATE, \"-|\", $annotatecmd, '-l', '-S', \"$tmpdir/.annotate_hints\", $filename)\n-\t or die \"Error invoking $annotatecmd -l -S $tmpdir/.annotate_hints $filename : $!\";\n+ my @cmd = (qw(git-annotate -l -S), $a_hints, $filename);\n+ if (!open(ANNOTATE, \"-|\", @cmd)) {\n+ print \"E error invoking \". join(' ',@cmd) .\": $!\\n\";\n+ return;\n+ }\n my $metadata = {};\n print \"E Annotations for $filename\\n\";\n print \"E ***************\\n\";\n@@ -1996,12 +2015,12 @@ sub transmitfile\n {\n open NEWFILE, \">\", $targetfile or die(\"Couldn't open '$targetfile' for writing : $!\");\n print NEWFILE $_ while ( <$fh> );\n- close NEWFILE;\n+ close NEWFILE or die(\"Failed to write '$targetfile': $!\");\n } else {\n print \"$size\\n\";\n print while ( <$fh> );\n }\n- close $fh or die (\"Couldn't close filehandle for transmitfile()\");\n+ close $fh or die (\"Couldn't close filehandle for transmitfile(): $!\");\n } else {\n die(\"Couldn't execute git-cat-file\");\n }\n@@ -2501,17 +2520,14 @@ sub update\n if ($parent eq $lastpicked) {\n next;\n }\n- open my $p, 'git-merge-base '. $lastpicked . ' '\n- . $parent . '|';\n- my @output = (<$p>);\n- close $p;\n- my $base = join('', @output);\n+ my $base = safe_pipe_capture('git-merge-base',\n+\t\t\t\t\t\t $lastpicked, $parent);\n chomp $base;\n if ($base) {\n my @merged;\n # print \"want to log between $base $parent \\n\";\n open(GITLOG, '-|', 'git-log', \"$base..$parent\")\n- or die \"Cannot call git-log: $!\";\n+\t\t\t or die \"Cannot call git-log: $!\";\n my $mergedhash;\n while () {\n chomp;\n-- \n1.5.3.rc1.5.g9d947\n"}]}