Re: [PATCH v2 5/6] tests: remove duplicate .gitmodules path

From: Glen Choo <chooglen@google.com>
Date: Mar 7, 2023, 00:51 UTC
Message-ID: <kl6llek9idt8.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <20230228185642.2357806-5-calvinwan@google.com>

Calvin Wan <calvinwan@google.com> writes:

Show 5 quoted lines

> Swapping `git add <submodule>` to `git submodule add <submodule>`
> in a previous patch created a .gitmodules file with multiple
> submodules pointing to the same path in certain tests. Fix tests
> so that they are run on the original added submodule rather than
> a separate manually configured submodule.

Without reading the diff, I thought that having multiple configured submodules was a bug that needed to be fixed (I also wasn't sure which previous patch was being referenced). But after reading the diff, this seems much more like a test simplification.

I think this is might be better squashed with the previous patch (the combined diff of both patches looks ok to me) with some extra clarification, e.g.:

  In some of the tests, we modify .gitmodules to check whether the
  value of "submodule.subname.ignore" is respected. When we were using
  "git add", we also had to temporarily turn the gitlink into a
  submodule by setting "submodule.subname.path", but since "git
  submodule add" handles that for us, let's use the real submodule
  instead.

Show 32 quoted lines

> @@ -194,27 +191,24 @@ test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match)'
>  '
>  
>  test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match) [.gitmodules]' '
> -	git config --add -f .gitmodules submodule.subname.ignore all &&
> -	git config --add -f .gitmodules submodule.subname.path sub &&
> +	git config --add -f .gitmodules submodule.sub.ignore all &&
>  	git commit -m "Update .gitmodules" .gitmodules &&
>  	git diff HEAD >actual2 &&
>  	test_must_be_empty actual2 &&
> -	git config -f .gitmodules submodule.subname.ignore untracked &&
> +	git config -f .gitmodules submodule.sub.ignore untracked &&
>  	git commit -m "Update .gitmodules" .gitmodules &&
>  	git diff HEAD >actual3 &&
>  	test_must_be_empty actual3 &&
> -	git config -f .gitmodules submodule.subname.ignore dirty &&
> +	git config -f .gitmodules submodule.sub.ignore dirty &&
>  	git commit -m "Update .gitmodules" .gitmodules &&
>  	git diff HEAD >actual4 &&
>  	test_must_be_empty actual4 &&
> -	git config submodule.subname.ignore none &&
> -	git config submodule.subname.path sub &&
> +	git config submodule.sub.ignore none &&
>  	git diff HEAD >actual &&
>  	sed -e "1,/^@@/d" actual >actual.body &&
>  	expect_from_to >expect.body $subprev $subprev-dirty &&
>  	test_cmp expect.body actual.body &&
> -	git config --remove-section submodule.subname &&
> -	git config --remove-section -f .gitmodules submodule.subname &&
> +	git config --unset submodule.sub.ignore &&
>  	git reset --hard pristine-gitmodules
>  '

Not the fault of this patch, but I think that this diff would have been easier to read if the latter part were moved into "test_when_finished", and it would be clear that we are just undoing what we are setting up a few lines earlier (instead of a whole test block earlier).