[RFC,0/6] add: block invalid submodules

Message

Calvin Wan Feb. 13, 2023, 6:21 p.m. UTC

I will be taking over a series that Josh was working on before he
went on parental leave. For this RFC, I was wondering whether this
approach to blocking users from eventually commiting invalid
submodules is feasible. Eg. should we block at commit time instead?
And if so, how would that approach look?

=== Cover letter ===

At $dayjob, we believe that we need to change the default behavior
around adding embedded repositories. Currently, we issue advice that
embedded repositories cannot be checked out when the top-level repo is
cloned, and that the user should probably use a submodule instead.

However, our experience is that many users ignore this advice, and
commit embedded repositories to shared projects. This causes toil for
repo administrators who must correct these mistakes.

The main point of this series is to change the warning issued in this
case into a fatal error. However, this breaks lots of test cases that
were implemented prior to the original creation of the warning, so the
bulk of the changes are test cleanups.

For reviewer convenience, I've tried to group the test changes with
similar structure into the same commits:
* Patch 1: Fixes a leak in submodule-config.c
* Patch 2: Move setup code into test_expect blocks
* Patch 3: Substitute `git submodule add` instead of `git add` for
  simple cases (no other adjustments necessary)
* Patch 4: As in patch 2, but also adjust expected diff output
* Patch 5: As in patch 2, but also adjust expected status output
* Patch 6: Change the embedded repo warning to a fatal error

An open question:
* Currently we can bypass the embedded repo check with
  `--no-warn-embedded-repo`. I've kept the name of the flag the same for
  backwards compatibility. Should we instead rename the flag?

Calvin Wan (1):
  leak fix: cache_put_path

Josh Steadmon (5):
  t4041, t4060: modernize test style
  tests: Use `git submodule add` instead of `git add`
  tests: use `git submodule add` and fix expected diffs
  tests: use `git submodule add` and fix expected status
  add: reject nested repositories

 Documentation/git-add.txt                    |   7 +-
 builtin/add.c                                |  28 ++-
 submodule-config.c                           |   4 +-
 t/t0008-ignores.sh                           |   2 +-
 t/t2013-checkout-submodule.sh                |   4 +-
 t/t2103-update-index-ignore-missing.sh       |   2 +-
 t/t2107-update-index-basic.sh                |   2 +-
 t/t3040-subprojects-basic.sh                 |   5 +-
 t/t3050-subprojects-fetch.sh                 |   3 +-
 t/t3404-rebase-interactive.sh                |   3 +-
 t/t3701-add-interactive.sh                   |   5 +-
 t/t4010-diff-pathspec.sh                     |   2 +-
 t/t4020-diff-external.sh                     |   2 +-
 t/t4027-diff-submodule.sh                    |  17 +-
 t/t4035-diff-quiet.sh                        |   1 +
 t/t4041-diff-submodule-option.sh             | 228 +++++++++++++++----
 t/t4060-diff-submodule-option-diff-format.sh | 224 +++++++++++++-----
 t/t5531-deep-submodule-push.sh               |   4 +-
 t/t6416-recursive-corner-cases.sh            |  12 +-
 t/t6430-merge-recursive.sh                   |   1 +
 t/t6437-submodule-merge.sh                   |  12 +-
 t/t7400-submodule-basic.sh                   |   4 +-
 t/t7401-submodule-summary.sh                 |   4 +-
 t/t7402-submodule-rebase.sh                  |   2 +-
 t/t7412-submodule-absorbgitdirs.sh           |   2 +-
 t/t7414-submodule-mistakes.sh                |  21 +-
 t/t7450-bad-git-dotfiles.sh                  |   2 +-
 t/t7506-status-submodule.sh                  |  15 +-
 t/t7508-status.sh                            | 134 +++++++++--
 29 files changed, 569 insertions(+), 183 deletions(-)