On Sun, Dec 6, 2020 at 5:56 PM brian m. carlson <sandals@xxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx> > --- > +/* > + * 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).
