Re: [PATCH v4 1/2] abspath: add a function to resolve paths with missing components

[Eric Sunshine <sunshine@sunshineco.com>]

On Sun, Dec 6, 2020 at 5:56 PM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
> We'd like to canonicalize paths such that we can preserve any number of
> trailing components that may be missing.  Let's add a function to do
> that, allowing either one or an unlimited number of components to
> canonicalize, and make strbuf_realpath a wrapper around it that allows
> just one component.

This commit message is too barebones. As a justification, "We'd
like..." is insufficient; it doesn't help the reader understand why
this change is desirable.

Further, the lack of explanation about the seemingly arbitrary "one or
infinite" condition confuses the issue. The first question which
popped into this reader's head was "why those two specific choices?".
What makes one missing path component special as opposed to any number
of missing components? (These questions are mostly rhetorical; I can
figure out reasonable answers, but the commit message ought to do a
better job of explaining.)

> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
> +/*
> + * Just like strbuf_realpath, but allows specifying how many missing components
> + * are permitted.  If many_missing is true, an arbitrary number of path
> + * components may be missing; otherwise, only the last component may be missing.
> + */
> +char *strbuf_realpath_missing(struct strbuf *resolved, const char *path,
> +                             int many_missing, int die_on_error)

This interface feels odd. Why would a caller ever want to call this
function with many_missing=0 when it would be easier, shorter, more
direct to simply call strbuf_realpath()? Is the plan to retire
strbuf_realpath() down the road?

A more orthogonal-feeling interface would be to make this function
_always_ allow any number of missing trailing path components (that
is, drop `many_missing` altogether). Doing so would simplify the
documentation and the signature. If the caller needs the original
behavior of only allowing the final path component to be missing, then
strbuf_realpath() can be used, as usual.

The name of the function is somewhat confusing, especially if you take
the suggestion of dropping the `many_missing` argument. Perhaps a name
such as strbuf_realpath_forgiving() would be more understandable.

Note that the above comments are only about the API. It's perfectly
fine if the two functions share an underlying private implementation
(making them each one-liners calling the actual worker function).