Re: [PATCH v2 2/7] string-list: align string_list_split() with its _in_place() counterpart

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 1, 2025, 03:43 UTC
Message-ID: <xmqqikj7ogf7.fsf@gitster.g>
In-Reply-To: <aIwndQtEoKNXRG5z@mbp>

shejialuo <shejialuo@gmail.com> writes:

Show 14 quoted lines

> On Thu, Jul 31, 2025 at 03:46:01PM -0700, Junio C Hamano wrote:
>> diff --git a/setup.c b/setup.c
>> index 6f52dab64c..b9f5eb8b51 100644
>> --- a/setup.c
>> +++ b/setup.c
>> @@ -1460,8 +1460,9 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
>>  
>>  	if (env_ceiling_dirs) {
>>  		int empty_entry_found = 0;
>> +		static const char path_sep[] = { PATH_SEP, '\0' };
>>  
>
> I am a little confused why we need to use `static`? Would this function
> be called many times?

I actually am confused why you would want anything other than static here. Writing this way would allow the compiler to realize that the array can be prepared at compile time, without need to do anything at runtime. If you made it non static, the runtime code would allocate two bytes worth of memory on stack, and stuff these two byte values there, each time this block is entered, which would be at least once.

> And I have a design question: by using "PATH_SEP", we need to convert
> this character to be string. Should we create a new variable named
> "PATH_SEP_STR" or whatever to do that?

Sorry, but I do not understand the question. You want to see something like

	#define PATH_SEP_STR "/"

you mean?  I do not offhand see why anybody would want to do so.

Show 8 quoted lines

>> -		string_list_split(&ceiling_dirs, env_ceiling_dirs, PATH_SEP, -1);
>> +		string_list_split(&ceiling_dirs, env_ceiling_dirs, path_sep, -1);
>>  		filter_string_list(&ceiling_dirs, 0,
>>  				   canonicalize_ceiling_entry, &empty_entry_found);
>>  		ceil_offset = longest_ancestor_length(dir->buf, &ceiling_dirs);
>
> Thanks,
> Jialuo