[2/3] t4216: fix broken '&&'-chain
diff mbox series

Message ID 5a20a97658fa8e6c874c9c9cafb2cf49e39f94d6.1593536481.git.me@ttaylorr.com
State New
Headers show
Series
  • commit-graph: introduce 'core.useBloomFilters'
Related show

Commit Message

Taylor Blau June 30, 2020, 5:17 p.m. UTC
In a759bfa9ee (t4216: add end to end tests for git log with Bloom
filters, 2020-04-06), a 'rm' invocation was added without a
corresponding '&&' chain.

This ends up working fine when the file already exists, in which case
'rm' exits cleanly and the rest of the function executes normally. When
the file does _not_ exist, however, 'rm' returns an unclean exit code,
causing the function to terminate.

Fix this by making the test use an '&&'-chain, and passing '-f' to
ignore missing files (as can be the case when specifying which tests are
'--run').

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 t/t4216-log-bloom.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Eric Sunshine June 30, 2020, 5:50 p.m. UTC | #1
On Tue, Jun 30, 2020 at 1:17 PM Taylor Blau <me@ttaylorr.com> wrote:
> In a759bfa9ee (t4216: add end to end tests for git log with Bloom
> filters, 2020-04-06), a 'rm' invocation was added without a
> corresponding '&&' chain.
>
> This ends up working fine when the file already exists, in which case
> 'rm' exits cleanly and the rest of the function executes normally. When
> the file does _not_ exist, however, 'rm' returns an unclean exit code,
> causing the function to terminate.

This explanation makes no sense. Since this command was not part of
the &&-chain, its failure would not cause the function to terminate
prematurely nor would it affect the return value of the function. This
explanation would make sense, however, if you're talking about the
behavior _after_ fixing the broken &&-chain.

> Fix this by making the test use an '&&'-chain, and passing '-f' to
> ignore missing files (as can be the case when specifying which tests are
> '--run').

The entire commit message is talking about implementation details and
merely repeats what the subject and patch itself already say quite
clearly; anyone familiar with 'rm' understands implicitly that '-f'
must be added to incorporate it into the &&-chain if the file's
presence is not guaranteed. Thus, you could drop the entire body of
the commit message without losing clarity...

With one minor exception: What is much more interesting for the reader
to know is whether the file being removed is guaranteed to exist (in
which case '-f' is unnecessary) or may be missing (requiring '-f'),
and under what conditions it might be missing. The very last part of
the last sentence of the current commit message gives a good hint
about the latter, thus would be a good bit to retain.

> diff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh
> @@ -53,7 +53,7 @@ sane_unset GIT_TRACE2_PERF_BRIEF
>  sane_unset GIT_TRACE2_CONFIG_PARAMS

Not related to this patch, but 'sane_unset' is pointless outside of a
test since there is no &&-chain to maintain. Plain 'unset' would work
just as well and be less misleading.

>  setup () {
> -       rm "$TRASH_DIRECTORY/trace.perf"
> +       rm -f "$TRASH_DIRECTORY/trace.perf" &&
Taylor Blau June 30, 2020, 6:39 p.m. UTC | #2
On Tue, Jun 30, 2020 at 01:50:22PM -0400, Eric Sunshine wrote:
> On Tue, Jun 30, 2020 at 1:17 PM Taylor Blau <me@ttaylorr.com> wrote:
> > In a759bfa9ee (t4216: add end to end tests for git log with Bloom
> > filters, 2020-04-06), a 'rm' invocation was added without a
> > corresponding '&&' chain.
> >
> > This ends up working fine when the file already exists, in which case
> > 'rm' exits cleanly and the rest of the function executes normally. When
> > the file does _not_ exist, however, 'rm' returns an unclean exit code,
> > causing the function to terminate.
>
> This explanation makes no sense. Since this command was not part of
> the &&-chain, its failure would not cause the function to terminate
> prematurely nor would it affect the return value of the function. This
> explanation would make sense, however, if you're talking about the
> behavior _after_ fixing the broken &&-chain.

Fair enough. For what it's worth, this explanation *does* make sense if
you 'set -e' beforehand, which I am accustomed to (and had incorrectly
assumed that tests in 't' also have 'set -e', when they do not).

I've corrected the patch and shortened it to account for your
suggestions. Mind taking a look at the updated version and telling me
what you think?

--- >8 ---

Subject: [PATCH] t4216: fix broken '&&'-chain

The 'rm' added in a759bfa9ee (t4216: add end to end tests for git log
with Bloom filters, 2020-04-06) should be placed within the function's
'&&'-chain.

The file being removed may not exist (for eg., in the case of '--run',
in which case it may not be generated beforehand by a skipped test), and
so add '-f' to account for the file's optional existence.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 t/t4216-log-bloom.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh
index c855bcd3e7..0b4cc4f8d1 100755
--- a/t/t4216-log-bloom.sh
+++ b/t/t4216-log-bloom.sh
@@ -53,7 +53,7 @@ sane_unset GIT_TRACE2_PERF_BRIEF
 sane_unset GIT_TRACE2_CONFIG_PARAMS

 setup () {
-	rm "$TRASH_DIRECTORY/trace.perf"
+	rm -f "$TRASH_DIRECTORY/trace.perf" &&
 	git -c core.commitGraph=false log --pretty="format:%s" $1 >log_wo_bloom &&
 	GIT_TRACE2_PERF="$TRASH_DIRECTORY/trace.perf" git -c core.commitGraph=true log --pretty="format:%s" $1 >log_w_bloom
 }
--
2.27.0.224.g4cfa086e50
Jeff King June 30, 2020, 6:55 p.m. UTC | #3
On Tue, Jun 30, 2020 at 01:50:22PM -0400, Eric Sunshine wrote:

> > diff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh
> > @@ -53,7 +53,7 @@ sane_unset GIT_TRACE2_PERF_BRIEF
> >  sane_unset GIT_TRACE2_CONFIG_PARAMS
> 
> Not related to this patch, but 'sane_unset' is pointless outside of a
> test since there is no &&-chain to maintain. Plain 'unset' would work
> just as well and be less misleading.

We do this in lots of other places, too. Try:

  git grep sane_unset | grep -v '&&'

Though seeing the variables they cover, I think many of them may come
from the same few authors. :)

I wonder if it is worth keeping these as sane_unset, though. Your
comment is based on the knowledge that the "sane" part of the function
is ignoring the return value. But we could conceivably have other
portability fixes (it's not impossible that a shell wants one "unset"
per variable, for example), in which case we'd want it in more places.

That's hypothetical, of course, but saying "just use our portable unset
wrapper that behaves sanely" seems like exactly what these call sites
want, now and in a hypothetical future.

-Peff
Jeff King June 30, 2020, 7:03 p.m. UTC | #4
On Tue, Jun 30, 2020 at 02:39:28PM -0400, Taylor Blau wrote:

> > > This ends up working fine when the file already exists, in which case
> > > 'rm' exits cleanly and the rest of the function executes normally. When
> > > the file does _not_ exist, however, 'rm' returns an unclean exit code,
> > > causing the function to terminate.
> >
> > This explanation makes no sense. Since this command was not part of
> > the &&-chain, its failure would not cause the function to terminate
> > prematurely nor would it affect the return value of the function. This
> > explanation would make sense, however, if you're talking about the
> > behavior _after_ fixing the broken &&-chain.
> 
> Fair enough. For what it's worth, this explanation *does* make sense if
> you 'set -e' beforehand, which I am accustomed to (and had incorrectly
> assumed that tests in 't' also have 'set -e', when they do not).

If we _really_ want to nitpick, it probably wouldn't terminate under
"set -e" because the call to "setup" is itself part of an &&-chain,
which suppresses "-e" handling (which is one of the many confusing "set
-e" behaviors that led us to avoid it in the first place).

But definitely your revised commit message below is more accurate.

However...

> --- >8 ---
> 
> Subject: [PATCH] t4216: fix broken '&&'-chain
> 
> The 'rm' added in a759bfa9ee (t4216: add end to end tests for git log
> with Bloom filters, 2020-04-06) should be placed within the function's
> '&&'-chain.
> 
> The file being removed may not exist (for eg., in the case of '--run',
> in which case it may not be generated beforehand by a skipped test), and
> so add '-f' to account for the file's optional existence.

Is the &&-chain really broken, or is the first command simply not part
of that chain? Perhaps a question for philosophers, but the more applied
question here is: what are we improving, and why?

The original code handled the fact that the file might not exist by not
including its exit code in the &&-chain which leads to the function's
return value. Your new code does so by putting it in the &&-chain but
asking "rm" to ignore errors. Is one better than the other?

I think so, but my argument would be more along the lines of:

  - without "-f", "rm" will complain about a missing file, which is
    distracting noise in the test log

  - once "-f" is added in to suppress that, we might as well add the
    command to the &&-chain. That's our normal style, so readers don't
    have to wonder if it's important or not. Plus it would help avoid a
    broken chain if more commands are added at the beginning of the
    function.

-Peff
Taylor Blau June 30, 2020, 7:12 p.m. UTC | #5
On Tue, Jun 30, 2020 at 03:03:25PM -0400, Jeff King wrote:
> On Tue, Jun 30, 2020 at 02:39:28PM -0400, Taylor Blau wrote:
>
> > > > This ends up working fine when the file already exists, in which case
> > > > 'rm' exits cleanly and the rest of the function executes normally. When
> > > > the file does _not_ exist, however, 'rm' returns an unclean exit code,
> > > > causing the function to terminate.
> > >
> > > This explanation makes no sense. Since this command was not part of
> > > the &&-chain, its failure would not cause the function to terminate
> > > prematurely nor would it affect the return value of the function. This
> > > explanation would make sense, however, if you're talking about the
> > > behavior _after_ fixing the broken &&-chain.
> >
> > Fair enough. For what it's worth, this explanation *does* make sense if
> > you 'set -e' beforehand, which I am accustomed to (and had incorrectly
> > assumed that tests in 't' also have 'set -e', when they do not).
>
> If we _really_ want to nitpick, it probably wouldn't terminate under
> "set -e" because the call to "setup" is itself part of an &&-chain,
> which suppresses "-e" handling (which is one of the many confusing "set
> -e" behaviors that led us to avoid it in the first place).

I learned something new about 'set -e'! I don't mind nitpicking at all,
it's useful information to know...

> But definitely your revised commit message below is more accurate.
>
> However...
>
> > --- >8 ---
> >
> > Subject: [PATCH] t4216: fix broken '&&'-chain
> >
> > The 'rm' added in a759bfa9ee (t4216: add end to end tests for git log
> > with Bloom filters, 2020-04-06) should be placed within the function's
> > '&&'-chain.
> >
> > The file being removed may not exist (for eg., in the case of '--run',
> > in which case it may not be generated beforehand by a skipped test), and
> > so add '-f' to account for the file's optional existence.
>
> Is the &&-chain really broken, or is the first command simply not part
> of that chain? Perhaps a question for philosophers, but the more applied
> question here is: what are we improving, and why?
>
> The original code handled the fact that the file might not exist by not
> including its exit code in the &&-chain which leads to the function's
> return value. Your new code does so by putting it in the &&-chain but
> asking "rm" to ignore errors. Is one better than the other?
>
> I think so, but my argument would be more along the lines of:
>
>   - without "-f", "rm" will complain about a missing file, which is
>     distracting noise in the test log
>
>   - once "-f" is added in to suppress that, we might as well add the
>     command to the &&-chain. That's our normal style, so readers don't
>     have to wonder if it's important or not. Plus it would help avoid a
>     broken chain if more commands are added at the beginning of the
>     function.

I made the change for basically these reasons, but mostly to bring this
function into good style as with the rest of our test suite (there are a
handful of other minor nits that we could look at, such as some odd
spacing, etc.).

Whether or not all of this needs to go into the commit message... I
don't know. On the one hand, I think that your explanation here is
clearer than what I wrote in the commit message, but on the other hand,
I think that amending it again may be belaboring an otherwise simple
change.

If you feel strongly, though, I'm happy to send a revised patch.

> -Peff

Thanks,
Taylor
Jeff King June 30, 2020, 7:19 p.m. UTC | #6
On Tue, Jun 30, 2020 at 03:12:31PM -0400, Taylor Blau wrote:

> > I think so, but my argument would be more along the lines of:
> >
> >   - without "-f", "rm" will complain about a missing file, which is
> >     distracting noise in the test log
> >
> >   - once "-f" is added in to suppress that, we might as well add the
> >     command to the &&-chain. That's our normal style, so readers don't
> >     have to wonder if it's important or not. Plus it would help avoid a
> >     broken chain if more commands are added at the beginning of the
> >     function.
> 
> I made the change for basically these reasons, but mostly to bring this
> function into good style as with the rest of our test suite (there are a
> handful of other minor nits that we could look at, such as some odd
> spacing, etc.).
> 
> Whether or not all of this needs to go into the commit message... I
> don't know. On the one hand, I think that your explanation here is
> clearer than what I wrote in the commit message, but on the other hand,
> I think that amending it again may be belaboring an otherwise simple
> change.
> 
> If you feel strongly, though, I'm happy to send a revised patch.

I agree it's a pretty trivial patch, but I think if it's worth applying
at all, then it's worth justifying it appropriately.

-Peff
Eric Sunshine June 30, 2020, 7:48 p.m. UTC | #7
On Tue, Jun 30, 2020 at 3:03 PM Jeff King <peff@peff.net> wrote:
> [...] what are we improving, and why?
>
> The original code handled the fact that the file might not exist by not
> including its exit code in the &&-chain which leads to the function's
> return value. Your new code does so by putting it in the &&-chain but
> asking "rm" to ignore errors. Is one better than the other?
>
> I think so, but my argument would be more along the lines of:
>
>   - without "-f", "rm" will complain about a missing file, which is
>     distracting noise in the test log

Indeed, a nice detail when reading verbose test output; one less thing
to distract the attention from the real/important problems.

>   - once "-f" is added in to suppress that, we might as well add the
>     command to the &&-chain. That's our normal style, so readers don't
>     have to wonder if it's important or not. Plus it would help avoid a
>     broken chain if more commands are added at the beginning of the
>     function.

The bit about commands possibly being added at the beginning of the
function probably deserves its own bullet point. I often (relatively
speaking) cite that reason when asking people to &&-chain variable
assignments at the beginning of a function.

    func () {
        a=1 &&
        b=2 &&
        ...
    }

Patch
diff mbox series

diff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh
index c855bcd3e7..0b4cc4f8d1 100755
--- a/t/t4216-log-bloom.sh
+++ b/t/t4216-log-bloom.sh
@@ -53,7 +53,7 @@  sane_unset GIT_TRACE2_PERF_BRIEF
 sane_unset GIT_TRACE2_CONFIG_PARAMS
 
 setup () {
-	rm "$TRASH_DIRECTORY/trace.perf"
+	rm -f "$TRASH_DIRECTORY/trace.perf" &&
 	git -c core.commitGraph=false log --pretty="format:%s" $1 >log_wo_bloom &&
 	GIT_TRACE2_PERF="$TRASH_DIRECTORY/trace.perf" git -c core.commitGraph=true log --pretty="format:%s" $1 >log_w_bloom
 }