Call checkout-index just once per file #424

Merged

wetneb merged 1 commit from xmo/mergiraf:optimise-checkout-index into main

2025-06-03 14:37:14 +02:00

xmo commented

2025-06-03 12:13:45 +02:00

Contributor

extract_revision(_from_git)? is always called 3 times (either directly or indirectly via extract_revision, doesn't really make a difference), one time to get the base version of a file, one time to get the left, and one time to get the right.

However this is a lot of unnecessary git interactions given Git can do that on its own by calling checkout-index --stage=all. It's a bit more complex to parse but not exactly heinous.

`extract_revision(_from_git)?` is always called 3 times (either directly or indirectly via `extract_revision`, doesn't really make a difference), one time to get the base version of a file, one time to get the left, and one time to get the right. However this is a lot of unnecessary git interactions given Git can do that on its own by calling `checkout-index --stage=all`. It's a bit more complex to parse but not exactly heinous.

xmo commented

2025-06-03 12:18:17 +02:00

Author

Contributor

This is a small optimisation on the git checkout-index calls to call that only once instead of thrice. de84f99c12 shows that --staged=all and --temp were added at the same time, and hints that --staged=all was very much added to speed up the case of resolving all three revisions:

Using git-unpack-file on each stage individually incurs a rather high penalty due to the need to fork for each file version obtained.

It's likely to conflict with #423 but the resolution should be straightforward?

This is a small optimisation on the git `checkout-index` calls to call that only once instead of thrice. https://github.com/git/git/commit/de84f99c12d1819479116685393afb1ebe99810b shows that `--staged=all` and `--temp` were added at the same time, and hints that `--staged=all` was very much added to speed up the case of resolving all three revisions: > Using git-unpack-file on each stage individually incurs a rather high penalty due to the need to fork for each file version obtained. It's likely to conflict with #423 but the resolution should be straightforward?

xmo commented

2025-06-03 12:19:02 +02:00

Author

Contributor

(~~and re-reading that commit, I now re-member that I need to handle the case of a . stage temp as a failure which I forgot to do~~ should be fixed)

(~~and re-reading that commit, I now re-member that I need to handle the case of a `.` stage temp as a failure which I forgot to do~~ should be fixed)

xmo force-pushed optimise-checkout-index from 147bad56a8 to 8819d1172c

2025-06-03 12:27:19 +02:00

Compare

wetneb approved these changes

2025-06-03 14:35:59 +02:00

wetneb left a comment

Owner

Hi @xmo, how nice! Thanks a lot for those contributions and welcome to the project :)

The optimization you propose here looks good to me. It's also covered by existing tests so I don't think we need more here.

In the future, I think it would be helpful to support returning only the left and right revisions, even though the base one is missing, as this is still a valid use case for Mergiraf (both sides add the same file with different contents). But your version is consistent with the existing behaviour of the tool, so I think it's worth keeping the scope of this PR to this optimization and tackle this improvement later.

Hi @xmo, how nice! Thanks a lot for those contributions and welcome to the project :) The optimization you propose here looks good to me. It's also covered by existing tests so I don't think we need more here. In the future, I think it would be helpful to support returning only the left and right revisions, even though the base one is missing, as this is still a valid use case for Mergiraf (both sides add the same file with different contents). But your version is consistent with the existing behaviour of the tool, so I think it's worth keeping the scope of this PR to this optimization and tackle this improvement later.