Re: [PATCH v5 11/14] core.fsyncmethod: tests for batch mode
- From
- Neeraj Singh <nksingh85@gmail.com>
- Date
- Mar 31, 2022, 03:55 UTC
- Message-ID
- <CANQDOdfoDSzpWbmnyaPyi9PoftbS7P7=8Y+RS_Bm3t0ggXpLEg@mail.gmail.com>
- In-Reply-To
- <xmqqbkxn8g0r.fsf@gitster.g>
On Wed, Mar 30, 2022 at 11:13 AM Junio C Hamano <gitster@pobox.com> wrote:
Show 31 quoted lines
>
> "Neeraj Singh via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > diff --git a/t/lib-unique-files.sh b/t/lib-unique-files.sh
> > new file mode 100644
> > index 00000000000..34c01a65256
> > --- /dev/null
> > +++ b/t/lib-unique-files.sh
> > @@ -0,0 +1,34 @@
> > +# Helper to create files with unique contents
> > +
> > +# Create multiple files with unique contents within this test run. Takes the
> > +# number of directories, the number of files in each directory, and the base
> > +# directory.
> > +#
> > +# test_create_unique_files 2 3 my_dir -- Creates 2 directories with 3 files
> > +# each in my_dir, all with contents
> > +# different from previous invocations
> > +# of this command in this run.
> > +
> > +test_create_unique_files () {
> > + test "$#" -ne 3 && BUG "3 param"
> > +
> > + local dirs="$1" &&
> > + local files="$2" &&
> > + local basedir="$3" &&
> > + local counter=0 &&
>
> The cover letter mentioned that in this round, "local var=val" is
> avoided?
>Whoops.. I guess I wasn't looking widely enough. I noticed failures on my Ubuntu system that uses dash.
Show 37 quoted lines
> I've seen instances of local being "curious", e.g. > https://lore.kernel.org/git/20220125092419.cgtfw32nk2niazfk@carbon/ > and the discussion indicates that it may still be relevant. > > > + local i && > > + local j && > > + test_tick && > > + local basedata=$basedir$test_tick && > > + rm -rf "$basedir" && > > + for i in $(test_seq $dirs) > > + do > > + local dir=$basedir/dir$i && > > This, too. > > To summarize the findings from the thread is: > > - very old releases of /bin/dash that predates Git, like 0.3.8, > would not correctly handle assignment on "local" at all. It may > not matter to us. > > - semi-old /bin/dash 0.5.10, which is still in use, mishandled > 'local var=$val', but 'local var="$val"' is an acceptable > workaround for the bug. "git grep" tells us that we use this > form in our shell scripts quite a lot, so we may be OK. > > - /bin/dash 0.5.11, which was tagged mid 2020, and newer would glok > 'local var=$val' just fine even $val has $IFS whitespace in it. > > So, I'd say the safe practice we should adopt is to use "local" one > per variable, assignment to the variable on the same line of "local" > is OK, but unlike the regular assignment, older dash may mishandle > the right-hand-side unless it is quoted, i.e. > > local var='string literal' > local var="$variable interpolation" >
Thanks for updating me on the documentation. I think I'll go back to one-line assignments with the proper quotations and single variables, just to keep things short and consistent. I've been looking up everything as I go with these shell scripts, but there's a lot of subtlety. CMD batch files are even uglier, but I've had many years to learn the common practices there.
I'll ask a question on the perf test patch on a related shell topic.
Thanks, Neeraj