Re: [PATCH 1/2] ref-filter: apply --ignore-case to all sorting keys

From: Jeff King <peff@peff.net>
Date: May 4, 2020, 15:13 UTC
Message-ID: <20200504151351.GC11373@coredump.intra.peff.net>
In-Reply-To: <20200503114351.GA28680@danh.dev>

On Sun, May 03, 2020 at 06:44:02PM +0700, Danh Doan wrote:

Show 18 quoted lines

> On 2020-05-03 05:11:57-0400, Jeff King <peff@peff.net> wrote:
> > +test_expect_success 'for-each-ref --ignore-case works on multiple sort keys' '
> > +	# name refs numerically to avoid case-insensitive filesystem conflicts
> > +	nr=0 &&
> > +	for email in a A b B
> > +	do
> > +		for subject in a A b B
> > +		do
> > +			GIT_COMMITTER_EMAIL="$email@example.com" \
> > +			git tag -m "tag $subject" icase-$(printf %02d $nr) &&
> > +			nr=$((nr+1))||
> 
> The CodingGuidelines said we want to spell `$nr` instead of `nr`
> inside arithmetic expansion for dash older than 0.5.4
> 
> I'm not sure if we should go with just `$((nr+1))` or it's better to
> loosen our Guidelines. Since Debian Jessie (oldest supported Debian)
> ships 0.5.7. I don't know about other systems.

Hmm, somehow I didn't know about that rule. We have many cases already in the test suite and elsewhere (try grepping for '$(([a-z]', which isn't exhaustive but turns up many examples).

Maybe it's time to loosen the rule?

I've actually seen style guides suggesting to never use "$" there for a few reasons:

  - it's slightly cleaner to read (this is the recommendation and
    rationale in Google's shell style guide)

  - it's less surprising if you somehow end up with a non-number in your
    variable:

      $ foo=bar
      $ bar=41
      $ echo $((foo + 1))
      dash: 8: Illegal number: bar
      $ echo $(($foo + 1))
      42

    That's using dash. With bash, both produce the answer 42! Clearly
    this isn't something we should be doing either way, but I'd much
    rather see "illegal number" in some cases which would alert us that
    something confusing is going on.

-Peff