git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
From: Jeff King <peff@peff.net>
To: Jonathan Nieder <jrnieder@gmail.com>
Cc: git@vger.kernel.org, Brandon Williams <bwilliams.eng@gmail.com>,
	Stefan Beller <sbeller@google.com>,
	Aaron Schrab <aaron@schrab.com>
Subject: Re: [RFC] submodule: munge paths to submodule git directories
Date: Thu, 17 Jan 2019 12:32:17 -0500
Message-ID: <20190117173216.GB27667@sigill.intra.peff.net> (raw)
In-Reply-To: <20190115012507.GK162110@google.com>

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@sigill.intra.peff.net/

  reply index

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-07 23:06 Brandon Williams
2018-08-07 23:25 ` Jonathan Nieder
2018-08-08  0:14 ` Junio C Hamano
2018-08-08 22:33 ` [PATCH 0/2] munge submodule names Brandon Williams
2018-08-08 22:33   ` [PATCH 1/2] submodule: create helper to build paths to submodule gitdirs Brandon Williams
2018-08-08 23:21     ` Stefan Beller
2018-08-09  0:45       ` Brandon Williams
2018-08-10 21:27     ` Junio C Hamano
2018-08-10 21:45       ` Brandon Williams
2018-08-08 22:33   ` [PATCH 2/2] submodule: munge paths to submodule git directories Brandon Williams
2018-08-09 21:26     ` Jeff King
2018-08-14 18:04       ` Brandon Williams
2018-08-14 18:57         ` Jonathan Nieder
2018-08-14 21:08           ` Stefan Beller
2018-08-14 21:12             ` Jonathan Nieder
2018-08-14 22:34               ` Stefan Beller
2018-08-16  2:34                 ` Jonathan Nieder
2018-08-16  2:39                   ` Stefan Beller
2018-08-16  2:47                     ` Jonathan Nieder
2018-08-16 17:34                       ` Brandon Williams
2018-08-16 18:19                       ` [PATCH] submodule: add config for where gitdirs are located Brandon Williams
2018-08-20 22:03                         ` Junio C Hamano
2018-08-16 15:07                   ` [PATCH 2/2] submodule: munge paths to submodule git directories Junio C Hamano
2018-08-14 18:58         ` Jeff King
2018-08-28 21:35         ` Stefan Beller
2018-08-29  5:25           ` Jeff King
2018-08-29 18:10             ` Stefan Beller
2018-08-29 21:03               ` Jeff King
2018-08-29 21:10                 ` Stefan Beller
2018-08-29 21:18                   ` Jonathan Nieder
2018-08-29 21:27                     ` Stefan Beller
2018-08-29 21:30                   ` Jeff King
2018-08-29 21:09             ` Jonathan Nieder
2018-08-29 21:14               ` Stefan Beller
2018-08-29 21:25                 ` Brandon Williams
2018-08-29 21:32               ` Jeff King
2018-08-16  0:19     ` Aaron Schrab
2019-01-15  1:25 ` [RFC] " Jonathan Nieder
2019-01-17 17:32   ` Jeff King [this message]
2019-01-17 17:57     ` Stefan Beller

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190117173216.GB27667@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=aaron@schrab.com \
    --cc=bwilliams.eng@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=jrnieder@gmail.com \
    --cc=sbeller@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox