Re: [PATCH 2/4] Clarify return value ownership of real_path and read_gitfile_gently

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 26, 2017, 05:01 UTC
Message-ID: <xmqqmv5iaw8o.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170925155927.32328-3-hanwen@google.com>

Han-Wen Nienhuys <hanwen@google.com> writes:

>>Subject: Re: [PATCH 2/4] Clarify return value ownership of real_path and read_gitfile_gently

We try to make "shortlog --no-merges" output consistently say what area the change is about, followed by a colon, followed by a short description.

Show 20 quoted lines

> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> ---
>  abspath.c | 3 +++
>  setup.c   | 3 ++-
>  2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/abspath.c b/abspath.c
> index 708aff8d4..792a2d533 100644
> --- a/abspath.c
> +++ b/abspath.c
> @@ -202,6 +202,9 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,
>  	return retval;
>  }
>  
> +/* Resolve `path` into an absolute, cleaned-up path. The return value
> + * comes from a global shared buffer and should not be freed.
> + */
>  const char *real_path(const char *path)
>  {
>  	static struct strbuf realpath = STRBUF_INIT;

The part about "what it does" is a good thing to have here, but I do not think the second sentence adds much if it stays here (if the comment were in a header file far from implementation, then it is a different matter). Besides, "should not be freed" is not the only important thing---it is already implied by the function returning "const char *" (i.e. the caller/receiver does not own it). It will stay valid only until the next call, so the caller needs to xstrdup() it if it wants to keep the value. That is equally important.

But quite honestly, that pattern appears very often in our codebase (all users of get_pathname() share the same characteristics) that these details (i.e. do not free, expect the buffer to be recycled) probaly is not worth it. Mentioning that the returned value is in a shared buffer for short-term use would be sufficient.

Show 11 quoted lines

> diff --git a/setup.c b/setup.c
> index 6d8380acd..31853724c 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -541,7 +541,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.
> + * return path to git directory if found. The return value comes from
> + * a shared pool and should not be freed.

OK, the returned value comes from the return value of real_path(), so the early half of the new warning is worth having. "should not be freed" is both extraneous (for those who are already aware of the common pattern in our codebase) and insufficient (for those who are not yet).

Thanks.

>   *
>   * On failure, if return_error_code is not NULL, return_error_code
>   * will be set to an error code and NULL will be returned. If