git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Stefan Beller <sbeller@google.com>
To: Jeff King <peff@peff.net>
Cc: Brandon Williams <bmwill@google.com>, Junio C Hamano <gitster@pobox.com>, "git@vger.kernel.org" <git@vger.kernel.org>
Subject: Re: [PATCH 1/2] add: warn when adding an embedded repository
Date: Wed, 14 Jun 2017 10:53:12 -0700
Message-ID: <CAGZ79kaN=XVe3OWE5DHsMfbzW_rZOdRurgSfpz52dSZDA_V6fg@mail.gmail.com> (raw)
In-Reply-To: <20170614063614.a34ovimjpz2g24qe@sigill.intra.peff.net>

On Tue, Jun 13, 2017 at 11:36 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Jun 13, 2017 at 10:07:43AM -0700, Stefan Beller wrote:
>
>> > I also waffled on whether we should ask the submodule code
>> > whether it knows about a particular path. Technically:
>> >
>> >   git config submodule.foo.path foo
>> >   git config submodule.foo.url git://...
>> >   git add foo
>> >
>> > is legal, but would still warn with this patch. I don't know
>> > how much we should care (it would also be easy to do on
>> > top).
>>
>> And here I was thinking this is not legal, because you may override
>> anything *except* submodule.*.path in the config. That is because
>> all the other settings (such as url, active flag, branch,
>> shallow recommendation) are dependent on the use case, the user,
>> changes to the environment (url) or such. The name<->path mapping
>> however is only to be changed via changes to the tracked content.
>> That is why it would make sense to disallow overriding the path
>> outside the tracked content.
>
> It was probably a mistake to use normal config as the example. Junio
> mentioned it as a case that could work if you communicate the submodule
> URL to somebody else out-of-band. My understanding was that you could
> set whatever you like in the regular config, but I think that is just
> showing my ignorance of submodules.
>
> Pretend like I said "-f .gitmodules" in each line above. ;)
>
>> In my ideal dream world of submodules we would have the following:
>>
>>   $ cat .gitmodules
>>   [submodule "sub42"]
>>     path = foo
>>   # path only in tree!
>
> TBH, I am not sure why we need "path"; couldn't we just use the
> subsection name as an implicit path?

That is what was done back in the time. But then people wanted to rename
submodules (i.e. move them around in the worktree), so the path is not
constant, so either we'd have to move around the git dir whenever the
submodule is renamed (bad idea IMO), or instead introduce a mapping
between (constant name <-> variable path). So that was done.

Historically (IIUC) we had submodule.path.url which then was changed
to submodule.name.url + name->path resolution. And as a hack(?) or
easy way out of a problem then, the name is often the same as the path
hence confusing people, when they see:

    [submodule "foo"]
        path = foo
        url = dadada/foo

What foo means what now? ;)
As a tangent: I want to make the default name different to the path.

So yeah, we want to keep the name and not mingle with implicit path.

I think we may even have bugs in our code base where the
name/path confusion shows.

Talking about another tangent:

  For files there is a rename detection available. For submodules
  It is hard to imagine that there ever will be such a rename detection
  as files have because of the explciit name<->path mapping.

  We *know* when a submodule was moved. So why even try
  to do rename detection? As we record only sha1s for a submodule
  you could swap two submodule object names by accident.
  Consider a superproject that contains different kernels, such as
  a kernel for your phone/embedded device and then a kernel for
  your workstation or other device. And these two kernels are different
  for technical reasons but share the same history.

  Now the inattentive user may make a mistake and git-add the
  "wrong" kernel submodule.  The smart Git would tell that it is a
  rename/move just as we have with files.

>
>> > +       OPT_HIDDEN_BOOL(0, "warn-embedded-repo", &warn_on_embedded_repo,
>> > +                       N_("warn when adding an embedded repository")),
>>
>> We do not have a lot of OPT_HIDDEN_BOOLs throughout the code base.
>> We should use them more often.
>>
>> It makes sense though in this case.
>
> Actually, my main reason is that it's nonsense to show
> "--warn-embedded-repo" in the help, when it's already the default. I
> would like to have written:
>
>   OPT_NEGBOOL(0, "no-warn-embedded-repo", &warn_on_embedded_repo,
>                 N_("disable warning when adding an embedded repository"))
>
> but we don't have such a thing (and the last discussion on it a few
> months ago left a lot of open questions). So given that this really
> isn't something I'd expect users to want, I figured hiding it was a good
> idea. I mentioned it in the manpage for script writers, but it's really
> not worth cluttering "git add -h".

ok :) If you really wanted, you could go with a raw OPTION though. ;)
This is fine with me though.

>
>> > +static const char embedded_advice[] = N_(
>> > +"You've added another git repository inside your current repository.\n"
>> > +"Clones of the outer repository will not also contain the contents of\n"
>> > +"the embedded repository. If you meant to add a submodule, use:\n"
>>
>> The "will not also" sounds a bit off to me. Maybe:
>>   ...
>>   Clones of the outer repository will not contain the contents
>>   of the embedded repository and has no way of knowing how
>>   to obtain the inner repo. If you meant to add a submodule ...
>
> Yeah, I think we could just strike the "also" (I played around with the
> wording here quite a bit and I think it was left from an earlier attempt
> where it made more sense).
>
> Your "no way of knowing" is probably a good thing to mention.
>
>> > +"See \"git help submodule\" for more information."
>>
>> Once the overhaul of the submodule documentation
>> comes along[1], we rather want to point at
>> "man 7 git-submodules", which explains the concepts and
>> then tell you about commands how to use it. For now the
>> git-submodule man page is ok.
>>
>> [1] https://public-inbox.org/git/20170607185354.10050-1-sbeller@google.com/
>
> Yeah, I poked around looking for a definitive "here's how submodules
> work" intro. I'm happy one is in the works, and I agree this should
> point there once it exists.
>
>> > +++ b/t/t7414-submodule-mistakes.sh
>> > @@ -0,0 +1,40 @@
>> > +#!/bin/sh
>> > +
>> > +test_description='handling of common mistakes people may make with submodules'
>>
>> That is one way to say it. Do we have other tests for
>> "you think it is a bug, but it is features" ? ;)
>> I like it though. :)
>
> Heh. I didn't know how else to lump it together. Just "test git add on a
> repository" felt like too little for its own script. I almost added it
> to t7400, but I think that script is plenty long enough as it is (it's
> also one of the longest-running scripts, I think).

Thanks for not doing that. :)

  parent reply index

Thread overview: 23+ messages in thread (expand / mbox.gz / Atom feed / [top])
2017-06-06  3:56 [BUG?] gitlink without .gitmodules no longer fails recursive clone Jeff King
2017-06-06 18:01 ` Stefan Beller
2017-06-06 18:10   ` Brandon Williams
2017-06-06 18:39     ` Jeff King
2017-06-09 23:19       ` Jeff King
2017-06-10  2:10         ` Junio C Hamano
2017-06-10  7:13           ` Jeff King
2017-06-10 11:12             ` Junio C Hamano
2017-06-12  5:30           ` Stefan Beller
2017-06-13  9:14         ` Jeff King
2017-06-13  9:24           ` [PATCH 1/2] add: warn when adding an embedded repository Jeff King
2017-06-13 17:07             ` Stefan Beller
2017-06-13 17:16               ` Brandon Williams
2017-06-14  6:36               ` Jeff King
2017-06-14 10:54                 ` [PATCH v2 0/2] " Jeff King
2017-06-14 10:58                   ` [PATCH v2 1/2] add: " Jeff King
2017-06-14 10:58                   ` [PATCH v2 2/2] t: move "git add submodule" into test blocks Jeff King
2017-06-14 17:53                 ` Stefan Beller [this message]
2017-06-15  6:01                   ` [PATCH 1/2] add: warn when adding an embedded repository Jeff King
2017-06-13 17:16             ` Junio C Hamano
2017-06-14  6:38               ` Jeff King
2017-06-13  9:24           ` [PATCH 2/2] t: move "git add submodule" into test blocks Jeff King
2017-06-13 17:15             ` 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 to all the recipients using the --to, --cc,
  and --in-reply-to switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAGZ79kaN=XVe3OWE5DHsMfbzW_rZOdRurgSfpz52dSZDA_V6fg@mail.gmail.com' \
    --to=sbeller@google.com \
    --cc=bmwill@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.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 mailing list mirror (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