Re: [PATCH v5 11/14] core.fsyncmethod: tests for batch mode

[Neeraj Singh <nksingh85@gmail.com>]

On Wed, Mar 30, 2022 at 11:13 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "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.

> 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