* [PATCH] git-cvsserver: detect/diagnose write failure, etc. @ 2007-07-06 12:18 Jim Meyering 2007-07-11 21:52 ` Junio C Hamano 0 siblings, 1 reply; 3+ messages in thread From: Jim Meyering @ 2007-07-06 12:18 UTC (permalink / raw) To: git Currently I maintain a CVS repository that mirrors the primary git repository for the GNU coreutils. It is kept in sync via a relatively fragile "update" (push) hook that runs git-cvsexportcommit every time I push to the git repository. Since it'd be nice to keep cvs support (so as not to *require* everyone to switch to git right away), without the actual CVS repository, I'm considering using git-cvsserver. However, before I switch, I'd like to have a little more confidence that it is reliable. I've included a patch below that makes it more likely to report if/when something goes wrong, usually just write errors. 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: $!"; # Ensure we have something sensible for the file mode if ( $mode =~ /u=(\w+)/ ) @@ -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"; @@ -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]; @@ -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"; @@ -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"); } @@ -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; -- 1.5.3.rc0.11.ge2b1a-dirty ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] git-cvsserver: detect/diagnose write failure, etc. 2007-07-06 12:18 [PATCH] git-cvsserver: detect/diagnose write failure, etc Jim Meyering @ 2007-07-11 21:52 ` Junio C Hamano 2007-07-14 18:48 ` Jim Meyering 0 siblings, 1 reply; 3+ messages in thread From: Junio C Hamano @ 2007-07-11 21:52 UTC (permalink / raw) To: Jim Meyering; +Cc: git, Frank Lichtenheld 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. ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] git-cvsserver: detect/diagnose write failure, etc. 2007-07-11 21:52 ` Junio C Hamano @ 2007-07-14 18:48 ` Jim Meyering 0 siblings, 0 replies; 3+ messages in thread From: Jim Meyering @ 2007-07-14 18:48 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Frank Lichtenheld Junio C Hamano <gitster@pobox.com> wrote: > 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. ... Hi Jun, Thanks for the feedback. Since no one else stepped forward, I've gone ahead and addressed the problems -- though I haven't tested any of these server-side diagnostics. I've added two new checks in req_Modified, to better diagnose an incomplete request. Also, I converted each of the "die" stmts in req_annotate to print "E..." + return. Signed-off-by: Jim Meyering <jim@meyering.net> --- git-cvsserver.perl | 50 +++++++++++++++++++++++++++++++++----------------- 1 files changed, 33 insertions(+), 17 deletions(-) diff --git a/git-cvsserver.perl b/git-cvsserver.perl index 10aba50..ae7d511 100755 --- a/git-cvsserver.perl +++ b/git-cvsserver.perl @@ -623,8 +623,12 @@ sub req_Modified my ( $cmd, $data ) = @_; my $mode = <STDIN>; + defined $mode + or (print "E end of file reading mode for $data\n"), return; chomp $mode; my $size = <STDIN>; + defined $size + or (print "E end of file reading size of $data\n"), return; chomp $size; # Grab config information @@ -644,7 +648,8 @@ sub req_Modified $bytesleft -= $blocksize; } - close $fh; + close $fh + or (print "E failed to write temporary, $filename: $!\n"), return; # Ensure we have something sensible for the file mode if ( $mode =~ /u=(\w+)/ ) @@ -901,8 +906,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: $!\nerror\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"; @@ -1737,14 +1747,16 @@ sub req_annotate system("git-read-tree", $lastseenin); unless ($? == 0) { - die "Error running git-read-tree $lastseenin $file_index $!"; + print "E error running git-read-tree $lastseenin $file_index $!\n"; + return; } $log->info("Created index '$file_index' with commit $lastseenin - exit status $?"); # do a checkout of the file system('git-checkout-index', '-f', '-u', $filename); unless ($? == 0) { - die "Error running git-checkout-index -f -u $filename : $!"; + print "E error running git-checkout-index -f -u $filename : $!\n"; + return; } $log->info("Annotate $filename"); @@ -1754,7 +1766,11 @@ 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"; + if (!open(ANNOTATEHINTS, '>', $a_hints)) { + print "E failed to open '$a_hints' for writing: $!\n"; + return; + } for (my $i=0; $i < @$revisions; $i++) { print ANNOTATEHINTS $revisions->[$i][2]; @@ -1765,11 +1781,14 @@ sub req_annotate } print ANNOTATEHINTS "\n"; - close ANNOTATEHINTS; + close ANNOTATEHINTS + or (print "E failed to write $a_hints: $!\n"), return; - 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); + if (!open(ANNOTATE, "-|", @cmd)) { + print "E error invoking ". join(' ',@cmd) .": $!\n"; + return; + } my $metadata = {}; print "E Annotations for $filename\n"; print "E ***************\n"; @@ -1996,12 +2015,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"); } @@ -2501,17 +2520,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; -- 1.5.3.rc1.5.g9d947 ^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2007-07-14 18:48 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2007-07-06 12:18 [PATCH] git-cvsserver: detect/diagnose write failure, etc Jim Meyering 2007-07-11 21:52 ` Junio C Hamano 2007-07-14 18:48 ` Jim Meyering
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).