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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 25, 2016, 22:39 UTC
Message-ID: <xmqqmvpm18sw.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAGZ79kZotAZeKJxRsHczMp5BG7edu=GLj6mox08M6afkMu1UDg@mail.gmail.com>

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.

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, 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?

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.

Show 12 quoted lines

> 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. 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").

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.

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.

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.