Re: [RFC PATCH 1/3] Make read_gitfile and resolve_gitfile thread safe

["Jean-Noël Avila" <avila.jn@gmail.com>]

Le 05/03/2024 à 02:21, Atneya Nair a écrit :

<snip>

> @@ -830,7 +833,8 @@ void read_gitfile_error_die(int error_code, const char *path, const char *dir)
>  
>  /*
>   * Try to read the location of the git directory from the .git file,
> - * return path to git directory if found. The return value comes from
> + * return path to git directory if found. If passed a valid strbuf, the return
> + * value is is a ptr to within the buffer. If strbuf is null, the return value comes from
>   * a shared buffer.
>   *
>   * On failure, if return_error_code is not NULL, return_error_code
> @@ -838,7 +842,7 @@ void read_gitfile_error_die(int error_code, const char *path, const char *dir)
>   * return_error_code is NULL the function will die instead (for most
>   * cases).
>   */
> -const char *read_gitfile_gently(const char *path, int *return_error_code)
> +const char *read_gitfile_gently(const char *path, int *return_error_code, struct strbuf* result_buf)
>  {
>  	const int max_file_size = 1 << 20;  /* 1MB */
>  	int error_code = 0;
> @@ -848,7 +852,10 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)
>  	struct stat st;
>  	int fd;
>  	ssize_t len;
> -	static struct strbuf realpath = STRBUF_INIT;
> +	static struct strbuf shared = STRBUF_INIT;
> +	if (!result_buf) {
> +		result_buf = &shared;
> +	}
>  

Question about general style: is it accepted practice to override a
parameter of a function?

I would have written:


@@ -838,7 +842,7 @@ void read_gitfile_error_die(int error_code, const
char *path, const char *dir)
   * return_error_code is NULL the function will die instead (for most
   * cases).
   */
-const char *read_gitfile_gently(const char *path, int *return_error_code)
+const char *read_gitfile_gently(const char *path, int
*return_error_code, struct strbuf* input_buf)
 {
 	const int max_file_size = 1 << 20;  /* 1MB */
 	int error_code = 0;
@@ -848,7 +852,10 @@ const char *read_gitfile_gently(const char *path,
int *return_error_code)
 	struct stat st;
 	int fd;
 	ssize_t len;
-	static struct strbuf realpath = STRBUF_INIT;
+	static struct strbuf shared = STRBUF_INIT;
+       struct strbuf* result_buf;
+	if (!input_buf) {
+		result_buf = &shared;
+	} else  {
+		result_buf = input_buf;
+	}


>  	if (stat(path, &st)) {
>  		/* NEEDSWORK: discern between ENOENT vs other errors */
> @@ -900,8 +907,10 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)
>  		goto cleanup_return;
>  	}
>  
> -	strbuf_realpath(&realpath, dir, 1);
> -	path = realpath.buf;
> +	strbuf_realpath(result_buf, dir, 1);
> +	path = result_buf->buf;
> +	// TODO is this valid?
> +	if (!path) die(_("Unexpected null from realpath '%s'"), dir);

In fact, this is not a null path, but an empty path (null is not part of
the string).
By the way, shouldn't this be an internal bug instead of a message to
the user?


Thanks