Re: [PATCH] git-submodule and --upload-pack

[Junio C Hamano <gitster@pobox.com>]

Giulio Eulisse <Giulio.Eulisse@cern.ch> writes:

> There was a thread a while ago aboyt having --upload-pack support for
> git-submodule.
>
> Given that there was no followup (as far as I can tell) and I needed
> pretty much
> the same functionality I ported Jason's patch to work on top of 1.6.4.2.

Thanks.

Can you point at the original patch with a usable commit log message, in
the gmane archive (i.e. http://thread.gmane.org/...) if possible?  I
do not think we have that patch queued anywhere even in 'pu'.

Given that it looks like a new feature, I do not think it would be
appropriate for any of the future 1.6.4.X series, but if it is useful we
may want to have it in the upcoming 1.6.5 release.

> Comments?

See below.

> diff --git a/Documentation/git-submodule.txt b/Documentation/git- 
> submodule.txt

The patch is linewrapped and will not be applicable.  But I'll comment on
the contents to save a round-trip.

> diff --git a/Documentation/gitmodules.txt b/Documentation/gitmodules.txt
> index 5daf750..bf982a6 100644
> --- a/Documentation/gitmodules.txt
> +++ b/Documentation/gitmodules.txt
> @@ -30,6 +30,14 @@ submodule.<name>.path::
>  submodule.<name>.url::
>  	Defines an url from where the submodule repository can be cloned.
>
> +submodule.<name>.receivepack::
> +	The default program to execute on the remote side when pushing.  See
> +	option \--receive-pack of linkgit:git-push[1].
> +
> +submodule.<name>.uploadpack::
> +	The default program to execute on the remote side when fetching.  See
> +	option \--upload-pack of linkgit:git-fetch-pack[1].

This placement of description in the documentation and variables in the
namespace quite sane, as these are properties of the remote site, and they
belong together with submodule.<name>.url.

> @@ -53,12 +60,16 @@ Consider the following .gitmodules file:
>
>  	[submodule "libbar"]
>  		path = include/bar
> -		url = git://bar.com/git/lib.git
> +		url = ssh://bar.com/~/git/lib.git
> +		uploadpack = /home/you/bin/git-upload-pack-wrapper
> +		receivepack = /home/you/bin/git-receive-pack-wrapper
> ...
> +For `libbar`, packs are retrieved and stored via the upload and receive
> +wrappers, respectively.

Using a custom wrapper in this example feels very misleading.  The option
is primarily meant as a workaround for a broken or hard-to-modify sshd
settings that does not allow you to include the directory you installed
upload-pack/receive-pack to the PATH environment when the ssh session is
not interactive.

> @@ -97,13 +99,30 @@ module_clone()
>  	test -e "$path" &&
>  	die "A file already exist at path '$path'"
>
> +        uploadpackCmd=""
> +
> +        if test "$uploadpack"
> +        then
> +          uploadpackCmd="--upload-pack $uploadpack"

Can the value of uploadpack contain a shell IFS?  This is a rhetorical
question---read on.

> +        fi
> +
>  	if test -n "$reference"
>  	then
> -		git-clone "$reference" -n "$url" "$path"
> +		git-clone $uploadpackCmd "$reference" -n "$url" "$path"

Without using uploadpackCmd and risking to trash IFS characters in the
variable, you can do something like:

    git clone ${uploadpack+--upload-pack "$uploadpack"} ...

I would prefer the code not to set variable "uploadpack" when nothing is
specified, instead of setting it to an empty string like this patch does,
but if you are going to use an empty string as a signal that no uploadpack
is specified, then you would need a colon between 'k'and '+' in the above.

> @@ -738,6 +801,18 @@ cmd_sync()
>  			remote=$(get_default_remote)
>  			say "Synchronizing submodule url for '$name'"
>  			git config remote."$remote".url "$url"
> +			uploadpack=$(git config -f .gitmodules submodule."$name".uploadpack)
> +			receivepack=$(git config -f .gitmodules
> submodule."$name".receivepack)
> +			if test "$uploadpack"
> +			then
> +			    git config submodule."$name".uploadpack "$uploadpack" ||
> +			    echo "  Warn: Failed to set uploadpack for
> $url' in submodule  path '$name'."
> +			fi
> +			if test "$receivepack"
> +			then
> +			    git config submodule."$name".receivepack "$receivepack" ||
> +			    echo "  Warn: Failed to set receivepack
> for '$url' in  submodule path '$name'."
> +			fi
>  		)

I do not agree with this part, nor what the "cmd_init" does.

Having URL/uploadpack/receivepack 3-tuple in the tracked .gitmodules is
sensible, as that is how the project expresses its recommendations to
people who clone the toplevel project.

However, after the top-level project is cloned and the submodules are
populated in the work tree of the person who cloned, I think these values
should be propagated to $path/.git/config, i.e. the configuration file of
the submodule checkout.

Inside cmd_update(), there is this code snippet:

	if test -z "$nofetch"
	then
		(unset GIT_DIR; cd "$path" &&
			git-fetch) ||
		die "Unable to fetch in submodule path '$path'"
	fi

And the patch does not touch it.  For this git-fetch to honour the custom
uploadpack your user configured, remote.origin.uploadpack variable in the
configuration file of the submodule checkout needs to be updated, because
this fetch will not (and should not) look at the configuration file of the
superproject.

You _could_ also copy them to submodule.$name.$var of the top-level
project if you really wanted to, but I do not think doing so serves any
useful purpose.