Re: [PATCH 1/6] test-lib: introduce test_commit_bulk

From: Jeff King <peff@peff.net>
Date: Jun 29, 2019, 00:25 UTC
Message-ID: <20190629002542.GF2625@sigill.intra.peff.net>
In-Reply-To: <CAPig+cQAKVOqYH-CcqzViP_zuKwg5fWO2_i8=Z5t=BVf9uoAXQ@mail.gmail.com>

On Fri, Jun 28, 2019 at 05:32:35PM -0400, Eric Sunshine wrote:

Show 10 quoted lines

> > +# The message, filename, and contents strings are evaluated by the shell inside
> > +# double-quotes, with $n set to the current commit number. So you can do:
> > +#
> > +#   test_commit_bulk --filename=file --contents='modification $n'
> 
> Considering that test_commit_bulk() is intended to be used within a
> test body, and considering that test bodies are almost always
> encapsulated in single quotes, recommending single quoting the value
> of --contents= seems contraindicated. Double quotes likely would be
> better.

I hoped people reading it would realize that they would need to suppress interpolation of $ one way or another. But I guess many people who touch the test suite aren't actually prolific shell writers.

Anyway, I'm going to look into changing this to a printf string, which would make this easier.

> > +       in_dir=${indir:+-C "$indir"}
> 
> Doesn't this suffer the problem in which some older/broken
> shells[1][2][3][4] incorrectly expand this to:

That line is leftover dead code; see my response to Junio.

> Same comment applies to other instances of ${indir:+-C "$indir"} below.

Those ones are fine because of the double-quotes (and whitespace splitting on the replacement value happens before interpolation). Try this:

  x=''
  y='with spaces'
  sh -c 'for i in "$@"; do echo arg: $i; done' -- \
    ${x:+-x "$x"} \
    ${y:+-y "$y"}

-Peff