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. > 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. > 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. > 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. [1] http://public-inbox.org/git/20180809212602.GA11342@xxxxxxxxxxxxxxxxxxxxx/
