Re: [PATCH 03/15] lib-submodule-update.sh: define tests for recursing into submodules

On Thu, Feb 16, 2017 at 12:39 PM, Junio C Hamano [off-list ref] wrote:

Stefan Beller [off-list ref] writes:

quoted

Currently lib-submodule-update.sh provides 2 functions
test_submodule_switch and test_submodule_forced_switch that are used by a
variety of tests to ensure that submodules behave as expected. The current
expected behavior is that submodules are not touched at all (see
42639d2317a for the exact setup).

In the future we want to teach all these commands to properly recurse
into submodules. To do that, we'll add two testing functions to
submodule-update-lib.sh test_submodule_switch_recursing and
test_submodule_forced_switch_recursing.

I'd remove "properly" and insert a colon after "add two ... to
submodule-update-lib.sh" before the names of two functions.

ok

quoted

+reset_work_tree_to_interested () {
+     reset_work_tree_to $1 &&
+     # indicate we are interested in the submodule:
+     git -C submodule_update config submodule.sub1.url "bogus" &&
+     # also have it available:
+     if ! test -d submodule_update/.git/modules/sub1
+     then
+             mkdir submodule_update/.git/modules &&

Would we want "mkdir -p" here to be safe?

Yes I cannot think of a downside of being overly cautious here.

quoted

+             cp -r submodule_update_repo/.git/modules/sub1 submodule_update/.git/modules/sub1

... ahh, wouldn't matter that much, we checked that modules/sub1
does not exist, and as long as nobody creates modules/ or modules/somethingelse
we are OK.

Well, I'll add the -p

quoted

+# Test that submodule contents are correctly updated when switching
+# between commits that change a submodule.
+# Test that the following transitions are correctly handled:
+# (These tests are also above in the case where we expect no change
+#  in the submodule)
+# - Updated submodule
+# - New submodule
+# - Removed submodule
+# - Directory containing tracked files replaced by submodule
+# - Submodule replaced by tracked files in directory
+# - Submodule replaced by tracked file with the same name
+# - tracked file replaced by submodule

These should work without trouble only when the paths involved in
the operation in the working tree are clean, right?  Just double
checking.  If they are dirty we should expect a failure, instead of
silent loss of information.

yes, I'll go over the tests again and add those cases if missing.

quoted

+     command="$1"

The dq-pair is not strictly needed on the RHS of the assignment, but
it is a good way to signal that we considered that we might receive
an argument with $IFS in it...

quoted

+                     $command add_sub1 &&

... and after doing so, not quoting $command here signals that we
expect command line splitting to happen.  Am I reading it correctly?
Without an actual caller appearing in this step, it is rather hard
to judge.

I followed the existing code without thinking about these points, but they are
valid and exactly how we'd expect the code to behave.
$1 / $command will be e.g. "git checkout --recurse-submodules" in
this patch series; but later on we could also have functions.
C.f.  t4137 which defines a function

    apply_3way () {
        git diff --ignore-submodules=dirty "..$1" | git apply --3way -
    }
    test_submodule_switch "apply_3way"

We'd want to have a similar thing for the recursing part, e.g.

    apply_3way_recursing () {
        git diff --submodule=diff "..$1" | git apply
--recurse-submodules --3way -
    }
    test_submodule_switch_recursing "apply_3way_recursing"

quoted

+                     echo sub1 > .git/info/exclude

    ">.git/info/exclude"

ok