Re: [PATCH] Add git-submodule command

From: Junio C Hamano <junkio@cox.net>
Date: May 26, 2007, 01:23 UTC
Message-ID: <7v1wh4ped4.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <11801386283399-git-send-email-hjemli@gmail.com>

Lars Hjemli <hjemli@gmail.com> writes:

Show 10 quoted lines

> With this entry in .gitmodules (and a commit reference in the index entry for
> the path "git"), the command 'git submodule init' will clone the repository
> at kernel.org into the directory "git".
>
> Signed-off-by: Lars Hjemli <hjemli@gmail.com>
> ---
>
> On 5/26/07, Lars Hjemli <hjemli@gmail.com> wrote:
>> I'll redo the patch, removing the branch-specific things, and try to
>> shut up :)

Hey, don't shut up. Starting small and covering corner cases incrementally is really the right approach.

Show 8 quoted lines

> +status::
> +	Show the status of the submodules. This will print the sha1 of the
> +	currently checked out commit for each submodule, along with the
> +	submodule path and the output of gitlink:git-describe[1] for the
> +	sha1. Each sha1 will be prefixed with '-' if the submodule is not
> +	initialized and '+' if the currently checked out submodule commit
> +	does not match the sha1 found in the index of the containing
> +	repository. This command is the default command for git-submodule.

(markup) probably you would want `` there for typewriter face.

(wording) didn't we have "the name of the hash function is SHA-1" patch earlier? I'd personally prefer calling them "object names", though...

Other than that, the command description is very nicely done, which means the design of the command set hence the semantics is cleanly done. Good job.

Show 17 quoted lines

> diff --git a/git-submodule.sh b/git-submodule.sh
> new file mode 100755
> index 0000000..247b1ee
> --- /dev/null
> +++ b/git-submodule.sh
> @@ -0,0 +1,172 @@
> ...
> +#
> +# print stuff on stdout unless -q was specified
> +#
> +say()
> +{
> +	if test -z "$quiet"
> +	then
> +		echo -e "$@"
> +	fi
> +}

We tend to avoid "echo -e" (not POSIX). I do not see any string you feed to this function that you would _want_ backslash escaped sequences (actually I would suspect you would not want them).

Show 9 quoted lines

> +
> +#
> +# Run clone + checkout on missing submodules
> +#
> +# $@ = requested paths (default to all)
> +#
> +modules_init()
> +{
> +	git ls-files --stage -- $@ | grep -e '^160000 ' |

Did you mean "$@", i.e. inside double-quotes?

Because this pattern would appear a lot in superproject support, it might be a good idea to give a new option, --subprojects, to git-ls-files to limit its output to 160000 entries, but that is a minor detail.

> +	while read mode sha1 stage path
> +	do

We would need to undo the shell-safety "quoted" output of paths here. I suspect it would be much easier to code this in Perl or Python, do the "grep -e" part above in the script, when we start caring about unwrapping c-quoting of path (or "ls-files -z").

But that is a minor detail we could fix up later.

Show 7 quoted lines

> +		test -d "$path/.git" && continue
> +
> +		if test -d "$path"
> +		then
> +			rmdir "$path" 2>/dev/null ||
> +			die "Directory '$path' exist, but not as a submodule"
> +		fi

Could the currently checked-out $path be a symlink to another directory, and what does the code do in such a case?

> +
> +		test -e "$path" &&
> +		die "A file already exist at path '$path'"

"test -e" is a relatively new invention and I tended to stay away from it, but it should be safe to use these days...

Show 6 quoted lines

> +		url=$(GIT_CONFIG=.gitmodules git-config module."$path".url)
> +		test -z "$url" &&
> +		die "No url found for submodule '$path' in .gitmodules"
> +
> +		git-clone "$url" "$path" ||
> +		die "Clone of submodule '$path' failed"

"git-clone -n" please, as you will checkout something different in the next step anyway.

> +
> +		$(unset GIT_DIR && cd "$path" && git-checkout -q "$sha1") ||
> +		die "Checkout of submodule '$path' failed"

Lose $() around this, as it is not producing a string which is the name of the command to run. You do want a subshell here because of chdir, so instead of losing $(), replace them with ().

Show 9 quoted lines

> ...
> +modules_update()
> +{
>...
> +		if test "$subsha1" != "$sha1"
> +		then
> +			$(unset GIT_DIR && cd "$path" && git-fetch &&
> +				git-checkout -q "$sha1") ||
> +			die "Unable to checkout '$sha1' in submodule '$path'"

Likewise.