From: K Jayatheerth Date: Mon, 22 Jun 2026 17:41:23 GMT Subject: Re: [GSoC Patch v7 1/3] path: extract append_formatted_path() and use in rev-parse Message-ID: In-Reply-To: Hey Junio, On Mon, Jun 22, 2026 at 2:32 AM Junio C Hamano wrote: > So, for the existing user of this logic, the preimage ... > > > -static void print_path(const char *path, const char *prefix, enum format_type format, enum default_type def) > > { ... > > - free(cwd); > > } > > ... now becomes this postimage. > Yes that's right! > > +static void print_path(const char *path, const char *prefix, > > + enum format_type format, enum default_type def) > > { > > + 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; > > } > > } > > + > > + append_formatted_path(&sb, path, prefix, fmt); > > + puts(sb.buf); > > + > > + strbuf_release(&sb); > > } > > Mostly, the code translates FORMAT_FOO constants into the new > PATH_FORMAT_FOO constants, and lets append_formatted_path() do the > heavy lifting. > > It is a minor point, but wouldn't it make it simpler to handle > format_default first? I.e., > > if (format == FORMAT_DEFAULT) > switch (def) { > case DEFAULT_RELATIVE: > format = DEFAULT_RELATIVE; > break; > ... > case DEFAULT_UNMODIFIED: > default: > format = DEFAULT_UNMODIFIED; > break; > } > switch (format) { > case FORMAT_RELATIVE: fmt = PATH_FORMAT_RELATIVE; break; > case FORMAT_CANONICAL: fmt = PATH_FORMAT_CANONICAL; break; > ... > } > > Perhaps yes, perhaps not. I dunno. > I see you have continued this point further I am going to respond to this in detail there. > > 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; > > In the orignal "print_path()", DEFAULT/UNMODIFIED did this "show > unmodified". OK. > > > + 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(); > > This is what was done in the original "print_path()" upfront, with > a similar comment to explay why this happens. Looking good. Also > we no longer call xgetcwd() when we do not need to, which is goodd. > > > + 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; > > + } > > There used to be a comment explaining why we make realpath calls, > which is now lost. Perhaps what the comment said was so obvious > that we are better off without it? I offhand do not know. > When the logic was a single block, the comment felt necessary to explain the flow. By splitting it into explicit switch cases, the logic became a bit more self-evident, so I removed it to reduce clutter. I kept the other comments where the reasoning is less obvious. > What is done to make the paths real is the same as before, which is > good. > > > + strbuf_addstr(dest, relative_path(path, prefix, &relative_buf)); > > + > > + strbuf_release(&relative_buf); > > + strbuf_release(&real_path); > > + strbuf_release(&real_prefix); > > + free(cwd); > > + break; > > + } > > OK. > > > + 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. > > + */ I thought this comment made sense keeping in for instance. > Identical to the original, which is good. > > + > > + 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); > > + } > > +} > > OK. > > > +/** > > + * 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); > > + > > It is slightly unsatisfying that this function is defined to > "append" to any existing value in the dest strbuf, rather than > storing the result in the dest strbuf. The original caller > print_path() passes an empty strbuf to this helper, so it can let > strbuf_realpath_*() functions to strbuf_reset() it (e.g., > abspath.c:get_root_part() called by strbuf_realpath_1(), wihch in > turn is called by strbuf_realpath() and strbuf_realpath_forgiving()) > it freely, which means that use of temporary strbuf like > canonical_buf only to copy it out to dest is wasteful and unneeded. > But other callers we will have for this helper later may want to > append to what they already have, so perhaps it is OK (on the other > hand, we could say that preserving and appending is what these > callers can do themselves). > Hmm, I thought about this for a while. Then I looked at what ls-tree.c does(using an accumulator). They already routinely use temporary `strbuf`s to calculate relative/absolute paths before appending them to their main output string. Because callers who need to accumulate can easily do the preserving and appending themselves with a temporary buffer, there is no reason to force that overhead into our helper. I will change the semantics from "append" to "replace", rename the helper back to `format_path()`. I hope I am looking at ls-tree.c correctly here : ) Eliminate the wasteful `canonical_buf` allocations so we can pass the destination buffer directly to functions like `strbuf_realpath_forgiving()`. This is a good suggestion actually, thanks! > Otherwise, looking good as a no-op bug-to-bug compatible rewrite, > with a slight optimization (to skip xgetcwd()). > > Thanks. On Mon, Jun 22, 2026 at 9:33 PM Junio C Hamano wrote: > > Junio C Hamano writes: > > ... > > It is a minor point, but wouldn't it make it simpler to handle > > format_default first? I.e., > > > > if (format == FORMAT_DEFAULT) > > switch (def) { > > case DEFAULT_RELATIVE: > > format = DEFAULT_RELATIVE; > > break; > > ... > > case DEFAULT_UNMODIFIED: > > default: > > format = DEFAULT_UNMODIFIED; > > break; > > } > > switch (format) { > > case FORMAT_RELATIVE: fmt = PATH_FORMAT_RELATIVE; break; > > case FORMAT_CANONICAL: fmt = PATH_FORMAT_CANONICAL; break; > > ... > > } > > > > Perhaps yes, perhaps not. I dunno. > > I do not consider the above an blocker, but it might make a > difference if we are going to acquire more modes and formats, so > once somebody tries to rewrite the logic and finds the resulting > code harder to follow (or not easier to follow), I would be happy to > see the above discarded ;-) > True, if new formats are introduced this would instantly become sloppy. I will change it to future proof since I am looking to send v8 for append_formatted_path(). Although I would be surprised to see an example for a new format. > >> +/** > >> + * 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); > >> + > > > > It is slightly unsatisfying that this function is defined to > > "append" to any existing value in the dest strbuf, rather than > > storing the result in the dest strbuf. The original caller > > print_path() passes an empty strbuf to this helper, so it can let > > strbuf_realpath_*() functions to strbuf_reset() it (e.g., > > abspath.c:get_root_part() called by strbuf_realpath_1(), wihch in > > turn is called by strbuf_realpath() and strbuf_realpath_forgiving()) > > it freely, which means that use of temporary strbuf like > > canonical_buf only to copy it out to dest is wasteful and unneeded. > > But other callers we will have for this helper later may want to > > append to what they already have, so perhaps it is OK (on the other > > hand, we could say that preserving and appending is what these > > callers can do themselves). > > This one we may want to consider a bit more seriously, but it is > entirely up to the future callers of the helper. If it would make > the callers much easier to write for this helper to have "append" > semantics, I'd be happy to accept the semantics of the above as-is, > but otherwise, I suspect it would be simpler to use if the helper is > defined to replase dest with the result, instead of appending the > result to dest. > I am still unsure if I am following ls-tree.c correctly. If I am then I think it is a very good change to have for v8 as I specified above. > > Otherwise, looking good as a no-op bug-to-bug compatible rewrite, > > with a slight optimization (to skip xgetcwd()). > > This part of the review does not change in any case. The > refactoring looks good. Thank you ; ) Regards, - K Jayatheerth