From: Phillip Wood Date: Tue, 23 Jun 2026 15:57:09 GMT Subject: Re: [GSoC Patch v7 1/3] path: extract append_formatted_path() and use in rev-parse Message-ID: <084ad4d0-d872-4c7f-94a8-ec2383c7a8ca@gmail.com> In-Reply-To: <20260621055534.46798-2-jayatheerthkulkarni2005@gmail.com> On 21/06/2026 06:55, K Jayatheerth wrote: > Path formatting logic in builtin/rev-parse.c writes directly to > stdout. Other builtins cannot reuse it. > > Extract this logic into append_formatted_path() in path.c and expose > a path_format enum in path.h. > > Convert rev-parse to use the new helper in the same step to validate > the API against existing tests and avoid introducing dead code. The new API looks good now, and so does the conversion of the existing code. I'm very happy with this version and don't have anything to add to Junio's comments Thanks Phillip > Mentored-by: Justin Tobler > Mentored-by: Lucas Seiki Oshiro > Signed-off-by: K Jayatheerth > --- > builtin/rev-parse.c | 73 ++++++++++++++++++--------------------------- > path.c | 69 ++++++++++++++++++++++++++++++++++++++++++ > path.h | 30 +++++++++++++++++++ > 3 files changed, 128 insertions(+), 44 deletions(-) > > diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c > index bb882678fe..6de01466db 100644 > --- a/builtin/rev-parse.c > +++ b/builtin/rev-parse.c > @@ -653,53 +653,38 @@ enum default_type { > DEFAULT_UNMODIFIED, > }; > > -static void print_path(const char *path, const char *prefix, enum format_type format, enum default_type def) > +static void print_path(const char *path, const char *prefix, > + enum format_type format, enum default_type def) > { > - 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 we're using RELATIVE_IF_SHARED mode, then > - * we want an absolute path unless the two share a common prefix, so don't > - * set it in that case, since doing so causes a relative path to always > - * be produced if possible. > - */ > - if (!prefix && (format != FORMAT_DEFAULT || def != DEFAULT_RELATIVE_IF_SHARED)) > - prefix = cwd = xgetcwd(); > - if (format == FORMAT_DEFAULT && def == DEFAULT_UNMODIFIED) { > - puts(path); > - } else if (format == FORMAT_RELATIVE || > - (format == FORMAT_DEFAULT && def == DEFAULT_RELATIVE)) { > - /* > - * In order for relative_path to work as expected, we need to > - * make sure that both paths are absolute paths. If we don't, > - * we can end up with an unexpected absolute path that the user > - * didn't want. > - */ > - struct strbuf buf = STRBUF_INIT, realbuf = STRBUF_INIT, prefixbuf = STRBUF_INIT; > - if (!is_absolute_path(path)) { > - strbuf_realpath_forgiving(&realbuf, path, 1); > - path = realbuf.buf; > - } > - if (!is_absolute_path(prefix)) { > - strbuf_realpath_forgiving(&prefixbuf, prefix, 1); > - prefix = prefixbuf.buf; > + struct strbuf sb = STRBUF_INIT; > + enum path_format fmt; > + > + if (format == FORMAT_RELATIVE) { > + fmt = PATH_FORMAT_RELATIVE; > + } else if (format == FORMAT_CANONICAL) { > + fmt = PATH_FORMAT_CANONICAL; > + } else /* FORMAT_DEFAULT */ { > + switch (def) { > + case DEFAULT_RELATIVE: > + fmt = PATH_FORMAT_RELATIVE; > + break; > + case DEFAULT_RELATIVE_IF_SHARED: > + fmt = PATH_FORMAT_RELATIVE_IF_SHARED; > + break; > + case DEFAULT_CANONICAL: > + fmt = PATH_FORMAT_CANONICAL; > + break; > + case DEFAULT_UNMODIFIED: > + default: > + fmt = PATH_FORMAT_UNMODIFIED; > + break; > } > - puts(relative_path(path, prefix, &buf)); > - strbuf_release(&buf); > - strbuf_release(&realbuf); > - strbuf_release(&prefixbuf); > - } else if (format == FORMAT_DEFAULT && def == DEFAULT_RELATIVE_IF_SHARED) { > - struct strbuf buf = STRBUF_INIT; > - puts(relative_path(path, prefix, &buf)); > - strbuf_release(&buf); > - } else { > - struct strbuf buf = STRBUF_INIT; > - strbuf_realpath_forgiving(&buf, path, 1); > - puts(buf.buf); > - strbuf_release(&buf); > } > - free(cwd); > + > + append_formatted_path(&sb, path, prefix, fmt); > + puts(sb.buf); > + > + strbuf_release(&sb); > } > > int cmd_rev_parse(int argc, > diff --git a/path.c b/path.c > index d7e17bf174..6d8e892ada 100644 > --- a/path.c > +++ b/path.c > @@ -1579,6 +1579,75 @@ char *xdg_cache_home(const char *filename) > return NULL; > } > > +void append_formatted_path(struct strbuf *dest, const char *path, > + const char *prefix, enum path_format format) > +{ > + switch (format) { > + case PATH_FORMAT_UNMODIFIED: > + strbuf_addstr(dest, path); > + break; > + > + case PATH_FORMAT_RELATIVE: { > + 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(dest, relative_path(path, prefix, &relative_buf)); > + > + strbuf_release(&relative_buf); > + strbuf_release(&real_path); > + strbuf_release(&real_prefix); > + free(cwd); > + break; > + } > + > + case 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(dest, relative_path(path, prefix, &relative_buf)); > + strbuf_release(&relative_buf); > + break; > + } > + > + case PATH_FORMAT_CANONICAL: { > + struct strbuf canonical_buf = STRBUF_INIT; > + > + strbuf_realpath_forgiving(&canonical_buf, path, 1); > + strbuf_addbuf(dest, &canonical_buf); > + > + strbuf_release(&canonical_buf); > + break; > + } > + > + default: > + BUG("unknown path_format value %d", format); > + } > +} > + > 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 4c2958a903..4d982a2c8e 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. > + * > + * `dest` : 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 append_formatted_path(struct strbuf *dest, const char *path, > + const char *prefix, enum path_format format); > + > # ifdef USE_THE_REPOSITORY_VARIABLE > # include "strbuf.h" > # include "repository.h"