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

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

[Giulio Eulisse]

Ciao,

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.

Comments?

Ciao,
Giulio
--

diff --git a/Documentation/git-submodule.txt b/Documentation/git- 
submodule.txt
index 683ba1a..50aa930 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -9,8 +9,8 @@ git-submodule - Initialize, update or inspect submodules
  SYNOPSIS
  --------
  [verse]
-'git submodule' [--quiet] add [-b branch]
-	      [--reference <repository>] [--] <repository> <path>
+'git submodule' [--quiet] add [-b branch]
+              [-u <git-upload-pack>] [--receive-pack <git-receive- 
pack>] [--reference <repository>] [--] <repository> <path>
  'git submodule' [--quiet] status [--cached] [--] [<path>...]
  'git submodule' [--quiet] init [--] [<path>...]
  'git submodule' [--quiet] update [--init] [-N|--no-fetch] [--rebase]
@@ -164,6 +164,19 @@ OPTIONS
  --branch::
  	Branch of repository to add as submodule.

+--upload-pack <upload-pack>::
+-u <upload-pack>::
+	When given, and the repository to clone from is accessed
+	via ssh, this specifies a non-default path for the
+	'git-upload-pack' program on the remote end.  See
+	linkgit:git-fetch-pack[1].
+
+--receive-pack <receive-pack>::
+	When given, and the repository to clone from is accessed
+	via ssh, this specifies a non-default path for the
+	'git-receive-pack' program on the remote end.  See
+	linkgit:git-push[1].
+
  --cached::
  	This option is only valid for status and summary commands.  These
  	commands typically use the commit found in the submodule HEAD, but
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].
+
  submodule.<name>.update::
  	Defines what to do when the submodule is updated by the superproject.
  	If 'checkout' (the default), the new commit specified in the
@@ -41,7 +49,6 @@ submodule.<name>.update::
  	This config option is overridden if 'git submodule update' is given
  	the '--merge' or '--rebase' options.

-
  EXAMPLES
  --------

@@ -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


  This defines two submodules, `libfoo` and `libbar`. These are  
expected to
  be checked out in the paths 'include/foo' and 'include/bar', and for  
both
  submodules an url is specified which can be used for cloning the  
submodules.
+For `libbar`, packs are retrieved and stored via the upload and receive
+wrappers, respectively.

  SEE ALSO
  --------
diff --git a/git-submodule.sh b/git-submodule.sh
index ebed711..25edd59 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -82,6 +82,8 @@ module_clone()
  	path=$1
  	url=$2
  	reference="$3"
+        uploadpack=$4
+        receivepack=$5

  	# If there already is a directory at the submodule path,
  	# expect it to be empty (since that is the default checkout
@@ -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"
+        fi
+
  	if test -n "$reference"
  	then
-		git-clone "$reference" -n "$url" "$path"
+		git-clone $uploadpackCmd "$reference" -n "$url" "$path"
  	else
-		git-clone -n "$url" "$path"
+		git-clone $uploadpackCmd -n "$url" "$path"
  	fi ||
  	die "Clone of '$url' into submodule path '$path' failed"
+	if test "$uploadpack"
+	then
+	    git config -f "${path}/.git/config" remote.origin.uploadpack  
"$uploadpack" ||
+	    echo "  Warn: Failed to set uploadpack for '$url' in submodule  
path '$path'."
+	fi
+	if test "$receivepack"
+	then
+	    git config -f "${path}/.git/config" remote.origin.receivepack  
"$receivepack" ||
+	    echo "  Warn: Failed to set receivepack for '$url' in submodule  
path '$path'."
+	fi
  }

  #
@@ -136,6 +155,16 @@ cmd_add()
  			reference="$1"
  			shift
  			;;
+		-u|--upload-pack)
+			case "$2" in '') usage ;; esac
+			uploadpack=$2
+			shift
+			;;
+		--receive-pack)
+			case "$2" in '') usage ;; esac
+			receivepack=$2
+			shift
+			;;
  		--)
  			shift
  			break
@@ -206,9 +235,17 @@ cmd_add()
  			;;
  		esac
  		git config submodule."$path".url "$url"
+		if test "$uploadpack"
+			then
+		    git config submodule."$path".uploadpack "$uploadpack"
+		fi
+		if test "$receivepack"
+		then
+		    git config submodule."$path".receivepack "$receivepack"
+		fi
  	else

-		module_clone "$path" "$realrepo" "$reference" || exit
+		module_clone "$path" "$realrepo" "$reference" "$uploadpack"  
"$receivepack" || exit
  		(
  			unset GIT_DIR
  			cd "$path" &&
@@ -224,7 +261,19 @@ cmd_add()
  	die "Failed to add submodule '$path'"

  	git config -f .gitmodules submodule."$path".path "$path" &&
-	git config -f .gitmodules submodule."$path".url "$repo" &&
+	git config -f .gitmodules submodule."$path".url "$repo" ||
+	die "Failed to register submodule '$path'"
+
+	if test "$uploadpack"
+        then
+	    git config -f .gitmodules submodule."$path".uploadpack  
"$uploadpack" ||
+	    die "Failed to register submodule '$path'"
+	fi
+	if test "$receivepack"
+        then
+	    git config -f .gitmodules submodule."$path".receivepack  
"$receivepack" ||
+	    die "Failed to register submodule '$path'"
+	fi
  	git add .gitmodules ||
  	die "Failed to register submodule '$path'"
  }
@@ -299,6 +348,19 @@ cmd_init()
  		git config submodule."$name".url "$url" ||
  		die "Failed to register url for submodule path '$path'"

+		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 '$path'."
+		fi
+		if test "$receivepack"
+		then
+		    git config submodule."$name".receivepack "$receivepack" ||
+		    echo "  Warn: Failed to set receivepack for '$url' in submodule  
path '$path'."
+		fi
+
  		upd="$(git config -f .gitmodules submodule."$name".update)"
  		test -z "$upd" ||
  		git config submodule."$name".update "$upd" ||
@@ -384,7 +446,8 @@ cmd_update()

  		if ! test -d "$path"/.git -o -f "$path"/.git
  		then
-			module_clone "$path" "$url" "$reference"|| exit
+			module_clone "$path" "$url" "$reference" "$(git config  
submodule."$name".uploadpack)" \
+                            "$(git config  
submodule."$name".receivepack)" || exit
  			subsha1=
  		else
  			subsha1=$(unset GIT_DIR; cd "$path" &&
@@ -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
  		)
  		fi
  	done

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

[Junio C Hamano]

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.

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

[Giulio Eulisse]

>> 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'.

Ciao,
sorry for the mangled patch... Looks like Mail.app did something  
weird...

http://article.gmane.org/gmane.comp.version-control.git/104188/match=git+submodule+upload+pack


> 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.
[...]

Ok. I will try to follow your suggestion and post an updated patch.

Ciao,
Giulio