Re: [PATCH 1/2] submodule: ignore trailing slash on superproject URL

From: Stefan Beller <sbeller@google.com>
Date: Oct 17, 2016, 17:58 UTC
Message-ID: <CAGZ79kYW2qK1GKxoKy_mkVkjjqEUzkh5aPSzDEfRd6U=PYDdzw@mail.gmail.com>
In-Reply-To: <xmqqfunvxxgu.fsf@gitster.mtv.corp.google.com>

On Mon, Oct 17, 2016 at 12:10 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 23 quoted lines

> Johannes Schindelin <Johannes.Schindelin@gmx.de> 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.

Show 66 quoted lines

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

Show 23 quoted lines

>
>
> [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.