Re: [PATCH 8/8] dir: avoid removing the current working directory

From: Elijah Newren <newren@gmail.com>
Date: Nov 23, 2021, 23:13 UTC
Message-ID: <CABPp-BHn0bE4ZSx25+28GD58sae=FVs63eQW-Fp8zwFAALcKFA@mail.gmail.com>
In-Reply-To: <kl6lzgputxxw.fsf@chooglen-macbookpro.roam.corp.google.com>

On Tue, Nov 23, 2021 at 12:33 PM Glen Choo <chooglen@google.com> wrote:

Show 22 quoted lines

>
> Elijah Newren <newren@gmail.com> 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.