On Tue, Nov 23, 2021 at 12:33 PM Glen Choo <chooglen@xxxxxxxxxx> wrote: > > Elijah Newren <newren@xxxxxxxxx> writes: > > >> I agree that most, possibly all, of our commands should prefer to die > >> than to remove the cwd, but that doesn't justify adding > >> application-level concerns to a general-purpose utility function. Even > >> if it sounds overly defensive, having an obviously correct utility > >> function makes it easier for future authors to know exactly what their > >> code is doing and why. And surely if we're imaginative enough, we can > >> definitely dream up some possible use cases for remove_path() that don't > >> want this dying behavior e.g. other applications that link to our > >> libraries, or some new merge strategy that may need to remove + restore > >> the cwd. > > > > Sounds like your objections here are based on a misunderstanding. I > > totally agree with you that adding dying behavior to these functions > > would be wrong. > > > > My patch doesn't do that. > > Ah my mistake, that should be s/die/'stop gently'. Even so, that is not > at the core of my objection, mixing of concerns is. If I were to introduce a new function, say remove_path_not_cwd(), to avoid this claimed mixing of concerns, what would that buy us? I've looked at every single caller of remove_path() in the git codebase. If I did introduce a new function, as you seem to want, my series would include two more commits: one that would replace _every_ call of remove_path() in the codebase with a call to the new function, and one that would delete the remove_path() declaration and definition in dir.[ch] since they would be unused. The net effect would be merely forcing git developers to learn a different function name. (I'd probably also follow it up later with another commit to rename remove_path_not_cwd() to remove_path(), for simplicity, and so I don't have to remember the longer name anymore.) I haven't yet found or heard of any potential callers, even hypothetical, that would be harmed by the modified behavior. Every case suggested so far actually sounds like a good candidate for the modified behavior.
