Re: [PATCH][RESEND] git-submodule: provide easy way of adding new submodules

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Sven Verdoolaege <skimo@xxxxxxxxxx> writes:

>  COMMANDS
>  --------
> +add::
> +	Add the given repository as a submodule at the given path
> +	to the changeset to be committed next.  In particular, the
> +	repository is cloned at the specified path, added to the
> +	changeset and registered in .gitmodules.   If no path is
> +	specified, the path is deduced from the repository specification.
> +

Somehow "git submodule add $URL $my_subdirectory" feels
unnatural, although it certainly is simpler to write the command
usage string.  Wouldn't a commit on the maintenance branch of
cgit.git want to say "Add the 'maint' branch of git.git as my
submodule", for example?

The alternatives I can come up with do not feel right either, though.

	git submodule $my_subdirectory $URL [$branch]
	git submodule $URL [--branch $branch] $my_subdirectory

Hmmm...

> diff --git a/git-submodule.sh b/git-submodule.sh
> index 89a3885..3df7121 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -1,13 +1,14 @@
>  #!/bin/sh
>  #
> -# git-submodules.sh: init, update or list git submodules
> +# git-submodules.sh: add, init, update or list git submodules
>  #
>  # Copyright (c) 2007 Lars Hjemli
>  
> -USAGE='[--quiet] [--cached] [status|init|update] [--] [<path>...]'
> +USAGE='[--quiet] [--cached] [add <repo>|status|init|update] [--] [<path>...]'

Can a single repo added at more than one path with this syntax?
I do not see that in the code, but this implies such.

>  . git-sh-setup
>  require_work_tree
>  
> +add=
>  init=
>  update=
>  status=
> @@ -25,6 +26,17 @@ say()
>  	fi
>  }
>  
> +get_repo_base() {
> +	(
> +		cd "`/bin/pwd`" &&
> +		cd "$1" || cd "$1.git" &&
> +		{
> +			cd .git
> +			pwd
> +		}
> +	) 2>/dev/null
> +}
> +

I've seen this code before elsewhere.  We do not need to
refactor right now with this patch, but please mark this copy
with something like:

	# NEEDSWORK: identical function exists in get_repo_base
        # in clone.sh
	get_repo_base () {
        	...

as a reminder.

> @@ -66,6 +78,44 @@ module_clone()
>  }
>  
>  #
> +# Add a new submodule to the working tree, .gitmodules and the index
> +#
> +# $@ = repo [path]
> +#
> +module_add()
> +{
> +	repo=$1
> +	path=$2
> +
> +	# Turn the source into an absolute path if
> +	# it is local
> +	if base=$(get_repo_base "$repo"); then
> +		repo="$base"
> +	fi
> +
> +	# Guess path from repo if not specified or strip trailing slashes
> +	if test -z "$path"; then
> +		path=$(echo "$repo" | sed -e 's|/*$||' -e 's|:*/*\.git$||' -e 's|.*[/:]||g')
> +	else
> +		path=$(echo "$path" | sed -e 's|/*$||')
> +	fi
> +
> +	test -e "$path" &&
> +	die "'$path' already exists"
> +
> +	module_clone "$path" "$repo" || exit

 - module_clone catches the "$path already exists" case; but the
   test is done differently.  One particular case of "an empty
   directory exists" is allowed there, but you are dying early
   to forbid it.  Is that warranted?  My gut feeling is that
   they should share the same check, iow, don't check yourself
   but have module_clone take care of the error case.

 - If $path does not exist in the worktree (because it hasn't
   been checked out), but does exist in the index, what should
   happen?  Should it be flagged as an error (in module_clone,
   not here)?

-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux