Re: [PATCHv3 5/5] submodule--helper clone: lose the extra prefix option

[Stefan Beller <sbeller@google.com>]

On Fri, Mar 25, 2016 at 3:39 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> The replacement works fine in all tests except for the recursive
>> tests as then the chdir is an important detail. In the submodule
>> there is no $wt_prefix (as it is the parents' wt_prefix we passed in),
>
> So the real reason is that we may tweak $wt_prefix to refer to a
> non-existing directory, that submodule--helper is buggy and does not
> notice it, and that using "-C $wt_prefix" would catch it because it
> first tries to chdir to it.

I agree that we tweak wt_prefix to refer to an existing path.
However I would claim that it's git-submodule.sh which is buggy, not
the submodule--helper, because the shell script gives the non-extistent
path to submodule--helper in the first place.

 "-C $wt_prefix" would catch it indeed, so we need to fix that before we
switch to -C (if at all)

>
> The calling script "git submodule" first sets $wt_prefix to the
> original directory, so there is no way chdir'ing back there would
> not work,

right,

> but if we update it (e.g. by appending a path to a
> submodule we want to work in), it may very well end up referring to
> a directory that does not exist (e.g. it may not have been
> "init"ed).  Is that what is going on?

Or by changing dirs in the shell script, i.e.

wt_prefix=$(git rev-parse --show-toplevel)
# wt_prefix is correct until we do:
cd sm_path

# If we now would "chdir'ing back there", we try to
(in $sm_path) >  cd ./$wt_prefix
# which would be  $sm_path/$wt_prefix and that may not exists.

>
> If that is the case, it makes a lot more sense as an explanation.
> The justification for the main change 4/5 in the log message,
> i.e. "-C $wt_prefix" is more familiar, was an irrelevant subjective
> statement that only said "we could change it to use -C" without
> explaining why "--prefix $wt_prefix" was wrong, and that was why I
> was unhappy about it.
>
>> So here is the example from before:
>>         repo/ # a superproject repository
>>         repo/untracked/ # an untracked dir in repo/
>>         repo/sub/ # a submodule
>>         repo/sub/subsub # a submodule of a submodule
>>
>> When calling "git submodule <cmd> --recursive" from within
>> repo/untracked/, the recursed instance will end up in
>> repo/sub/ with the parents prefix ("untracked/")
>>
>> In case of git -C $wt_prefix we would try to chdir into
>> repo/sub/untracked/ which doesn't exist and our journey ends here.
>
> That sounds like an unrelated bug, though.

It is the thing described above.

>  Whether -C or --prefix
> is used, we shouldn't be using "repo/sub/untracked" after going to
> "repo/sub".  If the <cmd> somehow wanted to refer to a relative path
> (e.g. "file") in the original directory, it should be using
> ../untracked as the base (e.g. it would use "../untracked/file").

In case of --prefix we do not chdir into "repo/sub/untracked"; however
we have cwd="repo/sub/" and wt_prefix=untracked (as it's stale from
the parent). We have no way of knowing where "untracked/" applies to.
As the root of the repository is now "repo/sub/" (It is a different repo,
the submodule), we may assume there is a "repo/sub/untracked/",
but we never compute that value or chdir there. It's just cwd and wt_prefix
being from different repositories. (superproject&submodule)

>
> Of course by using -C, you might notice that repo/sub/untracked does
> not exist, but that is not a proper error checking---what if the
> submodule at repo/sub does have a directory with that name?  IOW,
> the computation that gave repo/sub/untracked instead of ../untracked
> is wrong and using -C would not make it right.

There is no explicit computation of repo/sub/untracked, but it would happen
implicitely in the -C case as we'd be in the repo/sub and try to chdir
to 'untracked'
(interpreted as a relative path)

>
> And if you clear the prefix you originally obtained in calling
> script "git submodule", which is "untracked/", even if <cmd> somehow
> wanted to refer to the "file" in that directory, the only clue to do
> so is forever lost.  Again, this is unrelated to -C/--prefix, but
> that is what the patch 2 in the original series (which was rolled
> into patch 1 in the update) was about.

As of now this file would also be lost I would assume, as it is unclear
which repository you refer to.

If you are in the "subsub" submodule and know that the $wt_prefix=untracked,
you still don't know if the original command was invoked from the root super
project or the intermediate submodule.

In the current situation you /may/ be able to reconstruct that by going back
"prefix" as that seems to be the path we traveled so far.


>
> So I am not sure what the value of using -C is.  At least that
> "example from before" does not serve as a good justification.
>