Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fetch: choose a sensible default with --jobs=0 again #1483

Closed
wants to merge 1 commit into from

Conversation

rimrul
Copy link

@rimrul rimrul commented Feb 20, 2023

Prior to 51243f9 (run-command API: don't fall back on online_cpus(), 2022-10-12) git fetch --multiple --jobs=0 would choose some default amount of jobs, similar to git -c fetch.parallel=0 fetch --multiple. While our documentation only ever promised that fetch.parallel would fall back to a "sensible default", it makes sense to do the same for --jobs. So fall back to online_cpus() and not BUG() out.

This was originally reported at https://lore.kernel.org/git/PSAP153MB03910458707331B64FA7460DCAA19@PSAP153MB0391.APCP153.PROD.OUTLOOK.COM/T/#u

cc: Drew Noakes drnoakes@microsoft.com
cc: Ævar Arnfjörð Bjarmason avarab@gmail.com

prior to 51243f9 (run-command API: don't fall back on online_cpus(),
2022-10-12) `git fetch --multiple --jobs=0` would choose some default amount
of jobs, similar to `git -c fetch.parallel=0 fetch --multiple`. While our
documentation only ever promised that `fetch.parallel` would fall back to a
"sensible default", it makes sense to do the same for `--jobs`. So fall back
to online_cpus() and not BUG() out.

This fixes git-for-windows#4302

Reported-by: Drew Noakes <drnoakes@microsoft.com>
Signed-off-by: Matthias Aßhauer <mha1993@live.de>
@rimrul
Copy link
Author

rimrul commented Feb 20, 2023

What's happening with win test (8) why is that job at almost 5h?

@rimrul
Copy link
Author

rimrul commented Feb 20, 2023

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 20, 2023

Submitted as pull.1483.git.1676928805555.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1483/rimrul/fix-fetch-jobs-0-v1

To fetch this version to local tag pr-1483/rimrul/fix-fetch-jobs-0-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1483/rimrul/fix-fetch-jobs-0-v1

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 21, 2023

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

"Matthias Aßhauer via GitGitGadget"  <gitgitgadget@gmail.com>
writes:

> From: =?UTF-8?q?Matthias=20A=C3=9Fhauer?= <mha1993@live.de>
>
> prior to 51243f9 (run-command API: don't fall back on online_cpus(),
> 2022-10-12) `git fetch --multiple --jobs=0` would choose some default amount
> of jobs, similar to `git -c fetch.parallel=0 fetch --multiple`. While our
> documentation only ever promised that `fetch.parallel` would fall back to a
> "sensible default", it makes sense to do the same for `--jobs`. So fall back
> to online_cpus() and not BUG() out.

Yup, the way I read 51243f9f (run-command API: don't fall back on
online_cpus(), 2022-10-12) is that it wanted to make it a best
practice for the callers of the API to be making an explicit choice
of scaling with online_cpus() or other metrics depending on their
needs.  The problematic commit does touch some fetch-related code
paths and make them call online_cpus() themselves, so we probably
are looking at an inadequate tests, and this is a fix that is in
line of the spirit, completing what 51243f9f started to do.

Thanks, will queue.

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 21, 2023

This branch is now known as ma/fetch-parallel-use-online-cpus.

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 21, 2023

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

@gitgitgadget gitgitgadget bot added the seen label Feb 21, 2023
@dscho
Copy link
Member

dscho commented Feb 21, 2023

What's happening with win test (8) why is that job at almost 5h?

@rimrul most likely a hang in the MSYS2 runtime, seeing as the push variant of the same job succeeded in 7 minutes. I started a re-run.

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 22, 2023

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

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 22, 2023

This patch series was integrated into next via git@756d379.

@gitgitgadget gitgitgadget bot added the next label Feb 22, 2023
@gitgitgadget
Copy link

gitgitgadget bot commented Feb 23, 2023

There was a status update in the "New Topics" section about the branch ma/fetch-parallel-use-online-cpus on the Git mailing list:

"git fetch --jobs=0" used to hit a BUG(), which has been corrected
to use the available CPUs.

Will merge to 'master'.
source: <pull.1483.git.1676928805555.gitgitgadget@gmail.com>

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 24, 2023

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

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 24, 2023

This patch series was integrated into seen via git@0ad6bdb.

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 24, 2023

There was a status update in the "Cooking" section about the branch ma/fetch-parallel-use-online-cpus on the Git mailing list:

"git fetch --jobs=0" used to hit a BUG(), which has been corrected
to use the available CPUs.

Will merge to 'master'.
source: <pull.1483.git.1676928805555.gitgitgadget@gmail.com>

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 24, 2023

This patch series was integrated into seen via git@72ab6df.

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 25, 2023

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

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 25, 2023

This patch series was integrated into master via git@d180cc2.

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 25, 2023

This patch series was integrated into next via git@d180cc2.

@gitgitgadget gitgitgadget bot added the master label Feb 25, 2023
@gitgitgadget gitgitgadget bot closed this Feb 25, 2023
@gitgitgadget
Copy link

gitgitgadget bot commented Feb 25, 2023

Closed via d180cc2.

@rimrul rimrul deleted the fix-fetch-jobs-0 branch February 25, 2023 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants