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

From: shejialuo <shejialuo@gmail.com>
Date: Aug 1, 2025, 03:55 UTC
Message-ID: <aIw6ojvvDWLPllG7@mbp>
In-Reply-To: <xmqqikj7ogf7.fsf@gitster.g>

On Thu, Jul 31, 2025 at 08:43:24PM -0700, Junio C Hamano wrote:

Show 25 quoted lines

> shejialuo <shejialuo@gmail.com> writes:
> 
> > 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.
>

Sorry to make you confused. Because there are some other changes where you don't use `static`. That's the main reason why I ask this question.

--- a/t/helper/test-path-utils.c
+++ b/t/helper/test-path-utils.c
@@ -348,6 +348,7 @@ int cmd__path_utils(int argc, const char **argv)
 	if (argc == 4 && !strcmp(argv[1], "longest_ancestor_length")) {
 		int len;
 		struct string_list ceiling_dirs = STRING_LIST_INIT_DUP;
+		const char path_sep[] = { PATH_SEP, '\0' };
 		char *path = xstrdup(argv[2]);

> > 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.
> 

Yes, that's my question. Because I see that we would define `path_sep`
array in many place, so I wonder whether we could use such macro.