Re: [PATCH] worktree: add per-worktree config files
On Mon, Sep 24, 2018 at 4:21 PM Taylor Blau wrote:
> > @@ -602,6 +605,7 @@ int cmd_config(int argc, const char **argv, const char
> > *prefix)
> >PARSE_OPT_STOP_AT_NON_OPTION);
> >
> > if (use_global_config + use_system_config + use_local_config +
> > + use_worktree_config +
> > !!given_config_source.file + !!given_config_source.blob > 1) {
>
> I feel like we're getting into "let's extract a function" territory,
> here, since this line is growing in width. Perhaps:
>
> static int config_files_count()
> {
> return use_global_config + use_system_config + use_local_config +
> use_worktree_config +
> !!given_config_source.file +
> !!given_config_source.blob;
> }
>
> Simplifying the call to:
I think there's a bigger problem here because we have no good way to
describe that a bunch of options are mutually exclusive. We get by ok
if the number of options is two, but when it gets bigger (and this is
not the only place), it gets messier. Besides the long "if" condition,
I consider the error message "only one config file at a time"
inadequate. We should tell the user what exact options are
conflicting.
So, I'm going to bury my head in the sand this time and ignore the
problem, including adding config_files_count(). It could be a
interesting mini project if someone wants to jump in. Meanwhile it'll
go to the very bottom of my long long backlog.
--
Duy
Re: [PATCH] worktree: add per-worktree config files
On Thu, Sep 27, 2018 at 8:34 PM Ævar Arnfjörð Bjarmason wrote: > I see I'm misremembering most of the details here. I thought that if I put: > > [remote "whatever] > url = ... > > Into my ~/.gitconfig that it wouldn't work, but it does, e.g. here in my > ~/g/git: > > $ grep -A1 whatever .git/config > $ > $ grep -A1 whatever ~/.gitconfig > [remote "whatever"] > url = g...@github.com:test/git.git > > But there's still some special casing for .git/config going on, > e.g. here: > > $ git config remote.origin.url > g...@github.com:git/git.git > $ git config remote.whatever.url > g...@github.com:test/git.git > $ git remote get-url origin > g...@github.com:git/git.git > $ git remote get-url whatever > fatal: No such remote 'whatever' > > And: > > $ git remote set-url whatever g...@github.com:test2/git.git > fatal: No such remote 'whatever' > > So there is some special casing of .git/config somewhere. I looked into > this ages ago, and don't remember where that's done. To conclude this thread. Yes some code does know about where the config variable is from and it looks like only "git remote" and "git upload-pack" takes advantage of it [1] [2]. I considered documentation improvement for the git-remote part, but I don't see anywhere I can fit "some remote attributes can be shared from ~/.gitconfig, but a remote can only be added from repo-level config" in. Jeff already documented it well. I found one minor problem there in the documentation, which I will send separately. Thanks again for making me aware of this. [1] e459b073fb (remote rename: more carefully determine whether a remote is configured - 2017-01-19) [2] 20b20a22f8 (upload-pack: provide a hook for running pack-objects - 2016-05-18) -- Duy
Re: [PATCH] worktree: add per-worktree config files
On Thu, Sep 27, 2018 at 8:34 PM Ævar Arnfjörð Bjarmason wrote: > ... > > So there is some special casing of .git/config somewhere. I looked into > this ages ago, and don't remember where that's done. Thanks! At least know I have some clues to look into (and will do). -- Duy
Re: [PATCH] worktree: add per-worktree config files
On Thu, Sep 27 2018, Duy Nguyen wrote: > On Wed, Sep 26, 2018 at 8:25 PM Ævar Arnfjörð Bjarmason > wrote: >> On Sun, Sep 23 2018, Nguyễn Thái Ngọc Duy wrote: >> >> > +extensions.worktreeConfig:: >> > + If set, by default "git config" reads from both "config" and >> > + "config.worktree" file in that order. >> >> How does this interact with config that's now only used if it's in >> .git/config? E.g. you can't set remote.. in ~/.gitconfig, >> will that be inherited across the two of these? > > Er... we can't? If I remember correctly we don't have any enforcement > on where what config vars must or must not go. The only exception is > core.bare and core.worktree which is only read from $GIT_DIR/config > because of the way they are involved in .git directory discovery. If I > put remote "foo" in ~/.gitconfig, "git remote" happily reports remote > "foo" to me. > > To sum up, we always inherit config from higher levels, with > /etc/gitconfig being the highest and $GIT_DIR/config the lowest. It's > up to the user to share config between repos responsibly. This patch > only adds one more level, $GIT_DIR/config.worktree which is now the > lowest level. I see I'm misremembering most of the details here. I thought that if I put: [remote "whatever] url = ... Into my ~/.gitconfig that it wouldn't work, but it does, e.g. here in my ~/g/git: $ grep -A1 whatever .git/config $ $ grep -A1 whatever ~/.gitconfig [remote "whatever"] url = g...@github.com:test/git.git But there's still some special casing for .git/config going on, e.g. here: $ git config remote.origin.url g...@github.com:git/git.git $ git config remote.whatever.url g...@github.com:test/git.git $ git remote get-url origin g...@github.com:git/git.git $ git remote get-url whatever fatal: No such remote 'whatever' And: $ git remote set-url whatever g...@github.com:test2/git.git fatal: No such remote 'whatever' So there is some special casing of .git/config somewhere. I looked into this ages ago, and don't remember where that's done. I was wondering if these patches introduced any unwanted edge cases in this regard, e.g. if you're using the per-worktree config, and you have remotes in .git/config, does "git remote get/set-url" do the right thing? Then if we have remotes in .git/config, should we add new remotes to .git/config or the per-worktree file? I'd lean towards .git/config, since remotes are orthagonal to worktrees, and closely tied with refs which are shared no matter what this extension says, but maybe there's a good argument for doing it the other way around. >> > In multiple working >> > + directory mode, "config" file is shared while >> > + "config.worktree" is per-working directory. >> >> "But then how will it work with more than one?" I found myself thinking >> before reading some more and remembering .git/worktree. Shouldn't we >> consistently say: >> >> [...]"config" and "worktrees//config"[...] >> >> Or something like that? > > Point taken. Maybe I'm trying to hide implementation details too much.
Re: [PATCH] worktree: add per-worktree config files
On Wed, Sep 26, 2018 at 8:25 PM Ævar Arnfjörð Bjarmason wrote: > On Sun, Sep 23 2018, Nguyễn Thái Ngọc Duy wrote: > > > +extensions.worktreeConfig:: > > + If set, by default "git config" reads from both "config" and > > + "config.worktree" file in that order. > > How does this interact with config that's now only used if it's in > .git/config? E.g. you can't set remote.. in ~/.gitconfig, > will that be inherited across the two of these? Er... we can't? If I remember correctly we don't have any enforcement on where what config vars must or must not go. The only exception is core.bare and core.worktree which is only read from $GIT_DIR/config because of the way they are involved in .git directory discovery. If I put remote "foo" in ~/.gitconfig, "git remote" happily reports remote "foo" to me. To sum up, we always inherit config from higher levels, with /etc/gitconfig being the highest and $GIT_DIR/config the lowest. It's up to the user to share config between repos responsibly. This patch only adds one more level, $GIT_DIR/config.worktree which is now the lowest level. > > In multiple working > > + directory mode, "config" file is shared while > > + "config.worktree" is per-working directory. > > "But then how will it work with more than one?" I found myself thinking > before reading some more and remembering .git/worktree. Shouldn't we > consistently say: > > [...]"config" and "worktrees//config"[...] > > Or something like that? Point taken. Maybe I'm trying to hide implementation details too much. -- Duy
Re: [PATCH] worktree: add per-worktree config files
On Sun, Sep 23 2018, Nguyễn Thái Ngọc Duy wrote: > +extensions.worktreeConfig:: > + If set, by default "git config" reads from both "config" and > + "config.worktree" file in that order. How does this interact with config that's now only used if it's in .git/config? E.g. you can't set remote.. in ~/.gitconfig, will that be inherited across the two of these? > In multiple working > + directory mode, "config" file is shared while > + "config.worktree" is per-working directory. "But then how will it work with more than one?" I found myself thinking before reading some more and remembering .git/worktree. Shouldn't we consistently say: [...]"config" and "worktrees//config"[...] Or something like that?
Re: [PATCH] worktree: add per-worktree config files
Duy Nguyen writes: > I believe the main selling point of multiple worktrees is sharing > refs. You could easily avoid expensive clones with --local, but > synchronizing between different clones is not very convenient. Other > than that, different worktrees tend to behave like separate clones. OK. Even with the enforced limitation that no single branch can be checked out in multiple worktrees at the same time, it is more convenient as you can "merge" other branch and trust that the result on the checked-out branch in your worktree is immediately visible to other worktrees. > user has multiple worktrees, but the extension is not enabled. I'm Exactly. "config --worktree" in your script will silently break other worktrees; it wanted to affect only the current worktree, but it changed settings to all the others. > probably not clear in the commit message, but "git config" can detect > that the extension has not been enabled, automatically enable it (and > move core.bare and core.worktree (if present) to the main worktree's > private config), so "git config --worktree" will never share the > change. I wonder if that is good enough. The user in one worktree did "config --worktree" and all the worktrees now start diverging in their config---but that is true _only_ when they use "--worktree" option to say "I want this to be set differently from others". All the other calls to "git config" will stil be shared. So from a cursory thinking, it sounds OK to me, but somebody else may think of bad ramifications after a good night sleep.
Re: [PATCH] worktree: add per-worktree config files
On Tue, Sep 25, 2018 at 11:26 PM Junio C Hamano wrote: > Warning: devil's advocate mode on. Oh good. I need to a good dose of sanity anyway. > Done in either way, this will confuse the users. What is the reason > why people are led to think it is a good idea to use multiple > worktrees, even when they need different settings? What do they > want out of "multiple worktrees linked to a single repository" as > opposed to just a simple "as many clones as necessary"? Reduced > disk footprint? I believe the main selling point of multiple worktrees is sharing refs. You could easily avoid expensive clones with --local, but synchronizing between different clones is not very convenient. Other than that, different worktrees tend to behave like separate clones. This leaves a gray area where other things should be shared or not. I think the preference (or default mode) is still _not_ shared (*). Sharing more things (besides refs and object database) is just a new opportunity popping up when we implement multiple worktrees. Since multiple worktrees (or clones before its time) are grouped together, sometimes you would like to share common configuration. We could sort of achieve this already with includeIf but again not as convenient. (*) real life is not that simple. Since refs are shared, including _remotes_ refs, so configuration related to remotes should also be shared, or it will be a maintenance nightmare. Which is why $GIT_DIR/config so far has been shared besides the two exceptions that are core.bare and core.worktree. And I really like to get rid of these exceptions. > Is there a better way to achieve that without the > downside of multiple worktrees (e.g. configuration need to be > uniform)? Is there a better way to achieve sharing refs between clones? I gave it a minute but couldn't come up with a good answer :( > > (*) "git config --worktree" points back to "config" file when this > > extension is not present so that it works in any setup. > > Shouldn't it barf and error out instead? The intention is a uniform interface/api that works with both single and multiple worktrees configurations. Otherwise in your scripts you would need to write "if single worktree, do this, else do that". If "git config --worktree" works with both, the existing scripts can be audited and updated just a bit, adding "--worktree" where the config should not be shared, and we're done. > A user who hasn't enabled > the extension uses --worktree option and misled to believe that the > setting affects only a single worktree, even though the change is > made globally---that does not sound like a great end-user experience. I was talking about a single worktree. But I think here you meant the user has multiple worktrees, but the extension is not enabled. I'm probably not clear in the commit message, but "git config" can detect that the extension has not been enabled, automatically enable it (and move core.bare and core.worktree (if present) to the main worktree's private config), so "git config --worktree" will never share the change. But perhaps the user should be made aware of this situation and asked to explicitly enable the extension instead? It's certainly a more conservative approach. -- Duy
Re: [PATCH] worktree: add per-worktree config files
Nguyễn Thái Ngọc Duy writes: > This extension is most useful in multiple worktree setup because you > now have an option to store per-worktree config (which is either > .git/config.worktree for main worktree, or > .git/worktrees/xx/config.worktree for linked ones). Heh. "This is useful if you have multiple" is true without saying, because this is totally useless if you have only a single worktree. I'd suggest writing the above without "most useful". > This design places a bet on the assumption that the majority of config > variables are shared so it is the default mode. A safer move would be > default writes go to per-worktree file, so that accidental changes are > isolated. Warning: devil's advocate mode on. Done in either way, this will confuse the users. What is the reason why people are led to think it is a good idea to use multiple worktrees, even when they need different settings? What do they want out of "multiple worktrees linked to a single repository" as opposed to just a simple "as many clones as necessary"? Reduced disk footprint? Is there a better way to achieve that without the downside of multiple worktrees (e.g. configuration need to be uniform)? > (*) "git config --worktree" points back to "config" file when this > extension is not present so that it works in any setup. Shouldn't it barf and error out instead? A user who hasn't enabled the extension uses --worktree option and misled to believe that the setting affects only a single worktree, even though the change is made globally---that does not sound like a great end-user experience.
Re: [PATCH] worktree: add per-worktree config files
On Mon, Sep 24, 2018 at 4:21 PM Taylor Blau wrote:
> > +cmp_config() {
> > + if [ "$1" = "-C" ]; then
> > + shift &&
> > + GD="-C $1" &&
> > + shift
> > + else
> > + GD=
> > + fi &&
> > + echo "$1" >expected &&
> > + shift &&
> > + git $GD config "$@" >actual &&
> > + test_cmp expected actual
> > +}
>
> This cmp_config seems generally useful, perhaps beyond t2029. What do
> you think about putting it in t/test-lib-functions.sh instead?
Good point. t1300 (and I think some t13xx) does the same. Other tests also do
test "$(git config...)" = blah
which can also use this function to have better error reporting when
things go wrong.
--
Duy
Re: [PATCH] worktree: add per-worktree config files
On Sun, Sep 23, 2018 at 07:04:38PM +0200, Nguyễn Thái Ngọc Duy wrote:
> A new repo extension is added, worktreeConfig. When it is present:
I was initially scratching my head at why this should be a repository
extension, but...
> - The special treatment for core.bare and core.worktree, to stay
>effective only in main worktree, is gone. These config files are
>supposed to be in config.worktree.
This seems to be sufficient justification for it. A destructive action
(such as migrating configuration from one location to another, as you
have implemented in the patch below) should be something that the user
opts into, rather than having happen automatically.
> This extension is most useful in multiple worktree setup because you
> now have an option to store per-worktree config (which is either
> .git/config.worktree for main worktree, or
> .git/worktrees/xx/config.worktree for linked ones).
This sounds quite useful for these situations.
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 8d85d1a324..c24abf5871 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2,8 +2,9 @@ CONFIGURATION FILE
> --
>
> The Git configuration file contains a number of variables that affect
> -the Git commands' behavior. The `.git/config` file in each repository
> -is used to store the configuration for that repository, and
> +the Git commands' behavior. The files `.git/config` and optionally
> +`config.worktree` (see `extensions.worktreeConfig` below) are each
> +repository is used to store the configuration for that repository, and
Typo: 'are each'.
> ENVIRONMENT
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> index e2ee9fc21b..3f9112db56 100644
> --- a/Documentation/git-worktree.txt
> +++ b/Documentation/git-worktree.txt
> @@ -204,6 +204,43 @@ working trees, it can be used to identify worktrees. For
> example if
> you only have two working trees, at "/abc/def/ghi" and "/abc/def/ggg",
> then "ghi" or "def/ghi" is enough to point to the former working tree.
>
> +CONFIGURATION FILE
> +--
> +By default, the repository "config" file is shared across all working
> +directories. If the config variables `core.bare` or `core.worktree`
> +are already present in the config file, they will be applied to the
> +main working directory only.
> +
> +In order to have configuration specific to working directories, you
> +can turn on "worktreeConfig" extension, e.g.:
> +
> +
> +$ git config extensions.worktreeConfig true
> +
Good, this matches my expectation from above.
> @@ -24,6 +25,7 @@ static char key_delim = ' ';
> static char term = '\n';
>
> static int use_global_config, use_system_config, use_local_config;
> +static int use_worktree_config;
> static struct git_config_source given_config_source;
> static int actions, type;
> static char *default_value;
> @@ -123,6 +125,7 @@ static struct option builtin_config_options[] = {
> OPT_BOOL(0, "global", _global_config, N_("use global config file")),
> OPT_BOOL(0, "system", _system_config, N_("use system config file")),
> OPT_BOOL(0, "local", _local_config, N_("use repository config
> file")),
> + OPT_BOOL(0, "worktree", _worktree_config, N_("use per-worktree
> config file")),
> OPT_STRING('f', "file", _config_source.file, N_("file"), N_("use
> given config file")),
> OPT_STRING(0, "blob", _config_source.blob, N_("blob-id"),
> N_("read config from given blob object")),
> OPT_GROUP(N_("Action")),
> @@ -602,6 +605,7 @@ int cmd_config(int argc, const char **argv, const char
> *prefix)
>PARSE_OPT_STOP_AT_NON_OPTION);
>
> if (use_global_config + use_system_config + use_local_config +
> + use_worktree_config +
> !!given_config_source.file + !!given_config_source.blob > 1) {
I feel like we're getting into "let's extract a function" territory,
here, since this line is growing in width. Perhaps:
static int config_files_count()
{
return use_global_config + use_system_config + use_local_config +
use_worktree_config +
!!given_config_source.file +
!!given_config_source.blob;
}
Simplifying the call to:
> diff --git a/t/t2029-worktree-config.sh b/t/t2029-worktree-config.sh
> new file mode 100755
> index 00..4ebdf13cf9
> --- /dev/null
> +++ b/t/t2029-worktree-config.sh
> @@ -0,0 +1,82 @@
> +#!/bin/sh
> +
> +test_description="config file in multi worktree"
> +
> +. ./test-lib.sh
> +
> +cmp_config() {
> + if [ "$1" = "-C" ]; then
> + shift &&
> + GD="-C $1" &&
> + shift
> + else
> + GD=
> + fi &&
> + echo "$1" >expected &&
> + shift &&
> + git $GD config "$@" >actual &&
> + test_cmp expected actual
> +}
This cmp_config seems generally useful, perhaps beyond t2029. What do
you think about putting it in
Re: [PATCH] worktree: add per-worktree config files
On Sun, Sep 23, 2018 at 1:04 PM Nguyễn Thái Ngọc Duy wrote:
> A new repo extension is added, worktreeConfig. When it is present:
>
> - Repository config reading by default includes $GIT_DIR/config _and_
>$GIT_DIR/config.worktree. "config" file remains shared in multiple
>worktree setup.
>
> - The special treatment for core.bare and core.worktree, to stay
>effective only in main worktree, is gone. These config files are
>supposed to be in config.worktree.
"These config _settings_ are supposed to be..."
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> @@ -2,8 +2,9 @@ CONFIGURATION FILE
> The Git configuration file contains a number of variables that affect
> -the Git commands' behavior. The `.git/config` file in each repository
> -is used to store the configuration for that repository, and
> +the Git commands' behavior. The files `.git/config` and optionally
> +`config.worktree` (see `extensions.worktreeConfig` below) are each
s/are/in/
> +repository is used to store the configuration for that repository, and
> `$HOME/.gitconfig` is used to store a per-user configuration as
> diff --git a/t/t2029-worktree-config.sh b/t/t2029-worktree-config.sh
> @@ -0,0 +1,82 @@
> +cmp_config() {
> + if [ "$1" = "-C" ]; then
Style:
if test "$1" = "-C"
then
...
> +}
> +
> +test_expect_success 'setup' '
> + test_commit start &&
> + git config --worktree per.worktree is-ok &&
Did you want:
cmp_config false extensions.worktreeConfig
at this point in the test or something? It's not clear what the
intention is here.
> + git worktree add wt1 &&
> + git worktree add wt2 &&
> + git config --worktree per.worktree is-ok &&
> + cmp_config true extensions.worktreeConfig
> +'
> +test_expect_success 'core.bare no longer for main only' '
> + git config core.bare true &&
> + cmp_config true core.bare &&
> + cmp_config -C wt1 true core.bare &&
> + cmp_config -C wt2 true core.bare &&
> + git config --unset core.bare
Should this be?
test_when_finished "git config --unset core.bare" &&
or perhaps test_config()?
> +'
[PATCH] worktree: add per-worktree config files
A new repo extension is added, worktreeConfig. When it is present: - Repository config reading by default includes $GIT_DIR/config _and_ $GIT_DIR/config.worktree. "config" file remains shared in multiple worktree setup. - The special treatment for core.bare and core.worktree, to stay effective only in main worktree, is gone. These config files are supposed to be in config.worktree. This extension is most useful in multiple worktree setup because you now have an option to store per-worktree config (which is either .git/config.worktree for main worktree, or .git/worktrees/xx/config.worktree for linked ones). This extension can be used in single worktree mode, even though it's pretty much useless (but this can happen after you remove all linked worktrees and move back to single worktree). "git config" reads from both "config" and "config.worktree" by default (i.e. without either --user, --file...) when this extension is present. Default writes still go to "config", not "config.worktree". A new option --worktree is added for that (*). Since a new repo extension is introduced, existing git binaries should refuse to access to the repo (both from main and linked worktrees). So they will not misread the config file (i.e. skip the config.worktree part). They may still accidentally write to the config file anyway if they use with "git config --file ". This design places a bet on the assumption that the majority of config variables are shared so it is the default mode. A safer move would be default writes go to per-worktree file, so that accidental changes are isolated. (*) "git config --worktree" points back to "config" file when this extension is not present so that it works in any setup. Signed-off-by: Nguyễn Thái Ngọc Duy --- Documentation/config.txt | 11 +++- Documentation/git-config.txt | 26 +--- Documentation/git-worktree.txt | 37 Documentation/gitrepository-layout.txt | 8 +++ builtin/config.c | 16 - cache.h| 2 + config.c | 7 +++ environment.c | 1 + setup.c| 5 +- t/t2029-worktree-config.sh | 82 ++ worktree.c | 41 + worktree.h | 6 ++ 12 files changed, 231 insertions(+), 11 deletions(-) create mode 100755 t/t2029-worktree-config.sh diff --git a/Documentation/config.txt b/Documentation/config.txt index 8d85d1a324..c24abf5871 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -2,8 +2,9 @@ CONFIGURATION FILE -- The Git configuration file contains a number of variables that affect -the Git commands' behavior. The `.git/config` file in each repository -is used to store the configuration for that repository, and +the Git commands' behavior. The files `.git/config` and optionally +`config.worktree` (see `extensions.worktreeConfig` below) are each +repository is used to store the configuration for that repository, and `$HOME/.gitconfig` is used to store a per-user configuration as fallback values for the `.git/config` file. The file `/etc/gitconfig` can be used to store a system-wide default configuration. @@ -371,6 +372,12 @@ advice.*:: editor input from the user. -- +extensions.worktreeConfig:: + If set, by default "git config" reads from both "config" and + "config.worktree" file in that order. In multiple working + directory mode, "config" file is shared while + "config.worktree" is per-working directory. + core.fileMode:: Tells Git if the executable bit of files in the working tree is to be honored. diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt index 8e240435be..4870e00b89 100644 --- a/Documentation/git-config.txt +++ b/Documentation/git-config.txt @@ -45,13 +45,15 @@ unset an existing `--type` specifier with `--no-type`. When reading, the values are read from the system, global and repository local configuration files by default, and options -`--system`, `--global`, `--local` and `--file ` can be -used to tell the command to read from only that location (see <>). +`--system`, `--global`, `--local`, `--worktree` and +`--file ` can be used to tell the command to read from only +that location (see <>). When writing, the new value is written to the repository local configuration file by default, and options `--system`, `--global`, -`--file ` can be used to tell the command to write to -that location (you can say `--local` but that is the default). +`--worktree`, `--file ` can be used to tell the command to +write to that location (you can say `--local` but that is the +default). This command will fail with non-zero status upon error. Some exit codes are: @@ -131,6 +133,11 @@ from all available files. + See also <>. +--worktree:: +
