Re: [PATCH] submodules: fix of regression on fetching of non-init subsub-repo

["Peter Kästle" <peter.kaestle@nokia.com>]

Hi Eric,

On 04.12.20 19:06, Eric Sunshine wrote:
> On Fri, Dec 4, 2020 at 10:25 AM Peter Kaestle <peter.kaestle@nokia.com> 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@nokia.com>
>> ---
>> 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;