Re: [PATCH v4 4/4] maintenance: add support for systemd timers on Linux

From: Eric Sunshine <sunshine@sunshineco.com>
Date: May 24, 2021, 16:39 UTC
Message-ID: <CAPig+cQSbHM1ph+hg2dkR3fBoxF2pxAh+xYe8to4yGfXOpMLMA@mail.gmail.com>
In-Reply-To: <87r1hwo3e3.fsf@evledraar.gmail.com>

On Mon, May 24, 2021 at 6:03 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 21 quoted lines

> On Mon, May 24 2021, Lénaïc Huard wrote:
> > +test_expect_success 'start and stop Linux/systemd maintenance' '
> > +     write_script print-args <<-\EOF &&
> > +     printf "%s\n" "$*" >>args
> > +     EOF
> > +
> > +     XDG_CONFIG_HOME="$PWD" &&
> > +     export XDG_CONFIG_HOME &&
> > +     rm -f args &&
>
> If you're going to care about cleanup here, and personally I wouldn't,
> just call that "args" by the name "expect" instead (as is convention)
> and clobber it every time...
>
> Anyway, a better way to do the cleanup is:
>
>     test_when_finished "rm args" &&
>     echo this is the first time you write the file >args
>     [the rest of the test code]
>
> Then you don't need to re-rm it.

A few comments:

This is following an already-established pattern in this test script, so it would be unfortunate and out of place to change only these newly-added tests, thus it's probably better to follow the existing idiom as is done here. If someone wants to rework all this, it can be done later script-wide and need not be part of this series nor done by Lénaïc.

The name `args` is reflective of what is being captured here; specifically, it captures the arguments passed to the system-specific scheduler command. It's also the name used by all the other tests, so it's probably fine as-is.

The git-maintenance command is invoked multiple times in a single test and `args` needs to be removed before each invocation since its content is accumulated via `>>` within the `print-args` script, which is necessary since that script may be run multiple times by a single git-maintenance command. So, `rm -f args` is not mere cleanup here; it's an integral part of the test, thus test_when_finished() would be incorrect.