threads / patch / 38237

patchRe: [PATCH] git-new-workdir: support submodules

Subject: Re: [PATCH] git-new-workdir: support submodules

## tl;dr

11 messages between Dec 23, 2014 and Jan 28, 2015. Diffs are folded; open one to read it.

replies: 10people: 4as markdown or json

Craig Silverstein· Dec 23, 2014, 21:51 UTC · lore

[Ack, I forgot to cc myself on the original patch so now I can't reply to it normally. Hopefully my workaround doesn't mess up the threading too badly.]

Junio C Hamano <gitster <at> pobox.com> writes:
Show 18 quoted lines
>
> Hmmmm, does that mean that the submodule S in the original
> repository O's working tree and its checkout in the secondary
> working tree W created from O using git-new-workdir share the same
> repository location?  More specifically:
>
>     O/.git/                 - original repository
>         O/.git/index            - worktree state in O
>         O/S                     - submodule S's checkout in O
>         O/S/.git                - a gitfile pointing to O/.git/modules/S
>     O/.git/modules/S        - submodule S's repository contents
>         O/.git/modules/S/config - submodule S's config
>
>     W/.git/                 - secondary working tree
>         W/.git/config           - symlink to O/.git/config
>         W/.git/index            - worktree state in W (independent of O)
>     W/S                     - submodule S's checkout in W (independent of O)
>     W/S/.git                - a gitfile pointing to O/.git/modules/S

Right until the last line. The .git file holds a relative path (at least, it always does in my experience), so W/S/.git will point to W/.git/modules/S.

Also, to complete the story, my changes sets the following:
        W/.git/modules/S    - secondary working tree for S
             W/.git/modules/S/config   - symlink to O/.git/modules/S/config
             W/.git/modules/S/index    - worktree state in W's S
(independent of O and O's S)
> Doesn't a submodule checkout keep some state tied to the working
> tree in its repository configuration file?
Do you mean, in 'config' itself?  If so, I don't see it.  (Though it's
possible there are ways to use submodules that do keep working-tree
state in the config file, and we just happen not to use those ways.)
Here's what my webapp/.git/modules/khan-exercises/config looks like:
---
[core]
        repositoryformatversion = 0
        filemode = true
        bare = false
        logallrefupdates = true
        worktree = ../../../khan-exercises
[remote "origin"]
        url = http://github.com/Khan/khan-exercises.git
        fetch = +refs/heads/*:refs/remotes/origin/*
[branch "master"]
        remote = origin
        merge = refs/heads/master
        rebase = true
[submodule "test/qunit"]
        url = https://github.com/jquery/qunit.git
--

The only thing that seems vaguely working-tree related is the 'worktree' field, which is the field that motivated me to set up my patch the way it is.

> Wouldn't this change
> introduce problems by sharing O/.git/modules/S/config between the
> two checkouts?

It's true that this change does result in sharing that file, so if that's problematic then you're right. I'm afraid I don't know all the things that can go into a submodule config file.

(There are other things I don't know as well, such as: do we see .git files with 'gitdir: ...' contents only for submodules, or are there other ways to create them as well? Are 'gitdir' paths always relative? Are there special files in .git (or rather .git/modules/S) that exist only for submodules and not for 'normal' repos, that we need to worry about symlinking? I apologize for not knowing all these git internals, and hope you guys can help point out any gaps that affect this patch!)

craig
Craig Silverstein· Jan 24, 2015, 00:48 UTC · re: Craig Silverstein · lore
Ping! (now that the holidays are past)
craig

On Tue, Dec 23, 2014 at 1:51 PM, Craig Silverstein <csilvers@khanacademy.org> wrote:

Show 82 quoted lines
> [Ack, I forgot to cc myself on the original patch so now I can't reply
> to it normally.  Hopefully my workaround doesn't mess up the threading
> too badly.]
>
> Junio C Hamano <gitster <at> pobox.com> writes:
>>
>> Hmmmm, does that mean that the submodule S in the original
>> repository O's working tree and its checkout in the secondary
>> working tree W created from O using git-new-workdir share the same
>> repository location?  More specifically:
>>
>>     O/.git/                 - original repository
>>         O/.git/index            - worktree state in O
>>         O/S                     - submodule S's checkout in O
>>         O/S/.git                - a gitfile pointing to O/.git/modules/S
>>     O/.git/modules/S        - submodule S's repository contents
>>         O/.git/modules/S/config - submodule S's config
>>
>>     W/.git/                 - secondary working tree
>>         W/.git/config           - symlink to O/.git/config
>>         W/.git/index            - worktree state in W (independent of O)
>>     W/S                     - submodule S's checkout in W (independent of O)
>>     W/S/.git                - a gitfile pointing to O/.git/modules/S
>
> Right until the last line.  The .git file holds a relative path (at
> least, it always does in my experience), so W/S/.git will point to
> W/.git/modules/S.
>
> Also, to complete the story, my changes sets the following:
>
>         W/.git/modules/S    - secondary working tree for S
>              W/.git/modules/S/config   - symlink to O/.git/modules/S/config
>              W/.git/modules/S/index    - worktree state in W's S
> (independent of O and O's S)
>
>> Doesn't a submodule checkout keep some state tied to the working
>> tree in its repository configuration file?
>
> Do you mean, in 'config' itself?  If so, I don't see it.  (Though it's
> possible there are ways to use submodules that do keep working-tree
> state in the config file, and we just happen not to use those ways.)
> Here's what my webapp/.git/modules/khan-exercises/config looks like:
> ---
> [core]
>         repositoryformatversion = 0
>         filemode = true
>         bare = false
>         logallrefupdates = true
>         worktree = ../../../khan-exercises
> [remote "origin"]
>         url = http://github.com/Khan/khan-exercises.git
>         fetch = +refs/heads/*:refs/remotes/origin/*
> [branch "master"]
>         remote = origin
>         merge = refs/heads/master
>         rebase = true
> [submodule "test/qunit"]
>         url = https://github.com/jquery/qunit.git
> --
>
> The only thing that seems vaguely working-tree related is the
> 'worktree' field, which is the field that motivated me to set up my
> patch the way it is.
>
>> Wouldn't this change
>> introduce problems by sharing O/.git/modules/S/config between the
>> two checkouts?
>
> It's true that this change does result in sharing that file, so if
> that's problematic then you're right.  I'm afraid I don't know all the
> things that can go into a submodule config file.
>
> (There are other things I don't know as well, such as: do we see .git
> files with 'gitdir: ...' contents only for submodules, or are there
> other ways to create them as well?  Are 'gitdir' paths always
> relative?  Are there special files in .git (or rather .git/modules/S)
> that exist only for submodules and not for 'normal' repos, that we
> need to worry about symlinking?  I apologize for not knowing all these
> git internals, and hope you guys can help point out any gaps that
> affect this patch!)
>
> craig
Junio C Hamano· Jan 24, 2015, 01:37 UTC · re: Craig Silverstein · lore
Craig Silverstein <csilvers@khanacademy.org> writes:
Show 28 quoted lines
>>> Doesn't a submodule checkout keep some state tied to the working
>>> tree in its repository configuration file?
>>
>> Do you mean, in 'config' itself?  If so, I don't see it.  (Though it's
>> possible there are ways to use submodules that do keep working-tree
>> state in the config file, and we just happen not to use those ways.)
>> Here's what my webapp/.git/modules/khan-exercises/config looks like:
>> ---
>> [core]
>>         repositoryformatversion = 0
>>         filemode = true
>>         bare = false
>>         logallrefupdates = true
>>         worktree = ../../../khan-exercises
>> [remote "origin"]
>>         url = http://github.com/Khan/khan-exercises.git
>>         fetch = +refs/heads/*:refs/remotes/origin/*
>> [branch "master"]
>>         remote = origin
>>         merge = refs/heads/master
>>         rebase = true
>> [submodule "test/qunit"]
>>         url = https://github.com/jquery/qunit.git
>> --
>>
>> The only thing that seems vaguely working-tree related is the
>> 'worktree' field, which is the field that motivated me to set up my
>> patch the way it is.

That is the location of the working tree of the top-level superproject. Tied to the state of the submodule working tree appear in [submodule "test/qunit"] part.

In one new-workdir checkout, that submodule may be "submodule init"ed, while another one, it may not be.

Or one new-workdir checkout's branch may check out a top-level project from today while the other one may have a top-level project from two years ago, and between these two checkouts of the top-level project, the settings of submodule."test/qunit".* variables may have to be different (perhaps even URL may have to point at two different repositories, one historical one to grab the state two years ago, the other current one).

So sharing config between top-level checkouts may not be enough to "support submodules" (the patch title).

Craig Silverstein· Jan 25, 2015, 01:47 UTC · re: Junio C Hamano · lore

} Or one new-workdir checkout's branch may check out a top-level } project from today while the other one may have a top-level project } from two years ago,

This is also true, but just as much a problem with the 'git new-workdir' script as it existed before my change. It already symlinks the top-level .git/config directory, which lists a remote, submodules, and many other things. Does symlinking the config file for submodules add any new wrinkles, that symlinking the config file for the top-level repository does not?

craig
On Fri, Jan 23, 2015 at 5:37 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 48 quoted lines
> Craig Silverstein <csilvers@khanacademy.org> writes:
>
>>>> Doesn't a submodule checkout keep some state tied to the working
>>>> tree in its repository configuration file?
>>>
>>> Do you mean, in 'config' itself?  If so, I don't see it.  (Though it's
>>> possible there are ways to use submodules that do keep working-tree
>>> state in the config file, and we just happen not to use those ways.)
>>> Here's what my webapp/.git/modules/khan-exercises/config looks like:
>>> ---
>>> [core]
>>>         repositoryformatversion = 0
>>>         filemode = true
>>>         bare = false
>>>         logallrefupdates = true
>>>         worktree = ../../../khan-exercises
>>> [remote "origin"]
>>>         url = http://github.com/Khan/khan-exercises.git
>>>         fetch = +refs/heads/*:refs/remotes/origin/*
>>> [branch "master"]
>>>         remote = origin
>>>         merge = refs/heads/master
>>>         rebase = true
>>> [submodule "test/qunit"]
>>>         url = https://github.com/jquery/qunit.git
>>> --
>>>
>>> The only thing that seems vaguely working-tree related is the
>>> 'worktree' field, which is the field that motivated me to set up my
>>> patch the way it is.
>
> That is the location of the working tree of the top-level
> superproject.  Tied to the state of the submodule working tree
> appear in [submodule "test/qunit"] part.
>
> In one new-workdir checkout, that submodule may be "submodule
> init"ed, while another one, it may not be.
>
> Or one new-workdir checkout's branch may check out a top-level
> project from today while the other one may have a top-level project
> from two years ago, and between these two checkouts of the top-level
> project, the settings of submodule."test/qunit".* variables may have
> to be different (perhaps even URL may have to point at two different
> repositories, one historical one to grab the state two years ago,
> the other current one).
>
> So sharing config between top-level checkouts may not be enough to
> "support submodules" (the patch title).
Junio C Hamano· Jan 26, 2015, 04:05 UTC · re: Craig Silverstein · lore
Craig Silverstein <csilvers@khanacademy.org> writes:
Show 10 quoted lines
> } Or one new-workdir checkout's branch may check out a top-level
> } project from today while the other one may have a top-level project
> } from two years ago,
>
> This is also true, but just as much a problem with the 'git
> new-workdir' script as it existed before my change.  It already
> symlinks the top-level .git/config directory, which lists a remote,
> submodules, and many other things.  Does symlinking the config file
> for submodules add any new wrinkles, that symlinking the config file
> for the top-level repository does not?

The update under discussion is labeled as "support submodules"; presumably the only reason that such an update is good is because it will fix existing problems that makes the use of the script break when submodules are involved --- submodules are not supported with the current code, and this patch fixes the code to support it.

But then, you are saying that the update does not fix these existing issues around submodule support. So...?

Craig Silverstein· Jan 26, 2015, 04:57 UTC · re: Junio C Hamano · lore
> But then, you are saying that the update does not fix these existing
> issues around submodule support.  So...?

I guess my point is that the existing contrib script has proven to be useful to people, even though it imposes these constraints on clients wrt the config file (namely, you can't have multiple workdirs that need different values in the config file). This patch, in adding submodule support, I expect would be similarly useful to people even though it, also, imposes those same constraints to the submodule's config files.

I guess you'd rather see these config file issues fixed for all use cases? If so, I'm probably not the right person since I do not know enough about how config files are used in git -- I fear any changes I made would make some things worse for (some) existing clients of the script, which is not what I want. It sounds like this functionality is being reimplemented in git proper in any case, so perhaps it's best just to wait for that. I don't know what its submodule support will be, though.

craig
Junio C Hamano· Jan 26, 2015, 05:39 UTC · re: Craig Silverstein · lore
Craig Silverstein <csilvers@khanacademy.org> writes:
> This patch, in adding submodule support, I expect would be
> similarly useful to people even though it, also, imposes those
> same constraints to the submodule's config files.

I would expect that you would see exactly the same issue with Duy's multiple work tree series. This is not limited to new-workdir.

The right way to look at this is to fix what "git submodule" does; its use of "config" that is shared across branches is the root cause of the trouble. No other part of Git keeps data that needs to be per-branch (or more specifically "tied to the working tree state") in .git/config in such a way that leaving it stale when the working tree state changes breaks the system.

One way to do so might be to move the bits it stores in the config file to somewhere else that is more closely tied to the checkout state and handle that similar to .git/index and .git/HEAD when it comes to multiple work-trees.

Jens Lehmann· Jan 27, 2015, 17:35 UTC · re: Junio C Hamano · lore
Am 26.01.2015 um 06:39 schrieb Junio C Hamano:
Show 15 quoted lines
> Craig Silverstein <csilvers@khanacademy.org> writes:
>
>> This patch, in adding submodule support, I expect would be
>> similarly useful to people even though it, also, imposes those
>> same constraints to the submodule's config files.
>
> I would expect that you would see exactly the same issue with Duy's
> multiple work tree series.  This is not limited to new-workdir.
>
> The right way to look at this is to fix what "git submodule" does;
> its use of "config" that is shared across branches is the root cause
> of the trouble.  No other part of Git keeps data that needs to be
> per-branch (or more specifically "tied to the working tree state")
> in .git/config in such a way that leaving it stale when the working
> tree state changes breaks the system.

I'm not sure what submodule config shared across branches you are referring to here, the only two things that submodules by default store in .git/config are the url (to mark the submodule initialized) and the update setting (which I believe should be copied there in the first place, but changing that in a correct and backwards compatible would take some effort). This means you'll init (or deinit) a submodule in all multiple worktrees at the same time, but having the url setting in .git/config is a feature, not a bug.

And if you are talking about the core.worktree setting in .git/modules/<submodule_name>/config, I proposed way to get rid of that when we moved the submodule .git directory there (but that wasn't accepted):

    http://permalink.gmane.org/gmane.comp.version-control.git/188007

Additionally I suspect submodules are just the first to suffer, other worktree specific config (like e.g. EOL settings) affecting all multiple worktrees at the same time when changed in a single worktree might easily confuse other users too.

> One way to do so might be to move the bits it stores in the config
> file to somewhere else that is more closely tied to the checkout
> state and handle that similar to .git/index and .git/HEAD when it
> comes to multiple work-trees.

Yup, the idea of separating config entries into worktree and repo specific files sounds like the solution for these problems.

Duy Nguyen· Jan 28, 2015, 10:50 UTC · re: Jens Lehmann · lore
On Tue, Jan 27, 2015 at 06:35:41PM +0100, Jens Lehmann wrote:
Show 7 quoted lines
> > One way to do so might be to move the bits it stores in the config
> > file to somewhere else that is more closely tied to the checkout
> > state and handle that similar to .git/index and .git/HEAD when it
> > comes to multiple work-trees.
> 
> Yup, the idea of separating config entries into worktree and repo
> specific files sounds like the solution for these problems.

OK, would this work? This PoC patch adds a config key namespace local.* that is written to $GIT_DIR/config.local instead of $GIT_DIR/config (I still need to ban local.* in global config files). This config.local is loaded togethr with $GIT_DIR/config.

For nd/multiple-work-trees we can transparently map $GIT_DIR/config.local to $SUPER/worktrees/<id>/config.local. For git-new-workdir.sh, perhaps we can teach include.path to make config.local path relative to where the config symlink is, not where the symlink target is.

We can then teach setup.c to read local.core.worktree if core.worktree is not present, and more apdation in git-submodule.sh.. I don't expect big changes because git-submodule.sh just needs to read some other config keys and dont need to care if git-new-workdir.sh or nd/multiple-work-trees is being used.

-- 8< --
Show changes to config.c +26 −3
diff --git a/config.c b/config.c
index 752e2e2..237bd8e 100644
--- a/config.c
+++ b/config.c
@@ -1177,6 +1177,15 @@ int git_config_system(void)
 	return !git_env_bool("GIT_CONFIG_NOSYSTEM", 0);
 }
 
+static int config_local_filter(const char *var, const char *value, void *data)
+{
+	struct config_include_data *inc = data;
+
+	if (!starts_with(var, "local."))
+		return error("$GIT_DIR/config.local can only contain local.*");
+	return inc->fn(var, value, inc->data);
+}
+
 int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 {
 	int ret = 0, found = 0;
@@ -1202,8 +1211,19 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 	}
 
 	if (repo_config && !access_or_die(repo_config, R_OK, 0)) {
+		char *wt_config;
 		ret += git_config_from_file(fn, repo_config, data);
 		found += 1;
+		wt_config = git_pathdup("config.local");
+		if (!access_or_die(wt_config, R_OK, 0)) {
+			struct config_include_data inc = CONFIG_INCLUDE_INIT;
+			inc.fn = fn;
+			inc.data = data;
+			ret += git_config_from_file(config_local_filter,
+						    wt_config, &inc);
+			found += 1;
+		}
+		free(wt_config);
 	}
 
 	switch (git_config_from_parameters(fn, data)) {
@@ -1942,8 +1962,12 @@ int git_config_set_multivar_in_file(const char *config_filename,
 
 	store.multi_replace = multi_replace;
 
-	if (!config_filename)
-		config_filename = filename_buf = git_pathdup("config");
+	if (!config_filename) {
+		if (starts_with(key, "local."))
+			config_filename = filename_buf = git_pathdup("config.local");
+		else
+			config_filename = filename_buf = git_pathdup("config");
+	}
 
 	/*
 	 * The lock serves a purpose in addition to locking: the new
-- 8< --
Duy Nguyen· Jan 28, 2015, 10:53 UTC · re: Duy Nguyen · lore
On Wed, Jan 28, 2015 at 5:50 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> For git-new-workdir.sh, perhaps we can teach include.path to make
> config.local path relative to where the config symlink is, not where
> the symlink target is.

Ignore this part. I originally wanted to use include.path to load config.local, but there was an easier way. With this patch, I think config.local is already per worktree that is created by git-new-workdir.sh.

-- 
Duy
Junio C Hamano· Jan 28, 2015, 20:18 UTC · re: Duy Nguyen · lore
Duy Nguyen <pclouds@gmail.com> writes:
Show 9 quoted lines
> On Wed, Jan 28, 2015 at 5:50 PM, Duy Nguyen <pclouds@gmail.com> wrote:
>> For git-new-workdir.sh, perhaps we can teach include.path to make
>> config.local path relative to where the config symlink is, not where
>> the symlink target is.
>
> Ignore this part. I originally wanted to use include.path to load
> config.local, but there was an easier way. With this patch, I think
> config.local is already per worktree that is created by
> git-new-workdir.sh.

I think this may give us a cleaner approach to support a "keep multiple working trees from the same project that are configured differently" setting.

I am not sure how well it meshes to call this as ".local" with the "--local" option of "git config" (read: "local" is a loaded word, and the way we have used it have been "per repository", but now you are using it to mean "per working tree" that is finer grained), though.

I hope that there aren't people that are crazy enough to bind the same repository to two different places in the top-level project, and insist that we (re)use the submodule repository ;-).

← back to recent threads