git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Philippe Blain <levraiphilippeblain@gmail.com>
To: "brian m. carlson" <sandals@crustytoothpaste.net>, git@vger.kernel.org
Cc: Rose Kunkel <rose@rosekunkel.me>,
	Emily Shaffer <emilyshaffer@google.com>,
	Jonathan Tan <jonathantanmy@google.com>
Subject: Re: [PATCH] submodule: mark submodules with update=none as inactive
Date: Mon, 21 Jun 2021 23:45:45 -0400	[thread overview]
Message-ID: <56b5c722-8baf-9f9c-cc9f-5b5ed49d7fc3@gmail.com> (raw)
In-Reply-To: <20210619214449.2827705-1-sandals@crustytoothpaste.net>

Hi brian,

Le 2021-06-19 à 17:44, brian m. carlson a écrit :
> When the user recursively clones a repository with submodules and one or
> more of those submodules is marked with the submodule.<name>.update=none
> configuration, the submodule will end up being active.  This is a
> problem because we will have skipped cloning or checking out the
> submodule, and as a result, other commands, such as git reset or git
> checkout, will fail if they are invoked with --recurse-submodules (or
> when submodule.recurse is true).
> 
> This is obviously not the behavior the user wanted, so let's fix this by
> specifically setting the submodule as inactive in this case when we're
> cloning the repository. 

I'm not sure we want to fix it only at (recursive) clone time. The original bug report
was with 'git clone --recurse-submodules', but a non-recursive clone
followed by the usual 'git submodule init && git submodule update' (or
in one step 'git submodule update --init') would also lead to the same
bad experience (I'm not sure I want to call it a bug per se... it's certainly
a UX bug :)

> That will make us properly ignore the submodule
> when performing recursive operations.
> 
> Note that we only do this when the --require-init option is passed,
> which is only passed during clone.  That's because the update-clone
> submodule helper is also invoked during a user-invoked git submodule
> update, where we explicitly indicate that we override the configuration
> setting, so we don't want to set such a configuration option then.

I'm not sure what you mean here by 'where we explicitely indicate that we
override the configuration setting'. For me, as I wrote above,
'git clone --recurse-submodules' and 'git clone' followed by
'git submodule update --init' should lead to mostly [*] the same end result.

If you mean 'git submodule update --checkout', that indeed seems to sometimes override the 'update=none'
configuration (a fact which is absent from the documentation), then it's true that we
would not want to write 'active=false' at that invocation. As an aside, in my limited testing
I could not always get 'git submodule update --checkout' to clone and checkout 'update=none' submdules;
it would fail with "fatal: could not get a repository handle for submodule 'sub1'" because
'git checkout/reset --recurse-submodules' leaves a bad .git/modules/sub1/config file
with the core.worktree setting when the command fails (this should not happen)...

In any case, that leads me to think that maybe the right place to write the 'active' setting
would be during 'git submodule init', thus builtin/submodule--helper.c::init_submodule ?
This way it would lead to the same behaviour if the clone was recursive or not,
and it would not interfere with 'git submodule update --checkout'.

[*] I say 'mostly' because in the first case you end up with a 'submodule.active=.'
configuration, and no 'submodule.$name.active' configuration for each submodule,
whereas in the second case, there is no 'submodule.active' setting and
'submodule.$name.active' is true for each submodule.

> 
> Reported-by: Rose Kunkel <rose@rosekunkel.me>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
>   builtin/submodule--helper.c |  5 +++++
>   t/t5601-clone.sh            | 24 ++++++++++++++++++++++++
>   2 files changed, 29 insertions(+)

I think that such a change would warrant a mention in the doc, in
gitsubmodules [1], git-config [2], and probably git-submodule [3] if we agree that
'git submodule init' is the right place to set the 'active=false' flag.

Thanks for proposing a patch!

Philippe.

[1] https://git-scm.com/docs/gitmodules#Documentation/gitmodules.txt-submoduleltnamegtupdate
[2] https://git-scm.com/docs/git-config#Documentation/git-config.txt-submoduleltnamegtupdate
[3] https://git-scm.com/docs/git-submodule#Documentation/git-submodule.txt-init--ltpathgt82308203

  reply	other threads:[~2021-06-22  3:45 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-16  0:16 [BUG] `git reset --hard` fails with `update = none` submodules Rose Kunkel
2021-06-16  0:51 ` brian m. carlson
2021-06-16  0:57   ` Rose Kunkel
2021-06-16  1:03     ` Rose Kunkel
2021-06-16  1:15       ` Rose Kunkel
2021-06-16  1:25       ` brian m. carlson
2021-06-16  1:39         ` Rose Kunkel
2021-06-16  1:46           ` Rose Kunkel
2021-06-16  3:10         ` Junio C Hamano
2021-06-16 13:20           ` Philippe Blain
2021-06-17 23:52             ` brian m. carlson
2021-06-19 21:44               ` [PATCH] submodule: mark submodules with update=none as inactive brian m. carlson
2021-06-22  3:45                 ` Philippe Blain [this message]
2021-06-25 23:02                   ` brian m. carlson
2021-06-26 15:12                     ` Philippe Blain
2021-07-01 22:51               ` [PATCH v2] " brian m. carlson
2021-07-09 20:26                 ` Philippe Blain
2021-07-11 16:59                   ` brian m. carlson

Reply instructions:

You may reply publicly 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=56b5c722-8baf-9f9c-cc9f-5b5ed49d7fc3@gmail.com \
    --to=levraiphilippeblain@gmail.com \
    --cc=emilyshaffer@google.com \
    --cc=git@vger.kernel.org \
    --cc=jonathantanmy@google.com \
    --cc=rose@rosekunkel.me \
    --cc=sandals@crustytoothpaste.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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).