Re: [GSoC PATCH v2 1/4] path: introduce format_path() for centralized path formatting

[Justin Tobler <jltobler@gmail.com>]

On 26/06/05 10:00PM, K Jayatheerth wrote:
> The path-formatting logic inside `builtin/rev-parse.c` handles absolute,
> canonical, and relative formatting rules based on user-supplied options.
> However, this logic is tightly coupled to `rev-parse` and writes directly
> to stdout.
> 
> To allow other builtins (such as the upcoming `git repo` path keys) to
> re-use this logic, extract the core path-formatting algorithm into a centralized
> helper function, `format_path()`, in `path.c`.

Makes sense.

> Expose a single, streamlined `path_format` enum in `path.h` to let callers
> explicitly declare their formatting strategy (UNMODIFIED, RELATIVE,
> RELATIVE_IF_SHARED, or CANONICAL). This decouples the core algorithm from
> the localized fallback mechanics specific to `rev-parse`.

Ok, so rev-parse has its own logic to select the formatting strategy
used when printing paths that either relies on what the user provides or
a designated fallback format that is specific to the type of path. Since
that is specific to rev-parse, it makes to factor it out of the generic
helper function here.

> Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
> Mentored-by: Justin Tobler <jltobler@gmail.com>
> Mentored-by: Lucas Seiki Oshiro <lucasseikioshiro@gmail.com>
> ---
>  path.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  path.h | 30 ++++++++++++++++++++++++++++++
>  2 files changed, 88 insertions(+)
> 
> diff --git a/path.c b/path.c
> index d7e17bf174..2fcd24c5eb 100644
> --- a/path.c
> +++ b/path.c
> @@ -1579,6 +1579,64 @@ char *xdg_cache_home(const char *filename)
>  	return NULL;
>  }
>  
> +void format_path(struct strbuf *buf, const char *path,
> +		 const char *prefix, enum path_format format)
> +{
> +	if (format == PATH_FORMAT_UNMODIFIED) {
> +		strbuf_addstr(buf, path);
> +		return;
> +	}
> +
> +	if (format == PATH_FORMAT_RELATIVE) {

nit: we could just continue the "else if" chain here instead of
restarting it.

> +		struct strbuf relative_buf = STRBUF_INIT;
> +		struct strbuf real_path = STRBUF_INIT;
> +		struct strbuf real_prefix = STRBUF_INIT;
> +		char *cwd = NULL;
> +
> +		/*
> +		 * We don't ever produce a relative path if prefix is NULL,
> +		 * so set the prefix to the current directory so that we can
> +		 * produce a relative path whenever possible.
> +		 */
> +		if (!prefix)
> +			prefix = cwd = xgetcwd();
> +
> +		if (!is_absolute_path(path)) {
> +			strbuf_realpath_forgiving(&real_path, path, 1);
> +			path = real_path.buf;
> +		}
> +		if (!is_absolute_path(prefix)) {
> +			strbuf_realpath_forgiving(&real_prefix, prefix, 1);
> +			prefix = real_prefix.buf;
> +		}
> +
> +		strbuf_addstr(buf, relative_path(path, prefix, &relative_buf));
> +
> +		strbuf_release(&relative_buf);
> +		strbuf_release(&real_path);
> +		strbuf_release(&real_prefix);
> +		free(cwd);
> +	} else if (format == PATH_FORMAT_RELATIVE_IF_SHARED) {
> +		struct strbuf relative_buf = STRBUF_INIT;
> +
> +		/*
> +		 * If we're using RELATIVE_IF_SHARED mode, then we want an
> +		 * absolute path unless the two share a common prefix, so don't
> +		 * default the prefix to the current working directory. Doing so
> +		 * would cause a relative path to always be produced if possible.
> +		 */
> +		strbuf_addstr(buf, relative_path(path, prefix, &relative_buf));
> +		strbuf_release(&relative_buf);
> +	} else if (format == PATH_FORMAT_CANONICAL) {
> +		struct strbuf canonical_buf = STRBUF_INIT;
> +
> +		strbuf_realpath_forgiving(&canonical_buf, path, 1);
> +		strbuf_addbuf(buf, &canonical_buf);

Do we need `canonical_buf` here? Can we just add the path to `buf`
directly?

> +
> +		strbuf_release(&canonical_buf);
> +	}
> +}
> +
>  REPO_GIT_PATH_FUNC(squash_msg, "SQUASH_MSG")
>  REPO_GIT_PATH_FUNC(merge_msg, "MERGE_MSG")
>  REPO_GIT_PATH_FUNC(merge_rr, "MERGE_RR")
> diff --git a/path.h b/path.h
> index 0434ba5e07..a78e0fc141 100644
> --- a/path.h
> +++ b/path.h
> @@ -262,6 +262,36 @@ enum scld_error safe_create_leading_directories_no_share(char *path);
>  int safe_create_file_with_leading_directories(struct repository *repo,
>  					      const char *path);
>  
> +/**
> + * The formatting strategy to apply when writing a path into a buffer.
> + */
> +enum path_format {
> +	/* Output the path exactly as-is without any modifications. */
> +	PATH_FORMAT_UNMODIFIED,
> +
> +	/* Output a path relative to the provided directory prefix. */
> +	PATH_FORMAT_RELATIVE,
> +
> +	/* Output a relative path only if the path shares a root with the prefix. */
> +	PATH_FORMAT_RELATIVE_IF_SHARED,
> +
> +	/* Output a fully resolved, absolute canonical path. */
> +	PATH_FORMAT_CANONICAL
> +};
> +
> +/**
> + * Format a path according to the specified formatting strategy and append
> + * the result to the given strbuf.
> + *
> + * `buf`    : The string buffer to append the formatted path to.
> + * `path`   : The path string that needs to be formatted.
> + * `prefix` : The directory prefix to calculate relative offsets against.
> + * Pass NULL to default to the current working directory where applicable.
> + * `format` : The formatting behavior rule to execute.
> + */
> +void format_path(struct strbuf *buf, const char *path,
> +		 const char *prefix, enum path_format format);
> +

Ok so in this patch we are just adding the new path formatting
interface and will integrate it in the next one. Overall the direction
of this patch looks good to me.

-Justin