submodule-config: fix reading submodule.fetchJobs

PATCH Superseded Saagar Jha via GitGitGadget 4 participants 9 messages May 3 – May 10

lore

Share & tools

lore

Saagar Jha via GitGitGadget

author

2026-05-03 10:14 UTC #0 lore raw

PATCH 1/1 submodule-config: fix reading submodule.fetchJobs

From: Saagar Jha <[email protected]>

Signed-off-by Saagar Jha <[email protected]>

---
submodule-config: fix reading submodule.fetchJobs

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2287%2Fsaagarjha%2Fmaint-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2287/saagarjha/maint-v1
Pull-Request: https://github.com/git/git/pull/2287

 submodule-config.c          |  2 +-
 t/t7406-submodule-update.sh | 10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

submodule-config.c +1 -1

index 1f19fe2077..57b190678e 100644
@@ -1037,5 +1037,5 @@ static int gitmodules_update_clone_config(const char *var, const char *value,
 
 void update_clone_config_from_gitmodules(int *max_jobs)
 {
-	config_from_gitmodules(gitmodules_update_clone_config, the_repository, &max_jobs);
+	config_from_gitmodules(gitmodules_update_clone_config, the_repository, max_jobs);
 }

t/t7406-submodule-update.sh +10 -0

index 3adab12091..234a021fb3 100755
@@ -1055,6 +1055,16 @@ test_expect_success 'submodule update can be run in parallel' '
 	)
 '
 
+test_expect_success 'submodule update honors fetch jobs config from .gitmodules' '
+	test_when_finished "rm -rf super3" &&
+	git clone cloned super3 &&
+	(cd super3 &&
+	 git config -f .gitmodules submodule.fetchJobs 67 &&
+	 GIT_TRACE="$(pwd)/trace.out" git submodule update --init &&
+	 grep "67 tasks" trace.out
+	)
+'
+
 test_expect_success 'git clone passes the parallel jobs config on to submodules' '
 	test_when_finished "rm -rf super4" &&
 	GIT_TRACE=$(pwd)/trace.out git clone --recurse-submodules --jobs 7 . super4 &&

base-commit: 67ad42147a7acc2af6074753ebd03d904476118f

Signature · 1 line

gitgitgadget

Pablo

2026-05-03 11:07 UTC #1 lore raw

Quoted 48 lines · Saagar Jha via GitGitGadget

From: Saagar Jha <[email protected]>

Signed-off-by Saagar Jha <[email protected]>

---
submodule-config: fix reading submodule.fetchJobs

 submodule-config.c          |  2 +-
 t/t7406-submodule-update.sh | 10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

submodule-config.c +1 -1

index 1f19fe2077..57b190678e 100644
@@ -1037,5 +1037,5 @@ static int gitmodules_update_clone_config(const char *var, const char *value,
 
 void update_clone_config_from_gitmodules(int *max_jobs)
 {
-       config_from_gitmodules(gitmodules_update_clone_config, the_repository, &max_jobs);
+       config_from_gitmodules(gitmodules_update_clone_config, the_repository, max_jobs);
 }

t/t7406-submodule-update.sh +10 -0

index 3adab12091..234a021fb3 100755
@@ -1055,6 +1055,16 @@ test_expect_success 'submodule update can be run in parallel' '
        )
 '
 
+test_expect_success 'submodule update honors fetch jobs config from .gitmodules' '
+       test_when_finished "rm -rf super3" &&
+       git clone cloned super3 &&
+       (cd super3 &&
+        git config -f .gitmodules submodule.fetchJobs 67 &&
+        GIT_TRACE="$(pwd)/trace.out" git submodule update --init &&
+        grep "67 tasks" trace.out
+       )
+'
+
 test_expect_success 'git clone passes the parallel jobs config on to submodules' '
        test_when_finished "rm -rf super4" &&
        GIT_TRACE=$(pwd)/trace.out git clone --recurse-submodules --jobs 7 . super4 &&

Quoted 4 lines

base-commit: 67ad42147a7acc2af6074753ebd03d904476118f --
gitgitgadget

Hi Saagar!

A few hints before this can be reviewable:
- There is no commit body which, not only is a must, it makes it very hard reviewing because I cannot know what you wanted to do and I have to imagine it.
- CI tests seem to be failing, but the file that reports fail doesn't seem to be related with your changes.

About the code, I can understand the 'why': the function above 'fetch_config_from_gitmodules' builds an struct inside and sends the address &config, 'update_clone_config_from_gitmodules' calls the same function with &max_jobs but the error would be that max_jobs is already a pointer so there would be no need to pass by reference max_jobs.

But this kind of explanation is what should be on the commit body. Maybe not this technical as the code can be easily seen, but a high level explanation about what this is.

Looking forward for a v2,
--
Pablo

Saagar Jha

2026-05-03 11:24 UTC #2 lore raw

Hi Pablo! I figured the change was simple enough to be self-explanatory, but I expanded the description to indicate what the bug was. Let me know if there’s anything else you’d like me to fix (I agree that the CI failure is probably not related to my change, but I can try my hand at looking at it after if nobody has?)

Quoted 80 lines · Pablo

Quoted 45 lines · Saagar Jha via GitGitGadget
From: Saagar Jha <[email protected]>
Signed-off-by Saagar Jha <[email protected]>
---
submodule-config: fix reading submodule.fetchJobs
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2287%2Fsaagarjha%2Fmaint-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2287/saagarjha/maint-v1
Pull-Request: https://github.com/git/git/pull/2287
submodule-config.c | 2 +-
t/t7406-submodule-update.sh | 10 ++++++++++
2 files changed, 11 insertions(+), 1 deletion(-)
submodule-config.c +1 -1
index 1f19fe2077..57b190678e 100644
@@ -1037,5 +1037,5 @@ static int gitmodules_update_clone_config(const char *var, const char *value,
 
 void update_clone_config_from_gitmodules(int *max_jobs)
 {
-       config_from_gitmodules(gitmodules_update_clone_config, the_repository, &max_jobs);
+       config_from_gitmodules(gitmodules_update_clone_config, the_repository, max_jobs);
 }
t/t7406-submodule-update.sh +0 -0
index 3adab12091..234a021fb3 100755
@@ -1055,6 +1055,16 @@ test_expect_success 'submodule update can be run in parallel' '
       )
'
t/t7406-submodule-update.sh +10 -0
+test_expect_success 'submodule update honors fetch jobs config from .gitmodules' '
+       test_when_finished "rm -rf super3" &&
+       git clone cloned super3 &&
+       (cd super3 &&
+        git config -f .gitmodules submodule.fetchJobs 67 &&
+        GIT_TRACE="$(pwd)/trace.out" git submodule update --init &&
+        grep "67 tasks" trace.out
+       )
+'
+
test_expect_success 'git clone passes the parallel jobs config on to submodules' '
Quoted 7 lines
       test_when_finished "rm -rf super4" &&
       GIT_TRACE=$(pwd)/trace.out git clone --recurse-submodules --jobs 7 . super4 &&
base-commit: 67ad42147a7acc2af6074753ebd03d904476118f --
gitgitgadget
Hi Saagar!
A few hints before this can be reviewable:
- There is no commit body which, not only is a must, it makes it very hard reviewing because I cannot know what you wanted to do and I have to imagine it.
- CI tests seem to be failing, but the file that reports fail doesn't seem to be related with your changes.
About the code, I can understand the 'why': the function above 'fetch_config_from_gitmodules' builds an struct inside and sends the address &config, 'update_clone_config_from_gitmodules' calls the same function with &max_jobs but the error would be that max_jobs is already a pointer so there would be no need to pass by reference max_jobs.
But this kind of explanation is what should be on the commit body. Maybe not this technical as the code can be easily seen, but a high level explanation about what this is.
Looking forward for a v2,
--
Pablo

Pablo

2026-05-03 12:19 UTC #3 lore raw

Quoted 2 lines · Saagar Jha

Hi Pablo! I figured the change was simple enough to be self-explanatory, but I expanded the description to indicate what the bug was. Let me know if there’s anything else you’d like me to fix (I agree that the CI failure is probably not related to my change, but I can try my hand at looking at it after if nobody has?)

No, by doing a quick search you find:
https://lore.kernel.org/git/[email protected]/

I was noting it so other reviewers know that your patch has nothing to do with the CI failing.
Nit: Try to have 80 columns at most per line so the lines doesn't get too long

--
Pablo

Saagar Jha via GitGitGadget

author

2026-05-03 11:57 UTC #4 lore raw

PATCH v2 1/1 submodule-config: fix reading submodule.fetchJobs

In reply to Saagar Jha via GitGitGadget

From: Saagar Jha <[email protected]>

The old code accidentally passed &max_jobs rather than max_jobs into config_from_gitmodules, which caused the setting to be written to the wrong place and dropped.

Signed-off-by Saagar Jha <[email protected]>

---
submodule-config: fix reading submodule.fetchJobs

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2287%2Fsaagarjha%2Fmaint-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2287/saagarjha/maint-v2
Pull-Request: https://github.com/git/git/pull/2287

Range-diff vs v1:

1: 094c641227 ! 1: 868901f1a6 submodule-config: fix reading submodule.fetchJobs
@@ Metadata
## Commit message ##
submodule-config: fix reading submodule.fetchJobs

+ The old code accidentally passed &max_jobs rather than max_jobs into
+ config_from_gitmodules, which caused the setting to be written to the
+ wrong place and dropped.
+

Signed-off-by Saagar Jha <[email protected]>

## submodule-config.c ##

 submodule-config.c          |  2 +-
 t/t7406-submodule-update.sh | 10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

submodule-config.c +1 -1

index 1f19fe2077..57b190678e 100644
@@ -1037,5 +1037,5 @@ static int gitmodules_update_clone_config(const char *var, const char *value,
 
 void update_clone_config_from_gitmodules(int *max_jobs)
 {
-	config_from_gitmodules(gitmodules_update_clone_config, the_repository, &max_jobs);
+	config_from_gitmodules(gitmodules_update_clone_config, the_repository, max_jobs);
 }

t/t7406-submodule-update.sh +10 -0

index 3adab12091..234a021fb3 100755
@@ -1055,6 +1055,16 @@ test_expect_success 'submodule update can be run in parallel' '
 	)
 '
 
+test_expect_success 'submodule update honors fetch jobs config from .gitmodules' '
+	test_when_finished "rm -rf super3" &&
+	git clone cloned super3 &&
+	(cd super3 &&
+	 git config -f .gitmodules submodule.fetchJobs 67 &&
+	 GIT_TRACE="$(pwd)/trace.out" git submodule update --init &&
+	 grep "67 tasks" trace.out
+	)
+'
+
 test_expect_success 'git clone passes the parallel jobs config on to submodules' '
 	test_when_finished "rm -rf super4" &&
 	GIT_TRACE=$(pwd)/trace.out git clone --recurse-submodules --jobs 7 . super4 &&

base-commit: 67ad42147a7acc2af6074753ebd03d904476118f

Signature · 1 line

gitgitgadget

Pablo

2026-05-03 13:07 UTC #5 lore raw

Quoted 6 lines · Saagar Jha via GitGitGadget

From: Saagar Jha <[email protected]>
The old code accidentally passed &max_jobs rather than max_jobs into config_from_gitmodules, which caused the setting to be written to the wrong place and dropped.

Better, but we can improve this, following Documentation/SubmittingPatches [1], it is better to write in present tense something like:

update_clone_config_from_gitmodules() passes &max_jobs to config_from_gitmodules(), but max_jobs is already a pointer. This causes the config value to be written to the wrong address and dropped.

Pass max_jobs directly.

I recommend reading Documentation/SubmittingPatches:
[1]: https://github.com/git/git/blob/master/Documentation/SubmittingPatches

Quoted 65 lines

Signed-off-by Saagar Jha <[email protected]>
---
submodule-config: fix reading submodule.fetchJobs
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2287%2Fsaagarjha%2Fmaint-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2287/saagarjha/maint-v2
Pull-Request: https://github.com/git/git/pull/2287
Range-diff vs v1:
1: 094c641227 ! 1: 868901f1a6 submodule-config: fix reading submodule.fetchJobs
@@ Metadata
## Commit message ##
submodule-config: fix reading submodule.fetchJobs
+ The old code accidentally passed &max_jobs rather than max_jobs into
+ config_from_gitmodules, which caused the setting to be written to the
+ wrong place and dropped.
+
Signed-off-by Saagar Jha <[email protected]>
## submodule-config.c ##
 submodule-config.c          |  2 +-
 t/t7406-submodule-update.sh | 10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)
submodule-config.c +1 -1
index 1f19fe2077..57b190678e 100644
@@ -1037,5 +1037,5 @@ static int gitmodules_update_clone_config(const char *var, const char *value,
 
 void update_clone_config_from_gitmodules(int *max_jobs)
 {
-       config_from_gitmodules(gitmodules_update_clone_config, the_repository, &max_jobs);
+       config_from_gitmodules(gitmodules_update_clone_config, the_repository, max_jobs);
 }
t/t7406-submodule-update.sh +10 -0
index 3adab12091..234a021fb3 100755
@@ -1055,6 +1055,16 @@ test_expect_success 'submodule update can be run in parallel' '
        )
 '
 
+test_expect_success 'submodule update honors fetch jobs config from .gitmodules' '
+       test_when_finished "rm -rf super3" &&
+       git clone cloned super3 &&
+       (cd super3 &&
+        git config -f .gitmodules submodule.fetchJobs 67 &&
+        GIT_TRACE="$(pwd)/trace.out" git submodule update --init &&
+        grep "67 tasks" trace.out
+       )
+'
+
 test_expect_success 'git clone passes the parallel jobs config on to submodules' '
        test_when_finished "rm -rf super4" &&
        GIT_TRACE=$(pwd)/trace.out git clone --recurse-submodules --jobs 7 . super4 &&
base-commit: 67ad42147a7acc2af6074753ebd03d904476118f --
gitgitgadget

Junio C Hamano

2026-05-10 00:01 UTC #6 lore raw

Quoted 18 lines · Pablo

Quoted 6 lines · Saagar Jha via GitGitGadget
From: Saagar Jha <[email protected]>
The old code accidentally passed &max_jobs rather than max_jobs into config_from_gitmodules, which caused the setting to be written to the wrong place and dropped.
Better, but we can improve this, following Documentation/SubmittingPatches [1], it is better to write in present tense something like:
update_clone_config_from_gitmodules() passes &max_jobs to config_from_gitmodules(), but max_jobs is already a pointer. This causes the config value to be written to the wrong address and dropped.

Quoted 4 lines

Pass max_jobs directly.
I recommend reading Documentation/SubmittingPatches:
[1]: https://github.com/git/git/blob/master/Documentation/SubmittingPatches

Very well said. And your version reads more natural in the context of this project, to those who are used to "git log --no-merges".

The config_from_gitmodules() function takes (void *) as the third parameter unfortunately, which forces the compiler to take any pointer without complaints. Typically an "int" is the same size as or narrower than a "(void *)", and has no stricter alignment requirement than "(int *)", so passing the address of max_int (i.e., "&max_int") would have caused the callee to assign an "int" result to the on-stack "max_int" variable, corrupting the pointer but otherwise causing no crashes, since the pointer is not used after the function returns. That is why we never noticed this at either compile time or runtime. Only those who paid attention to the actual return value, like the test added by this patch, would have noticed it. Nicely done.

Quoted 9 lines

t/t7406-submodule-update.sh test_expect_success 'submodule update can be run in parallel' ' · line 1066

Quoted 9 lines

t/t7406-submodule-update.sh +9 -0

+test_expect_success 'submodule update honors fetch jobs config from .gitmodules' '
+       test_when_finished "rm -rf super3" &&
+       git clone cloned super3 &&
+       (cd super3 &&
+        git config -f .gitmodules submodule.fetchJobs 67 &&
+        GIT_TRACE="$(pwd)/trace.out" git submodule update --init &&
+        grep "67 tasks" trace.out
+       )
+'

This inherited badness from existing tests in the same script, but in the modern tests, the abouve would be written more like

test_expect_success 'submodule update honors fetch jobs config from .gitmodules' '

	test_when_finished "rm -rf super3" &&
	git clone cloned super3 &&
	(
		cd super3 &&
		git config -f .gitmodules submodule.fetchJobs 67 &&
		GIT_TRACE="$(pwd)/trace.out" git submodule update --init &&
		test_grep "67 tasks" trace.out
	)

in order to

(1) open-close parentheses for subshell on their own lines;
(2) test_grep to make the error stand out more when it fails

or avoiding a subshell altogether and do

test_expect_success 'submodule update honors fetch jobs config from .gitmodules' '

	test_when_finished "rm -rf super3" &&
	git clone cloned super3 &&
	git -C super3 config -f .gitmodules submodule.fetchJobs 67 &&
	GIT_TRACE="$(pwd)/trace.out" git -C super3 submodule update --init &&
	test_grep "67 tasks" trace.out

Saagar Jha via GitGitGadget

author

2026-05-03 13:52 UTC #7 lore raw

PATCH v3 1/1 submodule-config: fix reading submodule.fetchJobs

In reply to Saagar Jha via GitGitGadget

From: Saagar Jha <[email protected]>

Pass max_jobs directly.

Signed-off-by Saagar Jha <[email protected]>

---
submodule-config: fix reading submodule.fetchJobs

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2287%2Fsaagarjha%2Fmaint-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2287/saagarjha/maint-v3
Pull-Request: https://github.com/git/git/pull/2287

Range-diff vs v2:

1: 868901f1a6 ! 1: 70fb2ede0f submodule-config: fix reading submodule.fetchJobs
@@ Metadata
## Commit message ##
submodule-config: fix reading submodule.fetchJobs

- The old code accidentally passed &max_jobs rather than max_jobs into
- config_from_gitmodules, which caused the setting to be written to the
- wrong place and dropped.
+ update_clone_config_from_gitmodules() passes &max_jobs to
+ config_from_gitmodules(), but max_jobs is already a pointer. This causes
+ the config value to be written to the wrong address and get dropped.
+
+ Pass max_jobs directly.

Signed-off-by Saagar Jha <[email protected]>

 submodule-config.c          |  2 +-
 t/t7406-submodule-update.sh | 10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

submodule-config.c +1 -1

index 1f19fe2077..57b190678e 100644
@@ -1037,5 +1037,5 @@ static int gitmodules_update_clone_config(const char *var, const char *value,
 
 void update_clone_config_from_gitmodules(int *max_jobs)
 {
-	config_from_gitmodules(gitmodules_update_clone_config, the_repository, &max_jobs);
+	config_from_gitmodules(gitmodules_update_clone_config, the_repository, max_jobs);
 }

t/t7406-submodule-update.sh +10 -0

index 3adab12091..234a021fb3 100755
@@ -1055,6 +1055,16 @@ test_expect_success 'submodule update can be run in parallel' '
 	)
 '
 
+test_expect_success 'submodule update honors fetch jobs config from .gitmodules' '
+	test_when_finished "rm -rf super3" &&
+	git clone cloned super3 &&
+	(cd super3 &&
+	 git config -f .gitmodules submodule.fetchJobs 67 &&
+	 GIT_TRACE="$(pwd)/trace.out" git submodule update --init &&
+	 grep "67 tasks" trace.out
+	)
+'
+
 test_expect_success 'git clone passes the parallel jobs config on to submodules' '
 	test_when_finished "rm -rf super4" &&
 	GIT_TRACE=$(pwd)/trace.out git clone --recurse-submodules --jobs 7 . super4 &&

base-commit: 67ad42147a7acc2af6074753ebd03d904476118f

Signature · 1 line

gitgitgadget

Saagar Jha via GitGitGadget

author

2026-05-10 03:50 UTC #8 lore raw

PATCH v4 1/1 submodule-config: fix reading submodule.fetchJobs

From: Saagar Jha <[email protected]>

Pass max_jobs directly.

Signed-off-by Saagar Jha <[email protected]>

---
submodule-config: fix reading submodule.fetchJobs

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2287%2Fsaagarjha%2Fmaint-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2287/saagarjha/maint-v4
Pull-Request: https://github.com/git/git/pull/2287

Range-diff vs v3:

1: 70fb2ede0f ! 1: 415c17f4bb submodule-config: fix reading submodule.fetchJobs

     @@ t/t7406-submodule-update.sh: test_expect_success 'submodule update can be run in
      +test_expect_success 'submodule update honors fetch jobs config from .gitmodules' '
      +	test_when_finished "rm -rf super3" &&
      +	git clone cloned super3 &&
     -+	(cd super3 &&
     -+	 git config -f .gitmodules submodule.fetchJobs 67 &&
     -+	 GIT_TRACE="$(pwd)/trace.out" git submodule update --init &&
     -+	 grep "67 tasks" trace.out
     -+	)
     ++	git -C super3 config -f .gitmodules submodule.fetchJobs 67 &&
     ++	GIT_TRACE="$(pwd)/trace.out" git -C super3 submodule update --init &&
     ++	test_grep "67 tasks" trace.out
      +'
      +
       test_expect_success 'git clone passes the parallel jobs config on to submodules' '

 submodule-config.c          | 2 +-
 t/t7406-submodule-update.sh | 8 ++++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

submodule-config.c +1 -1

index 1f19fe2077..57b190678e 100644
@@ -1037,5 +1037,5 @@ static int gitmodules_update_clone_config(const char *var, const char *value,
 
 void update_clone_config_from_gitmodules(int *max_jobs)
 {
-	config_from_gitmodules(gitmodules_update_clone_config, the_repository, &max_jobs);
+	config_from_gitmodules(gitmodules_update_clone_config, the_repository, max_jobs);
 }

t/t7406-submodule-update.sh +8 -0

index 3adab12091..6abb00876a 100755
@@ -1055,6 +1055,14 @@ test_expect_success 'submodule update can be run in parallel' '
 	)
 '
 
+test_expect_success 'submodule update honors fetch jobs config from .gitmodules' '
+	test_when_finished "rm -rf super3" &&
+	git clone cloned super3 &&
+	git -C super3 config -f .gitmodules submodule.fetchJobs 67 &&
+	GIT_TRACE="$(pwd)/trace.out" git -C super3 submodule update --init &&
+	test_grep "67 tasks" trace.out
+'
+
 test_expect_success 'git clone passes the parallel jobs config on to submodules' '
 	test_when_finished "rm -rf super4" &&
 	GIT_TRACE=$(pwd)/trace.out git clone --recurse-submodules --jobs 7 . super4 &&

base-commit: 67ad42147a7acc2af6074753ebd03d904476118f

Signature · 1 line

gitgitgadget