* [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).