On 5/20/2025 7:35 AM, Junio C Hamano wrote: > Jacob Keller <jacob.e.keller@xxxxxxxxx> writes: > >> From: Jacob Keller <jacob.keller@xxxxxxxxx> >> >> The prefix_path_gently() -- and its wrapper prefix_path() -- function >> normalizes the provided path and optionally adds the provided prefix to >> relative paths. > > I find "optionally" confusing here. The original intended use case > of this function being that "the provided path", which comes from > the end user from the command line that names a file relative to the > directory the usre started the "git" command in, needs to be > adjusted after "git" chdir's up to the top-level of the working > tree, and the way to do so is to prepend the "prefix" computed by > "git" (which always ends with a slash). Adding the prefix to > relative paths is the central part of what it has to do. When the > end-user supplied path goes up e.g., "../file", we may have to lose > a level or more of the prefix before prepending, but that does not > change the fact that the helper function is about prepending the > prefix. > > And as you can guess from the above description, if the caller > passes prefix that does not end in a trailing slash, the caller is > buggy. > Sure. Part of the reason I was thinking modify this here instead of adding the trailing slash, is because to add a trailing slash I had to convert the path to a strbuf, where as by inserting it here thats handled by the format specifiers. >> If the prefix does not end in a trailing slash, the >> function will combine the last component of the prefix with the first >> component of the relative path. This is unlikely to produce a desirable >> result. > > True, and I do not mind being lenient to buggy callers, but given > that the majority of callers (i.e. all the existing callers) not > being buggy, I wonder if it is better to check and append a slash to > the end by a new caller that feeds a prefix that directly comes from > the end user? > > Unlike prefix that is internally generated by Git, we'd need to be a > lot more careful if we are taking end-user input directly and > passing it as a "prefix" (I take that the possible lack of trailing > slash as a signal that you are doing something like that in this > series). We may need to check and correct that they do not contain > "./", "//", or "../", for example, anyway, and adding the required > trailing slash at the end sounds like something we may want to do as > part of that input validation and massaging _before_ calling these > helper functions. Yea, I think it boils down to me passing the two paths from the user in as the prefix or root of the pathspecs in parse_pathspec. Perhaps we either need a different function to clean up the path first or modify the pathspec code to not use prefix_path here at all and use something else instead.
