Jim Meyering <jim@xxxxxxxxxxxx> 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.
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
