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

From: Glen Choo <chooglen@google.com>
Date: Nov 23, 2021, 18:19 UTC
Message-ID: <kl6lmtluka55.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <CABPp-BE0Bcimwr1wwcnnh+6apx7r114Oqnu=QDgKEn6VAHAtFg@mail.gmail.com>

Elijah Newren <newren@gmail.com> writes:

Show 13 quoted lines

>> 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);

Show 5 quoted lines

>> 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.