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

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

Hi James,

I saw that you sent a v3, but did not see any of this information (which
took a good while to assemble, as you might have guessed) in the commit
message. However, I think that message would make for the best home for
this information:

On Fri, 3 Dec 2021, Johannes Schindelin wrote:

> 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.

Thanks,
Dscho