On Mon, Oct 17, 2016 at 12:10 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Johannes Schindelin <Johannes.Schindelin@xxxxxx> writes:
>
>>> > expecting success:
>>> > actual=$(git submodule--helper resolve-relative-url-test '(null)' '/usr/src/git/wip/t/trash directory.t0060-path-utils/.' '../.') &&
>>> > test "$actual" = 'C:/git-sdk-64/usr/src/git/wip/t/trash directory.t0060-path-utils/.'
>>> >
>>> > +++ git submodule--helper resolve-relative-url-test '(null)' '/usr/src/git/wip/t/trash directory.t0060-path-utils/.' ../.
>>> > ++ actual=C:/git-sdk-64/usr/src/git/wip/t/.
>>> > ++ test C:/git-sdk-64/usr/src/git/wip/t/. = 'C:/git-sdk-64/usr/src/git/wip/t/trash directory.t0060-path-utils/.'
>
> This may well be total misunderstanding on my side, but is the
> expectation of this test even correct? If it wants to take "../."
> relative to "$LEAD/t/trash utils/.", should't it go one level up
> with ".." to $LEAD/t and then stay there with ".", expecting
> "$LEAD/t" which is what the above is giving us?
>
> IOW, the above makes me wonder why having one of these as the base
>
> A - path/to/dir
> B - path/to/dir/
> C - path/to/dir/.
>
> to resolve the relative "../." give different results.
Because the shell script originally did "just"
relative="../."
if path/to/dir/ ends with slash, chop it off.
while $relative starts with "../";
do
chop off starting '../' of relative
chop of last '/' and following from "path/to/dir/."
done
(Linux:)
As B was made to A first, only C differs as a result, because
you had one more '/' in there.
(Windows:)
However Windows also detects '/.' (C) and makes it an A,
(in C only, because shell code was not treated Windows-sy)
which is where the incompatibility between Windows and other
platforms arises.
So we have a couple of choices (for Git)now:
* go back to using shell only for submodule things as that doesn't
have the regression and it alos plays nicely with Git for Windows.
* use C for the submodule code in Git and revert the regression fix
"because consistency across platforms trumps
consistency over time"
* use C for the submodule code in Git and keep the regression fix
"because consistency over time in Git proper is more important
than playing nicely with Git for Windows"
* would it be possible to revert this to shell on Windows only?
> Whether bash
> on Windows removes the dot at the end of C to turn it into B, as
> long as A and B give us the same result we wouldn't be hitting the
> problem, no?
Well in Git proper A,B are the same and C is different.
(B was fixed as a regression)
In Windows C is like B, which was different without the regression
fix, but now it is the same as A, too.
>
>>> > test_submodule_relative_url "(null)" "$PWD/subsuper_update_r" "../subsubsuper_update_r" "$(pwd)/subsubsuper_update_r"
>>> > test_submodule_relative_url "(null)" "$PWD/super_update_r2" "../subsuper_update_r" "$(pwd)/subsuper_update_r"
>>> > -test_submodule_relative_url "(null)" "$PWD/." "../." "$(pwd)/."
>>> > +test_submodule_relative_url "(null)" "$(pwd)/." "../." "$(pwd)/."
>
>>> > The reasons this is ugly: we specifically test for *Unixy* paths when we
>>> > use $PWD, as opposed to *Windowsy* paths when using $(pwd).
>
> Just to ensure I am following this correctly, two tests that come
> before the one you are touching above have $PWD on the input side
> and $(pwd) on the expectation side. That is what you mean by the
> next paragraph, right? They want to make sure that you honor the
> Unixy user input on Windows and still produce Windowsy result, that
> is.
>
>>> > We do this to
>>> > ensure a certain level of confidence that running things such as
>>> >
>>> > git clone --recurse-submodules /z/project/.
>>> >
>>> > work. And now that does not work anymore.
>
> And I agree from that point of view that having to spell both sides
> as $(pwd) would mean you are not testing that "Unixy input to
> Windowsy output" expectation, but at the same time, I think you
> would want "Windowsy input to Windowsy output" combination also does
> produce correct result, which is not tested in the three tests shown
> above. IOW, probably you would want to test both (at least on any
> platform where $PWD and $(pwd) textually disagree) for all these
> [*1*], and the pair
>
> "../." taken relative to "$(pwd)/." must be "$(pwd)/."
> "../." taken relative to "$PWD/." must be "$(pwd)/."
>
> test, because of the limitation of your bash, cannot have the latter
> half of the pair, so you'd need to comment it out with in-code
> explanation, perhaps? IOW something along the lines of...
>
> -- >8 -- snip -- >8 --
>
> test_submodule_relative_url "(null)" "$(pwd)/subsuper_update_r" "../subsubsuper_update_r" "$(pwd)/subsubsuper_update_r"
> test_submodule_relative_url "(null)" "$(pwd)/super_update_r2" "../subsuper_update_r" "$(pwd)/subsuper_update_r"
> test_submodule_relative_url "(null)" "$(pwd)/." "../." "$(pwd)/."
>
> if test_have_prereq MINGW
> then
>
> test_submodule_relative_url "(null)" "$PWD/subsuper_update_r" "../subsubsuper_update_r" "$(pwd)/subsubsuper_update_r"
> test_submodule_relative_url "(null)" "$PWD/super_update_r2" "../subsuper_update_r" "$(pwd)/subsuper_update_r"
> # This does not work correctly because Win-Bash strips . at the end
> # "of $PWD/."
> # test_submodule_relative_url "(null)" "$PWD/." "../." "$(pwd)/."
>
> fi
>
> -- >8 -- snip -- >8 --
>
> In any case, I find it more disturbing that we somehow ended up with
> a system where these three things are expected to behave differently:
>
> A - path/to/dir
> B - path/to/dir/
> C - path/to/dir/.
>
> Is that something we can fix?
Well A, B are the same.
C is "obviously" different, when it comes to counting slashes for relative
path/url purposes, in the way that there are characters after the last slash
and just by coincidence '.' refers to the directory itself, C behaving like
'path/to/dir/sub' seems right to me.
So how do you imagine this fix going forward?
* Breaking existing users with /. at the end? by treating it the same as A,B
* Do some check based on time/version of Git and cover the old data?
* Forbid /. at the end from now on?
>
>
> [Footnote]
>
> *1* It is tempting to update the above test sequence using
> a helper like:
>
> tsru () {
> test_submodule_relative_url "(null)" "$(pwd)/$1" "$2" "$(pwd)/$3"
> if test_have_prereq MINGW
> then
> test_submodule_relative_url "(null)" "$PWD/$1" "$2" "$(pwd)/$3"
> fi
> }
>
> then write the above three tests like so:
>
> tsru subsuper_update_r ../subsubsuper_update_r subsubsuper_update_r
> tsru super_update_r2 ../subsuper_update_r subsuper_update_r
> tsru . ../. .
>
> but you would want to disable the MINGW half for only the third
> test, we cannot quite do that.