Re: [PATCH] git-cvsserver: detect/diagnose write failure, etc.

[Junio C Hamano <gitster@pobox.com>]

Jim Meyering <jim@meyering.net> writes:

> Beware: in some contexts (when running as server), one must
> not "die", but rather return an "error" indicator to the client.
> I did that in the second hunk.  However, I haven't carefully
> audited the others, and so, some of the "die" calls I added may
> end up killing the server, when a gentler failure is required.
>
> I've just looked, and can confirm that my change to req_Modified
> (first hunk) is wrong.  It should not die.  Rather, it should probably
> do something like the "print "E ... in hunk#2.  Ideally, someone
> would write a test to exercise code like this, to make sure it works.
>
> Jim

> diff --git a/git-cvsserver.perl b/git-cvsserver.perl
> index 5cbf27e..8d8d6f5 100755
> --- a/git-cvsserver.perl
> +++ b/git-cvsserver.perl
> @@ -644,7 +644,7 @@ sub req_Modified
>          $bytesleft -= $blocksize;
>      }
>  
> -    close $fh;
> +    close $fh or die "Failed to write temporary, $filename: $!";

True; dying is not appropriate here.

> @@ -901,8 +901,13 @@ sub req_update
>      # projects (heads in this case) to checkout.
>      #
>      if ($state->{module} eq '') {
> +	my $heads_dir = $state->{CVSROOT} . '/refs/heads';
> +	if (!opendir HEADS, $heads_dir) {
> +	  print "E [server aborted]: Failed to open directory, $heads_dir: $!\n"
> +	    . "error\n";
> +	  return 0;
> +	}
>          print "E cvs update: Updating .\n";
> -	opendir HEADS, $state->{CVSROOT} . '/refs/heads';
>  	while (my $head = readdir(HEADS)) {
>  	    if (-f $state->{CVSROOT} . '/refs/heads/' . $head) {
>  	        print "E cvs update: New directory `$head'\n";

Hmph.  The clients get no entries in the listing either way, but
I would say this is an improvement.

> @@ -1754,7 +1759,9 @@ sub req_annotate
>          # git-jsannotate telling us about commits we are hiding
>          # from the client.
>  
> -        open(ANNOTATEHINTS, ">$tmpdir/.annotate_hints") or die "Error opening > $tmpdir/.annotate_hints $!";
> +        my $a_hints = "$tmpdir/.annotate_hints";
> +        open(ANNOTATEHINTS, '>', $a_hints)
> +	  or die "Failed to open '$a_hints' for writing: $!";
>          for (my $i=0; $i < @$revisions; $i++)
>          {
>              print ANNOTATEHINTS $revisions->[$i][2];

Hmph, we seem to have:

                $log->warn("Error in annotate output! LINE: $_");
                print "E Annotate error \n";

which suggest me that throwing "E error" at the client and
returning might be better?

> @@ -1765,11 +1772,11 @@ sub req_annotate
>          }
>  
>          print ANNOTATEHINTS "\n";
> -        close ANNOTATEHINTS;
> +        close ANNOTATEHINTS or die "Failed to write $a_hints: $!";
>  
> -        my $annotatecmd = 'git-annotate';
> -        open(ANNOTATE, "-|", $annotatecmd, '-l', '-S', "$tmpdir/.annotate_hints", $filename)
> -	    or die "Error invoking $annotatecmd -l -S $tmpdir/.annotate_hints $filename : $!";
> +        my @cmd = (qw(git-annotate -l -S), $a_hints, $filename);
> +        open(ANNOTATE, "-|", @cmd)
> +	    or die "Error invoking ". join(' ',@cmd) .": $!";
>          my $metadata = {};
>          print "E Annotations for $filename\n";
>          print "E ***************\n";

Likewise...

> @@ -1996,12 +2003,12 @@ sub transmitfile
>          {
>              open NEWFILE, ">", $targetfile or die("Couldn't open '$targetfile' for writing : $!");
>              print NEWFILE $_ while ( <$fh> );
> -            close NEWFILE;
> +            close NEWFILE or die("Failed to write '$targetfile': $!");
>          } else {
>              print "$size\n";
>              print while ( <$fh> );
>          }
> -        close $fh or die ("Couldn't close filehandle for transmitfile()");
> +        close $fh or die ("Couldn't close filehandle for transmitfile(): $!");
>      } else {
>          die("Couldn't execute git-cat-file");
>      }

Ok.

> @@ -2501,17 +2508,14 @@ sub update
>                      if ($parent eq $lastpicked) {
>                          next;
>                      }
> -                    open my $p, 'git-merge-base '. $lastpicked . ' '
> -                    . $parent . '|';
> -                    my @output = (<$p>);
> -                    close $p;
> -                    my $base = join('', @output);
> +                    my $base = safe_pipe_capture('git-merge-base',
> +						 $lastpicked, $parent);
>                      chomp $base;
>                      if ($base) {
>                          my @merged;
>                          # print "want to log between  $base $parent \n";
>                          open(GITLOG, '-|', 'git-log', "$base..$parent")
> -                        or die "Cannot call git-log: $!";
> +			  or die "Cannot call git-log: $!";
>                          my $mergedhash;
>                          while (<GITLOG>) {
>                              chomp;

Ok.