Re: [RFC PATCH v2 10/12] t7416: add new test about HEAD:.gitmodules and not existing .gitmodules

On Thu, 2 Aug 2018 13:43:05 -0700
Stefan Beller [off-list ref] wrote:

On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite [off-list ref] wrote:

quoted

git submodule commands can now access .gitmodules from the current
branch even when it's not in the working tree, add some tests for that
scenario.

Signed-off-by: Antonio Ospite <redacted>
---

[...]

quoted

+NOTE: "git mv" and "git rm" are still supposed to work even without
+a .gitmodules file, as stated in the t3600-rm.sh and t7001-mv.sh tests.

"supposed to work" != "tested that it works" ?

"git mv submod new_submod" and "git rm submod" are actually expected to
work without the .gitmodules file, and there are tests about that in
t3600-rm.sh and t7001-mv.sh:

t3600-rm.sh:
  'rm does not complain when no .gitmodules file is found'

t7001-mv.sh:
  'git mv moves a submodule with a .git directory and no .gitmodules'  
  'mv does not complain when no .gitmodules file is found'

I am not sure what the NOTE wants to tell me? (Should I review those
tests to double check them now? or do we just want to tell future readers
of this test there are other tangent tests to this?)

Admittedly the NOTE is not useful without any context: during the
development of "submodule--helper config --stage" I initially assumed
that "git mv" and "git rm" should fail if .gitmodules was not available,
because these commands modify .gitmodules and I added code for that in
stage_updated_gitmodules().

But then later I found out that my assumption was wrong and that git has
tests to verify that these operations on submodules succeed even when
.gitmodules does not exist, which was a little of a surprise to me.

So I removed all my code that was conflicting with git assumptions, and
added the NOTE. However I guess that was primarily a note to myself, and
it should have not slipped in the public patches.

I think I will remove the note, it can be confusing and does not really
add anything, and even less considering that "submodule--helper config
--stage" is going to be dropped.

[...]

quoted

+test_expect_success 'not adding submodules when the gitmodules config is not checked out' '
+       (cd super &&
+               test_must_fail git submodule add ../new_submodule
+       )
+'
+
+# "git add" in the test above fails as expected, however it still leaves the
+# cloned tree in there and adds a config entry to .git/config. This is because
+# no cleanup is done by cmd_add in git-submodule.sh when "git
+# submodule--helper config" fails to add a new config setting.
+#
+# If we added the following commands to the test above:
+#
+#   rm -rf .git/modules/new_submodule &&
+#   git reset HEAD new_submodule &&
+#   rm -rf new_submodule

Alternatively we could check for the existence of .gitmodules
before starting all these things?

You mean in cmd_add(), before doing anything?

The following would anticipates the same check which makes "git submodule
add" fail:

diff --git a/git-submodule.sh b/git-submodule.sh
index ff258e2e8c..b261175143 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh

@@ -159,6 +159,11 @@ cmd_add()
                shift
        done

+       if test ! -e .gitmodules && git cat-file -e HEAD:.gitmodules
+       then
+                die "$(eval_gettext "please make sure that the .gitmodules file in the current branch is checked out")"
+       fi
+
        if test -n "$reference_path"
        then
                is_absolute_path "$reference_path" ||

This refers to .gitmodules explicitly but we said that we do not care
about that for now, if opaque access was ever needed in the future,
something like "submodule--helper config --is-writeable" could be added.

I think it is okay to not clean up if we check all "regular" or rather expected
things such as a non-writable .gitmodules file before actually doing it.
(This is similar to 'checkout' that walks the whole tree and checks if the
checkout is possible given the dirtyness of the tree, to either abort early
or pull through completely. In catastrophic problems such as a full disk
we'd still die in the middle of work)

quoted

+#
+# then the repository would be in a clean state and the test below would pass.
+#
+# Maybe cmd_add should do the cleanup from above itself when failing to add
+# a submodule.
+test_expect_failure 'init submodule after adding failed when the gitmodules config is not checked out' '

So this comment and test is about explaining why we can fail mid way through,
which we could not before unless we had the catastrophic event.

I think we should check for a "writable" .gitmodules file at the beginning,
which is if (G || (!G && !H)) [using the notation from the cover letter]?

quoted

+       (cd super &&
+               git submodule init

With the change from above this last test passes.

BTW the check I am using here and in the code of submodule--helper,
corresponds indeed to the boolean expression you mentioned, but
simplified and negated.

Thanks,
   Antonio

-- 
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?