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

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

On 07.12.20 20:22, Junio C Hamano wrote:
> Peter Kaestle <peter.kaestle@nokia.com> writes:
> 
>> +add_commit_push () {
>> +	dir="$1" &&
>> +	msg="$2" &&
>> +	shift 2 &&
>> +	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
>> +}
> 
> 
> 
>> +test_expect_success 'fetching a superproject containing an uninitialized sub/sub project' '
>> +	# depends on previous test for setup
>> +
>> +	git -C B/ fetch &&
>> +	compare_refs_in_dir A origin/master B origin/master
> 
> Can we do this without relying on the name of the default branch?
> Perhaps when outer, middle and inner are prepared, they can be
> forced to be on the 'sample' (not 'master' nor 'main') branch, or
> something like that?

Using origin/HEAD for compare_refs_in_dir should be fine without 
additional setup, as for the regression the "git -C B/ fetch" will fail 
and return with false (see description of the patch).  This 
compare_refs_in_dir is just for additional checking as you proposed in 
the mail:
https://public-inbox.org/git/xmqqk0uuct94.fsf@gitster.c.googlers.com/
------------8<-------------
> And from B that was an original copy of A with only the top and
> middle layer instantiated, you run "git fetch".  Are you happy as
> long as "git fetch" does not exit with non-zero status?  That is
> hard to believe---it may be a necessary condition for the command to
> exit with zero status, but you have other expectations, like what
> commit the remote tracking branch refs/remotes/origin/HEAD ought to
> be pointing at.  I think we should check that, too.
----------->8-------------


> 
>> +test_expect_success 'setup recursive fetch with uninit submodule' '
>> +	# does not depend on any previous test setups
>> +
>> +	git init main &&
>> +	git init sub &&
> 
> "super vs sub" would give us a better contrast than "main vs sub",
> and it would help reduce mistakes in the mechanical conversion of
> "master" to "main" happening in another topic.
> 

ok.


>> +	# In a regression the following git call will run into infinite recursion.
>> +	# To handle that, we connect the grep command to the git call by a pipe
>> +	# so that grep can kill the infinite recusion when detected.
>> +	# The recursion creates git output like:
>> +	# Fetching submodule sub
>> +	# Fetching submodule sub/sub              <-- [1]
>> +	# Fetching submodule sub/sub/sub
>> +	# ...
>> +	# [1] grep will trigger here and kill git by exiting and closing its stdin
> 
> "trigger here and kill..." -> "stop reading and cause git to
> eventually stop and die"
> 
> But we probably cannot use 'grep -m1' so it is a moot point.
> 
>> +
>> +	! git -C main fetch --recurse-submodules 2>&1 |
>> +		grep -v -m1 "Fetching submodule sub$" &&
> 
> Unfortunately, "grep -m<count>" is not even in POSIX, I would think.
> 
> What do we expect to happen in the correct case?

sigh, we can't use grep -m1.  Too bad, it was such a nice solution.

> 
>   - A line "Fetching submodule sub" and nothing else is given?  That
>     feels a bit brittle (how are we making sure, in the presence of
>     "2>&1", that we will not get any other output, like progress?)
> 
>   - "sub" is the only thing that appears on lines that begin with
>     "Fetching submodule" (i.e. "Fetching submodule $something" where
>     $something is not 'sub' is an error), and we allow other garbage
>     in the output?  That would be a bit more robust than the above.
> 
> As you seem to be comfortable using "sed" below, perhaps use it to
> extract the first few lines that say "^Fetching submodule " from the
> output and stop, and check that the output has only one such line
> about 'sub' and nothing else?

According to [1] posix sed offers equal possibility to quit like grep 
-m1 and I'll adopt:
$> yes posixgrepisnogoodforus | sed "/posix/q"

[1] https://pubs.opengroup.org/onlinepubs/9699919799/utilities/sed.html


Looking at the other mails I think I processed over all open comments 
and will prepare for v3.  Thanks again.

-- 
kind regards
--peter;