Re: [PATCH] subtree: fix argument handling in check_parents

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi James,


On Wed, 1 Dec 2021, James Limbouris via GitGitGadget wrote:

> From: James Limbouris <james@digitalmatter.com>
>
> check_parents was taking all of its arguments as a single string,
> and erroneously passing them to cache_miss as a single string.
> cache_miss would then fail, and the spurious cache misses it produced
> would hurt performance.
>
> For consistency, take multiple arguments in check_parents,
> and pass all of them to cache_miss separately.
>
> Signed-off-by: James Limbouris <james@digitalmatter.com>
> ---
>     subtree: fix argument handling in check_parents
>
>     Hello git developers. Please consider this small patch that fixes a bug
>     introduced during a coding style cleanup of the subtree command. Changes
>     to the argument handling were causing check_parents to fail when more
>     than one parent was supplied, which led to a small loss of performance.

When I look through the commit history of `git-subtree.sh`, I see that the
change was introduced in 315a84f9aa0 (subtree: use commits before rejoins
for splits, 2018-09-28) (which was not really a coding style cleanup).

The change was actually not done right, if I read the commit correctly,
because it added a new parameter _to the end_, even if the
`check_parents()` function took an arbitrary number of parameters already.
And indeed, it changed the `"$@"` into a "$1", pretending that only one
parent would be passed.

Now, I do not really understand under what circumstances multiple parents
could be passed to `check_parents()`, but I think that it does not matter
whether you use `--format=%P` or `^@` (the former was changed to the
latter in 19ad68d95d6 (subtree: performance improvement for finding
unexpected parent commits, 2018-10-12)), you can always get an arbitrary
number of parents that way.

The natural thing, now, would be to move the added `indent` parameter to
the front of the parameter list, but I see that there was some cleanup in
e9525a8a029 (subtree: have $indent actually affect indentation,
2021-04-27) which _removed_ that `indent` parameter.

So I _think_ your change is correct, even if I would love to see an
easy-to-understand description of the scenario where more than one parents
might be checked.

Another thing I would like to see is probably even more important: rather
than using $*, we should use the original "$@" instead (with
double-quotes). It should not matter a lot right now because we know that
the parameters are the output of `git rev-parse "$rev^@"` (which provides
them as a list of full object IDs, i.e. containing no white-space except
to delimit the IDs), but it still the correct form to use "$@" instead.

Thanks,
Dscho

>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1086%2Fjamesl-dm%2Fmaint-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1086/jamesl-dm/maint-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1086
>
>  contrib/subtree/git-subtree.sh | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
> index 7f767b5c38f..56f24000c2c 100755
> --- a/contrib/subtree/git-subtree.sh
> +++ b/contrib/subtree/git-subtree.sh
> @@ -296,10 +296,9 @@ cache_miss () {
>  	done
>  }
>
> -# Usage: check_parents PARENTS_EXPR
> +# Usage: check_parents [REVS...]
>  check_parents () {
> -	assert test $# = 1
> -	missed=$(cache_miss "$1") || exit $?
> +	missed=$(cache_miss $*) || exit $?
>  	local indent=$(($indent + 1))
>  	for miss in $missed
>  	do
> @@ -753,7 +752,7 @@ process_split_commit () {
>  	fi
>  	createcount=$(($createcount + 1))
>  	debug "parents: $parents"
> -	check_parents "$parents"
> +	check_parents $parents
>  	newparents=$(cache_get $parents) || exit $?
>  	debug "newparents: $newparents"
>
>
> base-commit: e9d7761bb94f20acc98824275e317fa82436c25d
> --
> gitgitgadget
>