Re: [PATCH 6/8] t/perf: add Scalar performance tests

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Sep 2, 2022, 09:16 UTC
Message-ID: <p9221sp1-r3on-ro35-3535-q423r641opn9@tzk.qr>
In-Reply-To: <86b7b9a3-8950-da9c-eae1-19ddb22456ec@github.com>

Hi Victoria,

On Thu, 1 Sep 2022, Victoria Dye wrote:

Show 55 quoted lines

> 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