Re: [PATCH 2/3] t4216: fix broken '&&'-chain

From: Jeff King <peff@peff.net>
Date: Jun 30, 2020, 18:55 UTC
Message-ID: <20200630185536.GA1888406@coredump.intra.peff.net>
In-Reply-To: <CAPig+cQv3cHe5ci3mDvNKYXbVQt6Rp5icG-woaDCqfAtzZ6SZw@mail.gmail.com>

On Tue, Jun 30, 2020 at 01:50:22PM -0400, Eric Sunshine wrote:

Show 7 quoted lines

> > 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