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. There is at least one other possible outcome, which is that remove_path() is replaced with remove_path_not_cwd() in all callers that obviously want it e.g. builtins, but not replaced in other callers. My mental model of this is that the two functions serve two different use cases: 1) remove_path(): Remove a path and all empty directories 2) remove_path_not_cwd(): Remove a path and all empty directories, except cwd > 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. I trust that you have considered this change carefully, so I will downgrade my objection to a nitpick. remove_path() seems nice to have as a low-level function but I certainly can't imagine any non-contrived use cases that *wouldn't* benefit. To me, a more compelling argument is that protecting cwd is important in order to ensure correctness, and user experience is an incidental benefit. AFAICT that is not the argument you are making, but perhaps there is some correctness benefit as well?
