Re: [PATCH v3 1/3] submodule: use "fetch" logic instead of custom remote discovery

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 16, 2020, 21:13 UTC
Message-ID: <xmqqa6vh80pq.fsf@gitster.c.googlers.com>
In-Reply-To: <20201114122132.4344-2-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Show 11 quoted lines

> Replace a use of the get_default_remote() function with an invocation
> of "git fetch"
>
> The "fetch" command already has logic to discover the remote for the
> current branch. However, before it learned to accept a custom
> refspec *and* use its idea of the default remote, it wasn't possible
> to get rid of some equivalent of the "get_default_remote" invocation
> here.
>
> As it turns out the recently added "--stdin" option to fetch[1] gives
> us a way to do that. Let's use it instead.

Ah, that was what your earlier question was about.

The hits from "git grep fetch_in_submodule" says that $1 is the path to the submodule, $2 is the $depth and $3 (when exists) is the object name of what we are fetching:

git-submodule.sh:578: fetch_in_submodule "$sm_path" $depth || git-submodule.sh:601: fetch_in_submodule "$sm_path" $depth || git-submodule.sh:607: fetch_in_submodule "$sm_path" $depth "$sha1" ||

Show 5 quoted lines

> While I'm at it simplify the "fetch_in_submodule" function. It wasn't
> necessary to pass "$@" to "fetch" since we'd only ever provide one
> SHA-1 as an argument in the previous "*" codepath (in addition to
> "--depth=N"). Rewrite the function to more narrowly reflect its
> use-case.

Show 8 quoted lines

>  	cd "$1" &&
> -	case "$2" in
> -	'')
> -		git fetch ;;
> -	*)
> -		shift
> -		git fetch $(get_default_remote) "$@" ;;
> -	esac

So when $2 (i.e. --depth=N) is empty, we just did the default "git fetch". Otherwise, "git fetch <remote> --depth=N [<sha1>]" was run.

> +	if test $# -eq 3
> +	then
> +		echo "$3" | git fetch --stdin "$2"

This is not quite equivalent in that an empty depth would have resulted in the default "git fetch" in the original code, with or without "$3", totally ignoring "$sha1". Is this a bugfix snuck in "while we are at it", or is this an unintended regression?

Show 6 quoted lines

> +	elif test "$2" -ne ""
> +	then
> +		git fetch "$2"
> +	else
> +		git fetch
> +	fi

Even if it is an unintended regression, fixing it should be trivial; we just need to flip the order of conditions, like so:

	if test -z "$2"
	then
		git fetch
	elif test $# = 3
	then
		echo "$3" | git fetch --stdin "$2"
	else
		git fetch "$2"
	fi