Re: [RFC] submodule: munge paths to submodule git directories

[Stefan Beller <sbeller@google.com>]

On Thu, Jan 17, 2019 at 9:32 AM Jeff King <peff@peff.net> wrote:
>
> On Mon, Jan 14, 2019 at 05:25:07PM -0800, Jonathan Nieder wrote:
>
> > I've put a summary in https://crbug.com/git/28 to make this easier to
> > pick up where we left off.  Summary from there of the upstream review:
> >
> > 1. Using urlencoding to escape the slashes is fine, but what if we
> >    want to escape some other character (for example to handle
> >    case-insensitive filesystems)?
> >
> >    Proposal: Store the escaping mapping in config[1] so it can be
> >    modified it in the future:
> >
> >       [submodule "plugin/hooks"]
> >               gitdirname = plugins%2fhooks
>
> I think it might be worth dealing with case-sensitivity _now_, since we
> know it's a problem. That doesn't make the problem of "what if we want
> to change the mapping later" go away, but it does make it a lot less
> likely to come up.

Makes sense.

>
> > 2. The urlencoded name could conflict with a submodule that has % in
> >    its name in an existing clone created by an older version of Git.
> >
> >    Proposal: Put submodules in a new .git/submodules/ directory
> >    instead of .git/modules/.
>
> This proposal is orthogonal to (1), right? I.e., if we store the mapping
> then that is what tells us we're using the mapped name.

Technically true, but it allows for easier implementation:
now we have 2 distinct namespaces, such that we can avoid
double booking easier:

    Consider 2 submodules "a b" and "a%20b".

Without (2), (1) is hard to explain as the first might have been encoded
to a%20b or there might have been the second put in place from a
current (old) version of Git. So we'd have to reason about these corner cases.

With (2) in place, we'd only ever have the second in a place "a%2520b"
(if I am to trust https://www.urlencoder.org/)

> > 3. These gitdirname settings can clutter up .git/config.
> >
> >    Proposal: For the "easy" cases (e.g. submodule name consisting of
> >    [a-z]*), allow omitting the gitdirname setting.
>
> Not having thought about it too hard, I suspect that may open back up
> corner cases with respect to backwards compatibility and ambiguity.
>
> Are you worried about human-readable clutter? I.e., that .git/config
> becomes hard to read? If so, then:
>
>   - I doubt this is any worse than the existing tracking-branch config.
>
>   - it might be reasonable to store it in .git/submodule-config, and
>     make sure that .git/config contains a single "[include]path =
>     submodule-config" line. I've been tempted to do that for
>     tracking-branch config.
>
> Or are you worried about the cost of parsing those entries? Basically
> every git command parses config linearly at least once; this normally
> isn't noticeable, but at some size it becomes a problem. I have no idea
> what that size is.
>
> If so, then I think we'd want submodule config in its own file but
> _without_ an include from the normal config file. That would break
> compatibility with anything that tries to use "git config
> submodule.foo.path", etc.
>
> That's all just musing. I'm actually not really convinced it's a
> problem.

ok, we can deal with the problem once it arises.

>
> > Is that a fair summary?  Are there concerns from the review that I
> > forgot, or would a new version of the series that addresses those
> > three problems put us in good shape?
>
> I don't really have a strong opinion either way. I still think the
> one-way transformation that the patch uses is less elegant than a real
> encode/decode round-trip (i.e., what I discussed in [1]). But I admit to
> not having thought through all of the details of the encode/decode
> thing, and certainly have not written the code.

The suggestion of adding at least a test for url encoding (2. from your mail)
is sensible.

Stefan

>
> [1] http://public-inbox.org/git/20180809212602.GA11342@sigill.intra.peff.net/