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

Stefan Beller <sbeller@google.com> writes:

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.