Fredrik Gustafsson <iveqy@xxxxxxxxx> writes:
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 87c9452..3ad3012 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -122,14 +122,56 @@ module_clone()
> path=$1
> url=$2
> reference="$3"
> + gitdir=
> + gitdir_base=
> + base_path=`echo $path | sed -e 's|[^/]*$||'`
We prefer $() over `` these days, no? Without dq around $path, you would
not be able to preserve $IFS inside $PATH. You are stripping a run of non
slash at the trailing end --- is 'dirname "$path"' insufficient?
I think you are using the path the submodule happens to be at in the
current checkout to decide where in the .git/modules in the superproject
to keep the submodule metadata directory. Shouldn't you be using
module_name to convert the $path to the name of the submodule (this is
important, as the same submodule that used to be at path P1 can be moved
to a different path P2 in the history).
> + if test -z "$GIT_DIR"
> then
> + gitdir=$(git rev-parse --git-dir)
> + gitdir_base="$gitdir/modules/$base_path"
> + gitdir="$gitdir/modules/$path"
> else
> + gitdir="$GIT_DIR/modules/$path"
> + gitdir_base="$GIT_DIR/modules/$base_path"
> + fi
Why do you need to switch on "test -z $GIT_DIR" yourself to have two
paths? Doesn't "git rev-parse --git-dir" already know to take $GIT_DIR
into account?
> + case $gitdir in
> + /*)
Indent case arm labels at the same level as "case/esac".
> + if test -d "$gitdir"
> + then
> + mkdir -p "$path"
> + echo "gitdir: $rel_gitdir" > "$path/.git"
Good: if it already exists, do not clone, but just reuse what .git/modules
hierarchy of the superproject has. Is it really necessary to have an ugly
loop to make things relative, though?
Also please lose the extra SP after redirection, i.e.
command >"$path/.git"
> + else
> + if !(test -d "$gitdir_base")
Do you need subshell for this?
> + then
> + mkdir -p "$gitdir_base"
> + fi
Doesn't unconditional "mkdir -p" do the right thing?
> + if test -n "$reference"
> + then
> + git-clone "$reference" -n "$url" "$path" --separate-git-dir "$gitdir"
> + else
> + git-clone -n "$url" "$path" --separate-git-dir "$gitdir"
> + fi ||
> + die "$(eval_gettext "Clone of '\$url' into submodule path '\$path' failed")"
> + fi
> }
> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index b2b26b7..57f5306 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -358,10 +358,10 @@ test_expect_success 'update --init' '
> git submodule update init > update.out &&
> cat update.out &&
> test_i18ngrep "not initialized" update.out &&
> - ! test -d init/.git &&
> + ! test -f init/.git &&
> git submodule update --init init &&
> - test -d init/.git
> + test -n $(grep "/^gitdir: /" init/.git)
I wonder if we want a new option to "git rev-parse" so that we can say
git rev-parse --is-well-formed-git-dir init/.git
to perform these checks without exposing the implimentation detail.
> diff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh
> index d600583..b0517f0 100755
> --- a/t/t7403-submodule-sync.sh
> +++ b/t/t7403-submodule-sync.sh
> @@ -55,7 +55,7 @@ test_expect_success '"git submodule sync" should update submodule URLs' '
> git pull --no-recurse-submodules &&
> git submodule sync
> ) &&
> - test -d "$(git config -f super-clone/submodule/.git/config \
> + test -d "$(git config -f super-clone/.git/modules/submodule/config \
> remote.origin.url)" &&
Is "submodule" initialized at this point? If so, I would think it is
vastly preferrable to say it like this:
test -d "$(
cd super-clone/submodule &&
git config --local remote.origin.url
)"
I won't comment on the rest, but I think you can follow the same line of
thought to come up with a fix so that they do not have to know the
implementation detail of where the subproject metainfo directory is.
--
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