subtree: fix argument handling in check_parents

[jamesl-dm]

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.

Sorry Dscho - it wasn't 100% clear to me which details were required. I've rerolled and tried again.
Also sorry if I'm not replying to the mail correctly - I'm not actually subscribed to the list, and this seems like the only easy way to get text onto it through gitgitgadget without fighting Outlook.

cc: Johannes Schindelin Johannes.Schindelin@gmx.de

[gitgitgadget]

Welcome to GitGitGadget

Hi @jamesl-dm, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests.

Please make sure that your Pull Request has a good description, as it will be used as cover letter.

Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:

• the lines should not exceed 76 columns,
• the first line should be like a header and typically start with a prefix like "tests:" or "revisions:" to state which subsystem the change is about, and
• the commit messages' body should be describing the "why?" of the change.
• Finally, the commit messages should end in a Signed-off-by: line matching the commits' author.

It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code.

Contributing the patches

Before you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form /allow. A good way to find other contributors is to locate recent pull requests where someone has been /allowed:

• Search: is:pr is:open "/allow"

Both the person who commented /allow and the PR author are able to /allow you.

An alternative is the channel #git-devel on the Libera Chat IRC network:

<newcontributor> I've just created my first PR, could someone please /allow me? https://github.com/gitgitgadget/git/pull/12345
<veteran> newcontributor: it is done
<newcontributor> thanks!

Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment /submit.

If you want to see what email(s) would be sent for a /submit request, add a PR comment /preview to have the email(s) sent to you. You must have a public GitHub email address for this.

After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail).

If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the (raw) link), then import it into your mail program. If you use GMail, you can do this via:

curl -g --user "<EMailAddress>:<Password>" \
    --url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txt

To iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description):

Changes since v1:
- Fixed a typo in the commit message (found by ...)
- Added a code comment to ... as suggested by ...
...

To send a new iteration, just add another PR comment with the contents: /submit.

Need help?

New contributors who want advice are encouraged to join git-mentoring@googlegroups.com, where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join.

You may also be able to find help in real time in the developer IRC channel, #git-devel on Libera Chat. Remember that IRC does not support offline messaging, so if you send someone a private message and log out, they cannot respond to you. The scrollback of #git-devel is archived, though.

[sgn]

/allow

[gitgitgadget]

User jamesl-dm is now allowed to use GitGitGadget.

WARNING: jamesl-dm has no public email address set on GitHub

[dscho]

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

@jamesl-dm before you /submit, please edit this first comment, as it will be sent as a cover letter (and therefore does not need to repeat the commit message).

[jamesl-dm]

/submit

[gitgitgadget]

Error: Could not determine full name of jamesl-dm

[jamesl-dm]

/submit

[gitgitgadget]

Submitted as pull.1086.git.1638324413653.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-1086/jamesl-dm/maint-v1

To fetch this version to local tag pr-1086/jamesl-dm/maint-v1:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-1086/jamesl-dm/maint-v1

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"James Limbouris via GitGitGadget" <gitgitgadget@gmail.com> writes:

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

I do not do "git subtree", and this cannot really be a proper review
that is more than "Looks OK from a cursory look", but anyway...

It seems that 315a84f9 (subtree: use commits before rejoins for
splits, 2018-09-28) is what broke the logic, but it does not look
like a coding style clean-up to me.

> 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 $?

We know at this point each of $1, $2, etc. have exactly one
revision, and we want cache_miss function to take one revision per
its parameter, so writing "$@" is much more preferrable over $* even
though they do the same thing in practice in the context of this
code, I think.

>  	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

[jamesl-dm]

/submit

[gitgitgadget]

Submitted as pull.1086.v2.git.1638424290015.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-1086/jamesl-dm/maint-v2

To fetch this version to local tag pr-1086/jamesl-dm/maint-v2:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-1086/jamesl-dm/maint-v2

[gitgitgadget]

This branch is now known as jl/subtree-check-parents-argument-passing-fix.

[gitgitgadget]

This patch series was integrated into seen via git@e898644.

[gitgitgadget]

This patch series was integrated into seen via git@78120a8.

[gitgitgadget]

On the Git mailing list, Johannes Schindelin wrote (reply to this):

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
>

[gitgitgadget]

User Johannes Schindelin <Johannes.Schindelin@gmx.de> has been added to the cc: list.

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

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

I agree with this.  I'll mark the topic as "Expecting a reroll" in
the next "What's cooking" report, so that we do not forget that we
are waiting for such an improved description in the log message.

Thanks.

[gitgitgadget]

This patch series was integrated into seen via git@940b660.

[gitgitgadget]

There was a status update in the "New Topics" section about the branch jl/subtree-check-parents-argument-passing-fix on the Git mailing list:

Fix performance-releated bug in "git subtree" (in contrib/).

Expecting a reroll.
cf. <nycvar.QRO.7.76.6.2112031610590.63@tvgsbejvaqbjf.bet>
source: <pull.1086.v2.git.1638424290015.gitgitgadget@gmail.com>

[gitgitgadget]

This patch series was integrated into seen via git@38b5295.

[jamesl-dm]

/submit

[gitgitgadget]

Submitted as pull.1086.v3.git.1638758742741.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-1086/jamesl-dm/maint-v3

To fetch this version to local tag pr-1086/jamesl-dm/maint-v3:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-1086/jamesl-dm/maint-v3

[gitgitgadget]

This patch series was integrated into seen via git@840a2fc.

[gitgitgadget]

This patch series was integrated into seen via git@2155c06.

[gitgitgadget]

On the Git mailing list, Johannes Schindelin wrote (reply to this):

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

[gitgitgadget]

This patch series was integrated into seen via git@9ebf0ed.

[gitgitgadget]

There was a status update in the "Cooking" section about the branch jl/subtree-check-parents-argument-passing-fix on the Git mailing list:

Fix performance-releated bug in "git subtree" (in contrib/).

Expecting a reroll.
cf. <nycvar.QRO.7.76.6.2112072243310.90@tvgsbejvaqbjf.bet>
source: <pull.1086.v3.git.1638758742741.gitgitgadget@gmail.com>

[gitgitgadget]

This patch series was integrated into seen via git@ac6b39a.

[gitgitgadget]

This patch series was integrated into seen via git@6eb351d.

[gitgitgadget]

This patch series was integrated into seen via git@01969c0.

[gitgitgadget]

This patch series was integrated into seen via git@fa8bfa9.

[gitgitgadget]

This patch series was integrated into seen via git@8003e4b.

[gitgitgadget]

This patch series was integrated into seen via git@fe36f5c.

[gitgitgadget]

This patch series was integrated into seen via git@bfd5cf7.

[gitgitgadget]

There was a status update in the "Cooking" section about the branch jl/subtree-check-parents-argument-passing-fix on the Git mailing list:

Fix performance-releated bug in "git subtree" (in contrib/).

Expecting a reroll.
cf. <nycvar.QRO.7.76.6.2112072243310.90@tvgsbejvaqbjf.bet>
source: <pull.1086.v3.git.1638758742741.gitgitgadget@gmail.com>

[gitgitgadget]

This patch series was integrated into seen via git@7f53514.

[gitgitgadget]

This patch series was integrated into seen via git@1f64549.

[gitgitgadget]

This patch series was integrated into seen via git@f5dee21.

[gitgitgadget]

There was a status update in the "Cooking" section about the branch jl/subtree-check-parents-argument-passing-fix on the Git mailing list:

Fix performance-releated bug in "git subtree" (in contrib/).

Expecting a reroll.
cf. <nycvar.QRO.7.76.6.2112072243310.90@tvgsbejvaqbjf.bet>
source: <pull.1086.v3.git.1638758742741.gitgitgadget@gmail.com>

[gitgitgadget]

This patch series was integrated into seen via git@cde1690.

[gitgitgadget]

This patch series was integrated into seen via git@a150b26.

[gitgitgadget]

There was a status update in the "Cooking" section about the branch jl/subtree-check-parents-argument-passing-fix on the Git mailing list:

Fix performance-releated bug in "git subtree" (in contrib/).

Expecting a reroll.
cf. <nycvar.QRO.7.76.6.2112072243310.90@tvgsbejvaqbjf.bet>
source: <pull.1086.v3.git.1638758742741.gitgitgadget@gmail.com>

[gitgitgadget]

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi James,

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

> From: James Limbouris <james@digitalmatter.com>
>
> 315a84f9aa0 (subtree: use commits before rejoins for splits, 2018-09-28)
> changed the signature of check_parents from 'check_parents [REV...]'
> to 'check_parents PARENTS_EXPR INDENT'. In other words the variable list
> of parent revisions became a list embedded in a string. However it
> neglected to unpack the list again before sending it to cache_miss,
> leading to incorrect calls whenever more than one parent was present.
> This is the case whenever a merge commit is processed, with the end
> result being a loss of performance from unecessary rechecks.
>
> The indent parameter was subsequently removed in e9525a8a029 (subtree:
> have $indent actually affect indentation, 2021-04-27), but the argument
> handling bug remained.
>
> 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
>
>     > 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.
>
>     Sorry Dscho - it wasn't 100% clear to me which details were required.
>     I've rerolled and tried again. Also sorry if I'm not replying to the
>     mail correctly - I'm not actually subscribed to the list, and this seems
>     like the only easy way to get text onto it through gitgitgadget without
>     fighting Outlook.

It is not Outlook you're fighting. It is the decision by the majordomo of
the Git mailing list to drop @outlook.com and @hotmail.com mails. Because
who would use those email addresses, amirite?

In any case, thank you so much for sending the fixed commit, and sorry for
not reviewing it earlier. It looks good to me! With this commit message, I
think it is good to go.

Thanks,
Dscho

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> In any case, thank you so much for sending the fixed commit, and sorry for
> not reviewing it earlier. It looks good to me! With this commit message, I
> think it is good to go.

Thanks, both.  Will replace.

[gitgitgadget]

This patch series was integrated into seen via git@3c7bae4.

[gitgitgadget]

This patch series was integrated into seen via git@ae52f51.

[gitgitgadget]

This patch series was integrated into seen via git@a795af2.

[gitgitgadget]

This patch series was integrated into next via git@7dc995f.

[gitgitgadget]

There was a status update in the "Cooking" section about the branch jl/subtree-check-parents-argument-passing-fix on the Git mailing list:

Fix performance-releated bug in "git subtree" (in contrib/).

Will merge to 'master'.
source: <pull.1086.v4.git.1638929518657.gitgitgadget@gmail.com>

[gitgitgadget]

This patch series was integrated into seen via git@9b6eda0.

[gitgitgadget]

This patch series was integrated into next via git@9b6eda0.

[gitgitgadget]

This patch series was integrated into master via git@9b6eda0.

[gitgitgadget]

Closed via 9b6eda0.