From: Adrian Ratiu <adrian.ratiu@collabora.com>
Date: Tue, 07 Oct 2025 09:23:43 GMT
Subject: Re: [PATCH v3 1/5] submodule--helper: use submodule_name_to_gitdir in add_submodule
Message-ID: <87plaz3w0w.fsf@collabora.com>
In-Reply-To: <xmqqh5wcq94v.fsf@gitster.g>

On Mon, 06 Oct 2025, Junio C Hamano <gitster@pobox.com> wrote:
> Adrian Ratiu <adrian.ratiu@collabora.com> writes: 
> 
>> While testing submodule gitdir path encoding, I noticed 
>> submodule--helper is still using a hardcoded name-based path 
>> leading to test failures, so convert it to the common helper 
>> function introduced by commit ce125d431a (submodule: extract 
>> path to submodule gitdir func, 2021-09-15)  and used in other 
>> locations across the source tree. 
> 
> OK.  To me during my first reading, the above read as if you 
> found an open coded logic here in add_submodule(), made it into 
> a new common helper function, and made this part as well as 
> other locations call that new common helper function.  Of course 
> that is not the case. 
> 
> Perhaps replacing everything after ", so convert it" with 
> something simpler like 
> 
>     ... to test failures.  Call submodule_name_to_gitdir() 
>     helper instead, which was invented exactly for this purpose 
>     and everybody else uses.  
> might have helped me to avoid such a confusion.  I dunno. 

Ack, I'll reword as you suggested to make it clearer.

 
>> diff --git a/builtin/submodule--helper.c 
>> b/builtin/submodule--helper.c index fcd73abe53..2873b2780e 
>> 100644 --- a/builtin/submodule--helper.c +++ 
>> b/builtin/submodule--helper.c @@ -3187,13 +3187,13 @@ static 
>> void append_fetch_remotes(struct strbuf *msg, const char 
>> *git_dir_path) 
>>   static int add_submodule(const struct add_data *add_data) { 
>> -	char *submod_gitdir_path; 
>>  	struct module_clone_data clone_data = 
>>  MODULE_CLONE_DATA_INIT; struct string_list reference = 
>>  STRING_LIST_INIT_NODUP; int ret = -1;  /* perhaps the path 
>>  already exists and is already a git repo, else clone it */ if 
>>  (is_directory(add_data->sm_path)) { 
>> +		char *submod_gitdir_path; 
> 
> This hunk is not related to the theme of the change and not 
> explained?  I think the variable becomes used only within this 
> block after the patch that loses the use of it on the "else" 
> side, so in that sense it is not strictly unrelated, but is a 
> fallout of this change.  If we were to mention the change in the 
> log message, something like "Also narrow the scope of a variable 
> that is no longer used in the updated code" would suffice.

Your understanding is correct, yes. I'll add it to the log msg.