Re: [RFC PATCH v2 12/12] submodule: remove the .gitmodules file when it is empty

From: Stefan Beller <sbeller@google.com>
Date: Aug 2, 2018, 21:15 UTC
Message-ID: <CAGZ79kaDK9nuUm_hqAZKMHydiaBDhDkM_y1UFwpRHoGjCQoq4w@mail.gmail.com>
In-Reply-To: <20180802134634.10300-13-ao2@ao2.it>

On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite <ao2@ao2.it> wrote:

>
> In particular this makes it possible to really clean things up when
> removing the last submodule with "git rm".

This sentence is a continuation of the subject line, and I had to reread it to follow along.

>
> The rationale is that if git creates .gitmodules when adding the first
> submodule it should also remove it when removing the last submodule.

I agree with this sentiment. It seems slightly odd to me to have this tied in the same patch series that changes .gitmodules reading behavior as I could think of this feature as orthogonal to what this series achieved up to patch 10.

> -       test_cmp expect actual &&
> +       test_cmp expect.both_deleted actual &&

This seems to be the re-occuring pattern in t3600, and given that we have

  cat >expect <<EOF
  M  .gitmodules
  D  submod
  EOF
  cat >expect.both_deleted<<EOF
  D  .gitmodules
  D  submod
  EOF

with no other writing of expect in the range, this seems to be correct. Maybe worth testing that we do not delete a .gitmodules file if we have more than one submodule? (But I would expect this to be covered implicitly somewhere in the test suite. If so that would be worth mentioning in the commit message instead of writing a test -- just looking quickly we do have " git rm --cached submodule2" in t7406 which might be sufficient?)

Show 13 quoted lines

>  test_expect_success "rm absorbs submodule's nested .git directory" '
> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index 48fd14fae6..2bb42a4c8f 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -99,6 +99,17 @@ inspect() {
>         )
>  }
>
> +test_expect_success 'removal of last submodule also removes empty .gitmodules' '
> +       (
> +               cd addtest &&
> +               test ! -d .git/modules &&

test_path_is_missing ?

> +               git submodule add -q "$submodurl" first_submod &&
> +               test -e .gitmodules &&

test_path_is_file

> +               git rm -f first_submod &&

Do we need to force it here?

> +               test ! -e .gitmodules

test_path_is_missing

Thanks for this series! Overall it was a pleasant read, though I had some comments.

Thanks, Stefan