Re: [PATCH v2] submodule-config: fix reading submodule.fetchJobs

[Junio C Hamano <gitster@pobox.com>]

Pablo <pabloosabaterr@gmail.com> writes:

> El dom, 3 may 2026 a las 13:57, Saagar Jha via GitGitGadget
> (<gitgitgadget@gmail.com>) escribió:
>>
>> From: Saagar Jha <saagar@saagarjha.com>
>>
>> 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

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.

>> +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
'