diff --git a/ddclient.in b/ddclient.in index b81d1b4..03e6735 100755 --- a/ddclient.in +++ b/ddclient.in @@ -1286,7 +1286,7 @@ sub main { %opt = %saved_opt; read_config(opt('file'), \%config, \%globals); init_config(); - read_recap(opt('cache'), \%recap); + read_recap(opt('cache')); print_info() if opt('debug') && opt('verbose'); $daemon = opt('daemon'); @@ -1626,20 +1626,18 @@ sub write_recap { ###################################################################### sub read_recap { my $file = shift; - my $config = shift; my $globals = {}; - %{$config} = (); + %recap = (); return if !(-e $file); my %saved = %opt; %opt = (); - $saved_recap = _read_config($config, $globals, "##\\s*$program-$version\\s*", $file); + $saved_recap = _read_config(\%recap, $globals, "##\\s*$program-$version\\s*", $file); %opt = %saved; # TODO: Why is this iterating over `keys(%recap)`? Shouldn't it iterate over `keys(%config)` # instead? for my $h (keys(%recap)) { - # TODO: `$config->{$h}` is guaranteed to exist because `$config` is `\%recap` and we're - # iterating over `keys(%recap)`. - next if !exists($config->{$h}); + # TODO: `$recap{$h}` is guaranteed to exist because we're iterating over `keys(%recap)`. + next if !exists($recap{$h}); # Only status variables are copied from `%recap` into `%config` because the recap should # not change the configuration. See the documenting comments for `%recap`. # TODO: Hard-coding this list impairs readability and maintainability. In particular, @@ -1648,15 +1646,15 @@ sub read_recap { # variables. for my $v (qw(atime mtime wtime ipv4 ipv6 status-ipv4 status-ipv6 warned-min-interval warned-min-error-interval)) { - # TODO: This is a no-op because `$config` is `\%recap`. I believe the original intent - # was to copy values that convey update status from `%recap` into `%config` so that - # ddclient can be restarted and resume where it left off and not violate - # `min-interval`, `min-error-interval`, etc. For the short term, this should copy the - # values into `%config`, not `%recap`. In the long term, nothing should be copied from - # `%recap` into `%config` because `%config` is not the semantically appropriate place - # to record update status. Code that currently reads/sets `$config{$h}{'status-ipv4'}` - # (and friends) should instead access `$recap{$h}{'status-ipv4'}` directly. - $config->{$h}{$v} = $recap{$h}{$v} if exists($recap{$h}{$v}); + # TODO: This is a no-op. I believe the original intent was to copy values that convey + # update status from `%recap` into `%config` so that ddclient can be restarted and + # resume where it left off and not violate `min-interval`, `min-error-interval`, etc. + # For the short term, this should copy the values into `%config`, not `%recap`. In the + # long term, nothing should be copied from `%recap` into `%config` because `%config` is + # not the semantically appropriate place to record update status. Code that currently + # reads/sets `$config{$h}{'status-ipv4'}` (and friends) should instead access + # `$recap{$h}{'status-ipv4'}` directly. + $recap{$h}{$v} = $recap{$h}{$v} if exists($recap{$h}{$v}); } } } diff --git a/t/read_recap.pl b/t/read_recap.pl index 47df70b..5577738 100644 --- a/t/read_recap.pl +++ b/t/read_recap.pl @@ -179,7 +179,7 @@ for my $tc (@test_cases) { local %ddclient::config; $ddclient::config{$_} = {%{$want_config{$_}}} for keys(%want_config); - ddclient::read_recap($cachef->filename(), \%ddclient::recap); + ddclient::read_recap($cachef->filename()); TODO: { local $TODO = $tc->{want_TODO};