git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Stefan Beller <sbeller@google.com>
To: Antonio Ospite <ao2@ao2.it>
Cc: git <git@vger.kernel.org>,
	Richard Hartmann <richih.mailinglist@gmail.com>
Subject: Re: [RFC 01/10] submodule: add 'core.submodulesFile' to override the '.gitmodules' path
Date: Thu, 12 Apr 2018 16:50:03 -0700
Message-ID: <CAGZ79kZV+MLaVp4AM77bJkT=MeBvDSHTxZCni8dkqq4VLWZ0AA@mail.gmail.com> (raw)
In-Reply-To: <20180412222047.5716-2-ao2@ao2.it>

Hi Antonio,

On Thu, Apr 12, 2018 at 3:20 PM, Antonio Ospite <ao2@ao2.it> wrote:
> When multiple repositories with detached work-trees take turns using the
> same directory as their work-tree, and more than one of them want to use
> submodules, there will be conflicts about the '.gitmodules' file.

unlike other files which would not conflict?
There might be file names such as LICENSE, Readme.md etc,
which are common enough that they would produce conflicts as well?
I find this argument on its own rather weak. ("Just delete everything in
the working dir before using it with another repository"). I might be
missing a crucial bit here?

> git hardcodes this path so it's not possible to override its location on
> a per-repository basis to allow such repositories to coexists
> peacefully.
>
> Make the path of the "gitmodules file" customizable exposing
> a 'core.submodulesFile' configuration setting.
>
> The default value will still be '.gitmodules' when 'core.submodulesFile'
> is not set.

ok.


> --- a/cache.h
> +++ b/cache.h
> @@ -1774,6 +1774,7 @@ extern void prepare_pager_args(struct child_process *, const char *pager);
>  extern const char *editor_program;
>  extern const char *askpass_program;
>  extern const char *excludes_file;
> +extern const char *submodules_file;

Could you place this variable in repository.h in struct repository?
(Some developers currently try to move any global state to that place,
as that makes working with e.g. nested submodules easier in-process
and you would not need to spawn processes for submodules)

Once migrated to the repository struct mentioned above, you'd access
it via the_repository->submodules_file for the main repository.


> diff --git a/git-submodule.sh b/git-submodule.sh
> index 24914963c..610fd0dc5 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -71,7 +71,9 @@ get_submodule_config () {
>         value=$(git config submodule."$name"."$option")
>         if test -z "$value"
>         then
> -               value=$(git config -f .gitmodules submodule."$name"."$option")
> +               gitmodules_file=$(git config core.submodulesfile)
> +               : ${gitmodules_file:=.gitmodules}
> +               value=$(git config -f "$gitmodules_file" submodule."$name"."$option")

I wonder if it would be cheaper to write a special config lookup now, e.g.
in builtin/submodule--helper.c we could have a "config-from-gitmodules"
subcommand that is looking up the modules file and then running the config
on that file.

I am surprised how little access of the .gitmodules is left in git-submodule.sh
(which is partially ported to the builtin/submodule--helper.c)

> diff --git a/submodule-config.c b/submodule-config.c
> index 3f2075764..8a3396ade 100644
> --- a/submodule-config.c
> +++ b/submodule-config.c
> @@ -468,7 +468,7 @@ static int gitmodule_oid_from_commit(const struct object_id *treeish_name,
>                 return 1;
>         }
>
> -       strbuf_addf(rev, "%s:.gitmodules", oid_to_hex(treeish_name));
> +       strbuf_addf(rev, "%s:%s", oid_to_hex(treeish_name), submodules_file);
>         if (get_oid(rev->buf, gitmodules_oid) >= 0)
>                 ret = 1;
>
> @@ -583,7 +583,7 @@ void repo_read_gitmodules(struct repository *repo)
>                 if (repo_read_index(repo) < 0)
>                         return;
>
> -               gitmodules = repo_worktree_path(repo, GITMODULES_FILE);
> +               gitmodules = repo_worktree_path(repo, submodules_file);
>
>                 if (!is_gitmodules_unmerged(repo->index))
>                         git_config_from_file(gitmodules_cb, gitmodules, repo);
> diff --git a/submodule.c b/submodule.c
> index 9a50168b2..2afbdb644 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -36,13 +36,13 @@ static struct oid_array ref_tips_after_fetch;
>   */
>  int is_gitmodules_unmerged(const struct index_state *istate)
>  {
> -       int pos = index_name_pos(istate, GITMODULES_FILE, strlen(GITMODULES_FILE));
> +       int pos = index_name_pos(istate, submodules_file, strlen(submodules_file));

Ah, regarding the coverletter: This clearly assumes the modules
file is in the tree. So at least here we would make an exception
for files outside the tree to either not check for un-merged-ness or
disallow that case entirely.



There are quite a few functions in submodule.c which access the new global. :/
So moving them to the_repository should be fine, but eventually (not
in this series)
all these functions would would want to take a repository argument as well
such that they work on more than the_repository.

Thanks,
Stefan

  reply index

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-12 22:20 [RFC 00/10] Make .the gitmodules file path configurable Antonio Ospite
2018-04-12 22:20 ` [RFC 01/10] submodule: add 'core.submodulesFile' to override the '.gitmodules' path Antonio Ospite
2018-04-12 23:50   ` Stefan Beller [this message]
2018-04-16 16:37     ` Antonio Ospite
2018-04-16 21:22       ` Stefan Beller
2018-04-18 11:43         ` Antonio Ospite
2018-04-18 18:44           ` Stefan Beller
2018-04-12 22:20 ` [RFC 02/10] submodule: fix getting custom gitmodule file in fetch command Antonio Ospite
2018-04-12 23:55   ` Stefan Beller
2018-04-16 16:18     ` Antonio Ospite
2018-04-16 19:23       ` Stefan Beller
2018-04-16 20:46         ` Antonio Ospite
2018-04-12 22:20 ` [RFC 04/10] submodule: document 'core.submodulesFile' and fix references to '.gitmodules' Antonio Ospite
2018-04-12 22:20 ` [RFC 05/10] submodule: adjust references to '.gitmodules' in comments Antonio Ospite
2018-04-12 22:20 ` [RFC 06/10] completion: add 'core.submodulesfile' to the git-completion.bash file Antonio Ospite
2018-04-12 23:36 ` [RFC 00/10] Make .the gitmodules file path configurable Stefan Beller
2018-04-16 11:33   ` Antonio Ospite
2018-04-16 19:22     ` Stefan Beller
2018-04-13  8:07 ` Antonio Ospite
2018-04-13  8:07 ` [RFC 07/10] FIXME: wrap-for-bin.sh: set 'core.submodulesFile' for each git invocation Antonio Ospite
2018-04-13  8:07   ` [RFC 08/10] FIXME: submodule: fix t1300-repo-config.sh to take into account the new config Antonio Ospite
2018-04-13  8:07   ` [RFC 09/10] FIXME: submodule: pass custom gitmodules file to 'test-tool submodule-config' Antonio Ospite
2018-04-13  8:07   ` [RFC 10/10] FIXME: add a hacky script to test the changes with a patched test suite Antonio Ospite
2018-04-13 20:05   ` [RFC 07/10] FIXME: wrap-for-bin.sh: set 'core.submodulesFile' for each git invocation Stefan Beller
2018-04-16 11:36     ` Antonio Ospite

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='CAGZ79kZV+MLaVp4AM77bJkT=MeBvDSHTxZCni8dkqq4VLWZ0AA@mail.gmail.com' \
    --to=sbeller@google.com \
    --cc=ao2@ao2.it \
    --cc=git@vger.kernel.org \
    --cc=richih.mailinglist@gmail.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 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