Hi Victoria,
On Thu, 1 Sep 2022, Victoria Dye wrote:
> Victoria Dye wrote:
> > Johannes Schindelin wrote:
> >>
> >> On Wed, 31 Aug 2022, Victoria Dye via GitGitGadget wrote:
> >>
> >>> [...]
> >>> +
> >>> +test_compare_perf () {
> >>> + command="$@"
> >>> + test_perf "$command (scalar)" "
> >>> + (
> >>> + cd scalar-clone/src &&
> >>> + $command
> >>> + )
> >>> + "
> >>> +
> >>> + test_perf "$command (non-scalar)" "
> >>> + (
> >>> + cd git-clone &&
> >>> + $command
> >>> + )
> >>> + "
> >>> +}
> >>> +
> >>> +test_compare_perf git status
> >>> +test_compare_perf test_commit --append --no-tag A
> >>
> >> Given the small numbers presented in the commit message, I suspect that
> >> even so much as running the command in a subshell might skew the timings
> >> at least on Windows, where subshells are very, very expensive.
> >>
> >> Given that both `git` and `test_commit` understand the `-C <directory>`
> >> syntax, this variant would resolve my concern:
> >>
> >> test_compare_perf () {
> >> command=$1
> >> shift
> >> args="$*"
> >>
> >> test_perf "$command (scalar)" "
> >> $command -C scalar-clone/src $args
> >> "
> >>
> >> test_perf "$command (non-scalar)" "
> >> $command -C git-clone $args
> >> "
> >> }
> >>
> >> What do you think?
> >
> > Makes sense to me! Although, out of curiosity, is there a reason you prefer
> > "$1 -> shift -> $*" over '$1' and '$@'?
>
> Whoops, I completely misread your snippet; the 'shift' is necessary to
> separate the '$command' out so that we can inject '-C'.
Yes, and I also changed the "$@" (which would usually expand to a
parameter list, except when it is used inside a string, in which case it
behaves like $* for convenience) because the "$*" conveys more correctly
what we do here.
Whenever I read "$@" anywhere, my mind puts a mental check mark behind the
"is this code safe with regards to spaces in arguments?" question.
However, this function would mishandle arguments that contain spaces, and
reading "$*" makes me aware of that, so that I can avoid passing such
arguments.
So for me, using $* here is the right thing to do: It makes it less likely
that someone like me adds code in the future that assumes that even
arguments with spaces in them would be handled.
Thanks,
Dscho
