Git Mailing List Archive on lore.kernel.org
 help / color / Atom feed
From: Jeff King <peff@peff.net>
To: Eric Sunshine <sunshine@sunshineco.com>
Cc: Taylor Blau <me@ttaylorr.com>, Git List <git@vger.kernel.org>,
	Derrick Stolee <dstolee@microsoft.com>
Subject: Re: [PATCH 2/3] t4216: fix broken '&&'-chain
Date: Tue, 30 Jun 2020 14:55:36 -0400
Message-ID: <20200630185536.GA1888406@coredump.intra.peff.net> (raw)
In-Reply-To: <CAPig+cQv3cHe5ci3mDvNKYXbVQt6Rp5icG-woaDCqfAtzZ6SZw@mail.gmail.com>

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

  parent reply index

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-30 17:17 [PATCH 0/3] commit-graph: introduce 'core.useBloomFilters' Taylor Blau
2020-06-30 17:17 ` [PATCH 1/3] commit-graph: pass a 'struct repository *' in more places Taylor Blau
2020-06-30 20:52   ` Derrick Stolee
2020-06-30 17:17 ` [PATCH 2/3] t4216: fix broken '&&'-chain Taylor Blau
2020-06-30 17:50   ` Eric Sunshine
2020-06-30 18:39     ` Taylor Blau
2020-06-30 19:03       ` Jeff King
2020-06-30 19:12         ` Taylor Blau
2020-06-30 19:19           ` Jeff King
2020-06-30 19:48         ` Eric Sunshine
2020-06-30 18:55     ` Jeff King [this message]
2020-06-30 17:17 ` [PATCH 3/3] commit-graph: respect 'core.useBloomFilters' Taylor Blau
2020-06-30 19:18   ` Jeff King
2020-06-30 19:27     ` Taylor Blau
2020-06-30 19:33       ` Jeff King
2020-08-03 19:02 ` [PATCH 0/3] commit-graph: introduce 'core.useBloomFilters' Taylor Blau

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200630185536.GA1888406@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=dstolee@microsoft.com \
    --cc=git@vger.kernel.org \
    --cc=me@ttaylorr.com \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Git Mailing List Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/git/0 git/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 git git/ https://lore.kernel.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.git


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git