Re: [RFC PATCH] Move git-dir for submodules
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 21, 2011, 20:28 UTC
- Message-ID
- <7vhb6f1ipp.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <1311267139-14658-1-git-send-email-iveqy@iveqy.com>
Fredrik Gustafsson <iveqy@iveqy.com> writes:
Show 11 quoted lines
> 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).
Show 9 quoted lines
> + 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?
Show 9 quoted lines
> + 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 > }
Show 13 quoted lines
> 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.
Show 11 quoted lines
> 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.