Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

Untangle issues with END blocks, and exit codes (introduce fatal_exit)

When $? is set inside of an END block it changes the exit code returned
to the OS. (See the docs for $? in perlvar.)

We were calling routines that would execute system/backticks inside of
an END block in Git::Deploy::Timing, which would clobber the exit code.

Various patches ensued to try to work around this fact, but ultimately
confused the matter more. This patch effectively reverts those patches,
and makes it so we get configuration for Git::Deploy::Timing at startup
and not shutdown, thus leaving the exit code alone during teardown.

This patch also introduces a new sub exported by Git::Deploy::Say called
fatal_exit($code,@msg) which is intended for top level routines to use for
fatal exits which should return a specific exit code. Internal routines
should continue to use _die() as it is trapable wheras fatal_exit() is
not. In the long run we need to rething this separation of
functionality.
  • Loading branch information...
commit 5b1ba41ac49d59a18e55156404650a4c3568dbd6 1 parent d3a7b78
@demerphq demerphq authored
View
35 bin/git-deploy
@@ -36,14 +36,7 @@ use lib $GIT_DEPLOY_LIB;
use Git::Deploy::Timing qw(
push_timings
should_write_timings
- write_timings
);
-local $SIG{__DIE__} = sub {
- print STDERR "@_";
- push_timings("gdt_end");
- write_timings();
- die;
-};
use Git::Deploy::Say;
use Git::Deploy;
@@ -103,7 +96,7 @@ sub do_pre_checks {
push_timings("gdt_internal__do_pre_checks__start");
if ( my $unclean_status= check_if_working_dir_is_clean() ) {
- _die "working directory not clean. git status reports:\n" . $unclean_status;
+ _fatal_exit 1, "working directory not clean. git status reports:\n" . $unclean_status;
}
# check that we are actually on a branch
@@ -113,7 +106,7 @@ sub do_pre_checks {
"Checking if this commit is on any branches -- this may take a minute\n";
my @branches= what_branches_can_reach_head();
if ( !@branches ) {
- _die "The current commit is not reachable from any branch tip\n",
+ _fatal_exit 1, "The current commit is not reachable from any branch tip\n",
"This probably means someone committed a change against an old commit\n",
"You should probably do something like:\n\n",
"\tgit checkout -b temporary_branch HEAD\n",
@@ -123,7 +116,7 @@ sub do_pre_checks {
"However you should investigate what changes were involved before doing so\n";
}
else {
- _die join( "\n\t", "The current checkout is on the following branches:", @branches ),
+ _fatal_exit 1, join( "\n\t", "The current checkout is on the following branches:", @branches ),
"\n",
"This most likely means that someone has checked out an old commit\n",
"and you need to do:\n",
@@ -449,7 +442,7 @@ GetOptions(
'v|verbose+' => \( $VERBOSE= ($ENV{GIT_DEPLOY_VERBOSE} || 0) ),
'help|?' => \( my $help ),
'man' => \( my $man ),
- 'V|version' => sub { print "$0 v" . ($Git::Deploy::VERSION || "Git") . "\n"; exit },
+ 'V|version' => sub { print "$0 v" . ($Git::Deploy::VERSION || "Git") . "\n"; exit(0); },
) or pod2usage(2);
pod2usage(1) if $help;
pod2usage( -exitstatus => 0, -verbose => 2 ) if $man;
@@ -493,7 +486,7 @@ init_gitdir();
if (defined(my $require= get_config_int("restrict-umask",undef))) {
my $umask= umask;
if ( $umask != $require ) {
- _die sprintf( "Your umask is not set properly; got %04o instead of %04o\n", $umask, $require);
+ _fatal_exit 17, sprintf( "Your umask is not set properly; got %04o instead of %04o\n", $umask, $require);
}
}
@@ -605,7 +598,8 @@ if ( $action eq 'status' ) {
my @extra= @ARGV;
if ( $git_wrap{$action} ) {
if ( !$start_tag ) {
- exit;
+ _warn "Not in rollout session. Maybe you meant to use git and not git-deploy?";
+ exit(1);
}
elsif ($rollout_tag) {
exec( "git", $action, @extra, "$rollout_tag..$start_tag" );
@@ -617,7 +611,7 @@ if ( $git_wrap{$action} ) {
if ( $action eq 'modified' ) {
show_modified( $start_tag, $rollout_tag );
- exit;
+ exit(0);
}
my $revert_choice;
@@ -634,7 +628,7 @@ if (@ARGV) {
# we want this before actions are handled. but not for show_actions that
# dont involve looking at the git tree.
if ( !$prefix ) {
- _die "It appears you have not specified a tag prefix and it is not configured either.\n"
+ _fatal_exit 19, "It appears you have not specified a tag prefix and it is not configured either.\n"
. "You can use:\n\n"
. " git config deploy.tag-prefix PREFIX\n\n"
. "to configure this repository for deployment, or provide one on the command line.\n";
@@ -657,7 +651,7 @@ if ( $list || $show_tag ) {
}
if (!$force and (!get_config("user.name","") or !get_config("user.email",""))) {
- _die "Please make sure you execute:\n\n"
+ _fatal_exit 17, "Please make sure you execute:\n\n"
. " git config --global user.name 'Your Name'\n"
. " git config --global user.email 'email\@host.com'\n"
. "\n"
@@ -671,7 +665,7 @@ if ($check_clean) {
if ( $make_tag or $action eq 'tag' ) {
do_fetch($remote_site);
- _die "You aren't allowed to make tags for the prefix '$prefix' (can-make-tags is 'false')"
+ _fatal_exit 1, "You aren't allowed to make tags for the prefix '$prefix' (can-make-tags is 'false')"
unless get_config_bool("can-make-tags",'false');
if ( !@message and $action eq 'tag' ) {
if ( $prefix =~ /^(\w+?)_?prt$/ ) {
@@ -681,7 +675,7 @@ if ( $make_tag or $action eq 'tag' ) {
push @message, "Working tag for '$prefix'\n";
}
}
- _die "Message is not optional with --make-tag" unless @message;
+ _fatal_exit 1, "Message is not optional with --make-tag" unless @message;
my $tag= make_dated_tag( $prefix, $date_fmt, @message )
or _die "Failed to create tag!\n";
if ( $action eq 'tag' ) {
@@ -862,7 +856,7 @@ while (@actions) {
_warn "--force enabling rolling out same thing you had when you started\n";
}
else {
- _die "It seems like there is nothing to $action\n",
+ _fatal_exit 1 => "It seems like there is nothing to $action\n",
"The commit for HEAD ($commit_for_HEAD)\n",
"Is the same as the start commit ($commit_for_start, tag $start_tag)\n",
"\n",
@@ -1150,9 +1144,6 @@ while (@actions) {
push_timings("gdt_push_tags");
push_tags( $remote_site );
-push_timings("gdt_end");
-write_timings();
-
exit(0);
__END__
View
8 lib/Git/Deploy.pm
@@ -11,7 +11,7 @@ use Sys::Hostname qw(hostname);
use Fcntl qw(:DEFAULT :flock);
use Cwd qw(cwd abs_path);
use File::Spec::Functions qw(catdir);
-use Git::Deploy::Timing qw(push_timings);
+use Git::Deploy::Timing qw(push_timings init_timings);
use Git::Deploy::Say;
use Data::Dumper;
@@ -103,6 +103,9 @@ sub init_gitdir {
# place regardless of where the tool was run from).
chdir "$gitdir/.."
or _die "Failed to chdir to root of git working tree:'$gitdir/..': $!";
+
+ init_timings(get_config_bool("log-timing-data",'false'), log_directory());
+
return $gitdir;
}
@@ -1304,7 +1307,7 @@ sub check_for_unpushed_commits {
}
}
push_timings("gdt_internal__check_for_unpushed_commits__end");
- _die "Will not proceed.\n" if @cherry and !$force;
+ _fatal_exit 1, "Will not proceed.\n" if @cherry and !$force;
return 0;
}
@@ -1501,4 +1504,5 @@ sub log_directory {
return $log_directory;
}
+
1;
View
10 lib/Git/Deploy/Say.pm
@@ -21,6 +21,7 @@ BEGIN {
our @EXPORT = qw(
_error
_die
+ _fatal_exit
_warn
_info
_say
@@ -163,6 +164,15 @@ sub _die(@) {
die colored([COLOR_DIE], $msg), "\n";
}
+sub _fatal_exit(@) {
+ my $code= shift;
+ my $msg= _msg( "# FATAL:", @_ );
+ __log($msg);
+ chomp $msg;
+ warn colored([COLOR_DIE], $msg), "\n";
+ exit($code);
+}
+
sub _error(@) {
__say( [COLOR_DIE], "# ERROR:", @_ );
} # still bad, but not fatal
View
30 lib/Git/Deploy/Timing.pm
@@ -5,12 +5,12 @@ use Exporter 'import';
use Time::HiRes;
our @EXPORT = qw(
+ init_timings
push_timings
should_write_timings
- write_timings
);
-our (@timings, $write_timings, @real_argv);
+our (@timings, $write_timings, @real_argv, $enabled, $log_directory);
BEGIN {
# @timings is a set of 4-tuples: [ $tag, $time_stamp, $time_since_last_step, $time_since_start_tag ]
@timings= (
@@ -26,6 +26,11 @@ BEGIN {
@real_argv= @ARGV;
}
+sub init_timings {
+ $enabled= shift;
+ $log_directory= shift;
+}
+
sub should_write_timings {
$write_timings= 1;
}
@@ -47,13 +52,10 @@ sub push_timings {
}
sub write_timings {
- return unless $write_timings;
- # Do we even have to write the timing data?
- require Git::Deploy;
- return unless Git::Deploy::get_config_bool("log-timing-data",'false');
+ return unless $enabled && $write_timings;
+
# Where do we write it?
- my $log_directory;
- unless ( $log_directory = Git::Deploy::log_directory() ) {
+ unless ( $log_directory ) {
warn "Not writing timing data: 'log_directory' has not been configured.";
return;
}
@@ -71,4 +73,16 @@ sub write_timings {
close $fh;
}
+END {
+ # Note! Nothing called in here should shell out, or in any way change $?
+ # this means call anything that might use system() or `` or qx().
+ # See perldoc perlvar and read the docs on $?.
+ eval {
+ push_timings("gdt_end");
+ write_timings();
+ 1;
+ } or warn "Failed to write timings: $@";
+}
+
+
1;
View
2  t/lib/Git/Deploy/Test.pm
@@ -111,6 +111,8 @@ sub _run_git_deploy {
my ($ctx, %args) = @_;
my $wanted_exit_code = $args{wanted_exit_code} || 0;
+ local $Test::Builder::Level = $Test::Builder::Level + 1;
+
my $out_dir = $ctx->{out_dir};
$ctx->{"last_$_"} = catfile($out_dir, "last_$_") for qw(stdout stderr);
_system "$ctx->{git_deploy} $args{args} >$ctx->{last_stdout} 2>$ctx->{last_stderr}", $wanted_exit_code;
View
1  t/run.t
@@ -18,6 +18,7 @@ git_deploy_test(
"A rollout etc.",
sub {
my $ctx = shift;
+ local $Test::Builder::Level = $Test::Builder::Level + 1;
_run_git_deploy(
$ctx,
args => "start",
Please sign in to comment.
Something went wrong with that request. Please try again.