Re: [PATCHv3 1/5] submodule: prepare recursive path from non root directory

[Junio C Hamano <gitster@pobox.com>]

Stefan Beller <sbeller@google.com> writes:

> diff --git a/git-submodule.sh b/git-submodule.sh
> index 43c68de..d83608c 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -825,8 +825,9 @@ Maybe you want to use 'update --init'?")"
>  		if test -n "$recursive"
>  		then
>  			(
> -				prefix="$prefix$sm_path/"
> +				prefix="$(relative_path $prefix$sm_path)/"

Don't you need to protect yourself from IFS chars in $prefix and
$sm_path here (the other invocation of relative_path in this patch
does not have this issue, I would think).

As I think about it more, I am more inclined to say "-C $prefix" is
going in a wrong direction, so justifying this change solely on that
move is somewhat sad.

 - Making things relative should not hurt.

 - Clearing wt_prefix feels like the right thing, because we are
   moving to the root of the working tree of a different repository
   and wt_prefix that tells us where the user originally was in the
   superproject is not useful in the context of the submodule, which
   after all is a separate project.  The user however cannot refer
   to things on the filesystem (including use of pathspecs), as they
   are taken relative to the root level of each submodule by
   clearing wt_prefix, though.  i.e.

       $ git submodule $cmd --read-from-this-file=../m/file

   that is started from t/ subdirectory of the superproject that
   recurses into the submodule at m/ (sitting next to t/) should be
   able to read from "file" sitting at the root level of the working
   tree of submodule m/ by stripping ../m/ (and relative-path should
   be able to help with that), but that may become impossible
   because the fact that the user named ../m/file relative to t/
   subdirectory is lost by clearing wt_prefix that used to hold t/
   here.

is the closest justification I can come to, which is weak ("should
not hurt" does not justify, and "users cannot ever do this without
undoing this change" does not, either).

I think the worst part of this change is that the log message does
not make it clear why it is OK not to clear wt_prefix and not to use
relative_path if you use --prefix, while the "-C $prefix" approach
has problems without these changes.  Without that explanation, these
symptoms, i.e. the fact that you need the changes in this patch,
only smells like an indication that "-C $prefix" approach is somehow
flawed.  I cannot quite pinpoint what that "somehow" is, though.

>  				clear_local_git_env
> +				wt_prefix=
>  				cd "$sm_path" &&
>  				eval cmd_update
>  			)
> @@ -1159,6 +1160,7 @@ cmd_status()
>  			(
>  				prefix="$displaypath/"
>  				clear_local_git_env
> +				wt_prefix=
>  				cd "$sm_path" &&
>  				eval cmd_status
>  			) ||
> @@ -1239,7 +1241,8 @@ cmd_sync()
>  
>  				if test -n "$recursive"
>  				then
> -					prefix="$prefix$sm_path/"
> +					prefix=$(relative_path "$prefix$sm_path/")
> +					wt_prefix=
>  					eval cmd_sync
>  				fi
>  			)