On Thu, Jan 17, 2019 at 9:32 AM Jeff King <peff@xxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx/
