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

[Glen Choo <chooglen@google.com>]

Elijah Newren <newren@gmail.com> writes:

>> I don't think it's appropriate to implement user-facing concern (don't remove
>> cwd because this will confuse users) in library functions like remove_path().
>> remove_path() has other callers e.g. midx.c, and possible future callers e.g.
>> we're working on adding a command to delete corrupted commit-graphs and this
>> library function would be extremely handy.
>
> I think we'd want this code change for those cases too.  Said another
> way, why wouldn't these callers want to avoid deleting the original
> current working directory of the git process (which is likely still
> the current working directory of the parent process)?  Deleting that
> directory causes problems regardless of whether it's a user-facing
> command (rm, merge, stash, etc.) or something more internal (midx or
> commit-graphs stuff being called by gc) that is doing the deleting.

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.

I'm not going to say that we'll *definitely* need remove_path()
in its current form, but mixing concerns like this is an invitation to
unexpected behavior. An (imperfect) example that demonstrates this
principle is https://lore.kernel.org/git/24bffdab139435173712101aaf72f7277298c99d.1632497954.git.gitgitgadget@gmail.com/,
where we made a change to a generic path matching function in order to
speed up unpack_trees(), but accidentally ended up breaking gitignore.

> Putting it in this helper function means we protect all current and
> future callers without developers having to remember which
> "remove_path()" variant they need and why.

Code comments sound like adequate protection to me. Fudging the names a
little..

  /*
   * Pretend we have utility function that generalizes
   * check-then-delete (though we probably won't need it).
   */
  static int remove_path_conditionally(const char *name, check_path_fn can_delete_path);

  /**
   * This is identical to remove_path(), except that it will die if
   * attempting to remove the_cwd. When writing Git commands, prefer
   * using this over remove_path() so that we don't delete the cwd and
   * leave the user in a confusing state.
   */
  int remove_path_except_cwd(const char *name)
  {
    return remove_path_conditionally(name, die_on_cwd);
  }

  /*
   * Tries to remove the path with empty directories along it, ignores
   * ENOENT. Unless you really need to remove the path unconditionally,
   * consider using remove_path_except_cwd() instead.
   */
  int remove_path(const char *name);

>> It seems more appropriate to check the_cwd from builtin/add.c and builtin/rm.c
>> instead.
>
> Not sure how you determined that those two paths are affected or that
> those are the only two.

Typo: s/add/apply.

I took the example from your test cases:

  diff --git a/t/t2501-cwd-empty.sh b/t/t2501-cwd-empty.sh
  index ff4e7cd89fa..4362e7b15e5 100755
  --- a/t/t2501-cwd-empty.sh
  +++ b/t/t2501-cwd-empty.sh
  @@ -191,7 +191,7 @@ test_expect_success 'revert fails if cwd needs to be removed' '
    test_path_is_dir dirORfile
   '
 
  -test_expect_failure 'rm does not remove cwd incidentally' '
  +test_expect_success 'rm does not remove cwd incidentally' '
    test_when_finished "git reset --hard" &&
    git checkout foo/bar/baz &&
 
  @@ -205,7 +205,7 @@ test_expect_failure 'rm does not remove cwd incidentally' '
    test_path_is_dir foo
   '
 
  -test_expect_failure 'apply does not remove cwd incidentally' '
  +test_expect_success 'apply does not remove cwd incidentally' '
    test_when_finished "git reset --hard" &&
    git checkout foo/bar/baz &&

I read this as "I made these changes in order to make these tests pass".
I really like the 'TDD-ish' approach you used in this series; as a
reader, it gave me a clear idea of the expected outcome of your changes.
From that perspective, the fact that there are certainly untested paths
which are affected takes away some of the benefits of this approach.