Hi Eric,
On 04.12.20 19:06, Eric Sunshine wrote:
On Fri, Dec 4, 2020 at 10:25 AM Peter Kaestle <peter.kaestle@xxxxxxxxx> wrote:
[...]
Furthermore a regression test case is added, which tests for recursive
fetches on a superproject with uninitialized sub repositories. This
issue was leading to an infinite loop when doing a revert of a62387b.
Just a few small comments (nothing comprehensive) from a quick scan of
the patch...
Mostly they are just minor style issues, not necessarily worth a
re-roll, but there is one actionable item.
thanks for your comments. A new patch will follow soon.
Signed-off-by: Peter Kaestle <peter.kaestle@xxxxxxxxx>
---
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
@@ -719,4 +719,98 @@ test_expect_success 'fetch new submodule commit intermittently referenced by sup
+add_commit_push () {
+ dir="$1"
+ msg="$2"
+ shift 2
We typically recommend including these assignments in the &&-chain to
future-proof against someone later inserting code above them and not
realizing that that code is not part of the &&-chain, in which case if
the new code fails, the failure might go unnoticed.
ok
+ git -C "$dir" add "$@" &&
+ git -C "$dir" commit -a -m "$msg" &&
+ git -C "$dir" push
+}
+
+compare_refs_in_dir () {
+ fail= &&
+ if test "x$1" = 'x!'
+ then
+ fail='!' &&
+ shift
+ fi &&
+ git -C "$1" rev-parse --verify "$2" >expect &&
+ git -C "$3" rev-parse --verify "$4" >actual &&
+ eval $fail test_cmp expect actual
+}
We have a test_cmp_rev() similar to this but it doesn't support -C as
some of our other test functions do. I briefly wondered if it would
make sense to extend it to understand -C, but even that wouldn't help
this case since compare_refs_in_dir() introduced here involves two
distinct directories. The need here is so special-purpose that it
likely would not make sense to upgrade test_cmp_rev() to accommodate
it. Okay.
Yes, I saw that there's a similar function and I tried to modify this
one first. Unfortunately this didn't work without touching much
unaffected test code. So I propose to continue with this additional
function.
+test_expect_success 'setup nested submodule fetch test' '
+ # does not depend on any previous test setups
+
+ for repo in outer middle inner
+ do
+ (
+ git init --bare $repo &&
+ git clone $repo ${repo}_content &&
+ echo "$repo" >"${repo}_content/file" &&
+ add_commit_push ${repo}_content "initial" file
+ ) || return 1
+ done &&
What is the purpose of the subshell here? Is it to ensure that commits
in each repo have identical timestamps? Or is it just for making the
&& and || expression more clear? If the latter, we normally don't
bother with the parentheses.
It was intended to make the correlation of && and || clear. I have
experienced many cases in the past where things were screwed because it
was not clearly understood by everybody. I'll propose next patch
without this subshell.
+ git clone outer A &&
+ git -C A submodule add "$pwd/middle" &&
+ git -C A/middle/ submodule add "$pwd/inner" &&
+ add_commit_push A/middle/ "adding inner sub" .gitmodules inner &&
+ add_commit_push A/ "adding middle sub" .gitmodules middle &&
+
+ git clone outer B &&
+ git -C B/ submodule update --init middle &&
+
+ compare_refs_in_dir A HEAD B HEAD &&
+ compare_refs_in_dir A/middle HEAD B/middle HEAD &&
+ test -f B/file &&
+ test -f B/middle/file &&
+ ! test -f B/middle/inner/file &&
These days we typically use test_path_exists() (or
test_path_is_file()) and test_path_is_missing() rather than bare
`test`.
ok.
+test_expect_success 'setup recursive fetch with uninit submodule' '
+ # does not depend on any previous test setups
+
+ git init main &&
+ git init sub &&
+
+ touch sub/file &&
Unless the timestamp of the file is significant to the test, in which
case `touch` is used, we normally create empty files like this:
>sub/file &&
ok.
+test_expect_success 'recursive fetch with uninit submodule' '
+ git -C main submodule deinit -f sub &&
+ ! git -C main fetch --recurse-submodules |&
+ grep -v -m1 "Fetching submodule sub$" &&
We want the test scripts to be portable, thus avoid Bashisms such as `|&`. > We also avoid placing a Git command upstream in a pipe since doing so
causes the exit code of the Git command to be lost. Instead, we would
normally send the Git output to a file and then send that file to
whatever would be downstream of the Git command in the pipe. So, a
mechanical rewrite of the above (without thinking too hard about it)
might be:
git -C main fetch --recurse-submodules >out 2>&1 &&
! grep -v -m1 "Fetching submodule sub$" &&
In general I agree, but for this special test case, it's required to
have the two commands connected by a pipe, as the grep needs to kill the
git call in error case. Otherwise for this regression git would go for
an infinite recursion loop.
Of course, we can go for a "git 2>&1 | grep" solution.
+ git -C main submodule status |
+ sed -e "s/^-//" -e "s/ sub$//" >actual &&
Same comment about avoiding Git upstream in a pipe, so perhaps:
git -C main submodule status >out &&
sed -e "s/^-//" -e "s/ sub$//" out >actual &&
+ test_cmp expect actual
+'
ok.
--
kind regards
--peter;
