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

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/

      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
2019-01-17 17:57     ` Stefan Beller [this message]

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='CAGZ79kbofESBSTHbDdPmeZgb2Pwz=8FVmtXG6x376utMyS0vqA@mail.gmail.com' \
    --to=sbeller@google.com \
    --cc=aaron@schrab.com \
    --cc=bwilliams.eng@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=jrnieder@gmail.com \
    --cc=peff@peff.net \
    /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/
       or Tor2web: https://www.tor2web.org/

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