From: "Peter Kästle" <peter.kaestle@nokia.com>
To: Eric Sunshine <sunshine@sunshineco.com>
Cc: Junio C Hamano <gitster@pobox.com>,
Philippe Blain <levraiphilippeblain@gmail.com>,
Git List <git@vger.kernel.org>,
Ralf Thielow <ralf.thielow@gmail.com>
Subject: Re: [PATCH] submodules: fix of regression on fetching of non-init subsub-repo
Date: Mon, 7 Dec 2020 09:28:39 +0100 [thread overview]
Message-ID: <0deeeabe-9590-db7a-4a07-447d43df7a24@nokia.com> (raw)
In-Reply-To: <CAPig+cR69HJefRMfH_5-dHOMVY-VmVgbqQuWV90ednDEjrnExw@mail.gmail.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;
next prev parent reply other threads:[~2020-12-07 8:29 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-12-02 15:56 BUG in fetching non-checked out submodule Ralf Thielow
2020-12-02 17:19 ` Philippe Blain
2020-12-02 23:06 ` Junio C Hamano
2020-12-03 7:54 ` Peter Kästle
2020-12-03 15:25 ` Philippe Blain
2020-12-03 15:33 ` Peter Kästle
2020-12-03 18:12 ` Junio C Hamano
2020-12-04 15:23 ` [PATCH] submodules: fix of regression on fetching of non-init subsub-repo Peter Kaestle
2020-12-04 18:06 ` Eric Sunshine
2020-12-07 8:28 ` Peter Kästle [this message]
2020-12-07 8:40 ` Eric Sunshine
2020-12-07 13:46 ` [PATCH v2] " Peter Kaestle
2020-12-07 18:42 ` Philippe Blain
2020-12-07 19:43 ` Junio C Hamano
2020-12-08 8:46 ` Peter Kästle
2020-12-07 19:56 ` Junio C Hamano
2020-12-08 14:06 ` Peter Kästle
2020-12-07 19:22 ` Junio C Hamano
2020-12-07 20:44 ` Philippe Blain
2020-12-07 21:02 ` Junio C Hamano
2020-12-07 21:10 ` Junio C Hamano
2020-12-08 14:58 ` Peter Kästle
2020-12-08 15:42 ` [PATCH v3] " Peter Kaestle
2020-12-08 15:51 ` Peter Kästle
2020-12-08 20:46 ` Junio C Hamano
2020-12-08 23:25 ` Philippe Blain
2020-12-09 9:58 ` Peter Kästle
2020-12-09 10:58 ` [PATCH v4] " Peter Kaestle
2020-12-09 14:00 ` Philippe Blain
2020-12-03 7:45 ` BUG in fetching non-checked out submodule Ralf Thielow
2020-12-03 8:20 ` Peter Kästle
2020-12-03 9:38 ` Ralf Thielow
2020-12-03 9:43 ` Peter Kästle
2020-12-03 12:30 ` Ralf Thielow
2020-12-03 15:10 ` Peter Kästle
2020-12-03 16:45 ` Ralf Thielow
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=0deeeabe-9590-db7a-4a07-447d43df7a24@nokia.com \
--to=peter.kaestle@nokia.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=levraiphilippeblain@gmail.com \
--cc=ralf.thielow@gmail.com \
--cc=sunshine@sunshineco.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).