Using url.insteadOf in git-clone

Using url.insteadOf in git-clone

[Pieter de Bie]

Hi,

I sometimes use url.insteadOf to create a shortcut to a central  
repository.
For example, having something like[*1*]

[url "git://repo.or.cz/git/"]
	insteadOf = "repo:"

in my global gitconfig allows me to do a 'git fetch repo:dscho.git'.  
I'd also
like to use that with git clone :). Currently if I try that, I get

Vienna:~ pieter$ git clone repo:dscho.git
Initialize dscho/.git
Initialized empty Git repository in /Users/pieter/dscho/.git/
ssh: Error resolving hostname repo: nodename nor servname provided, or  
not known
fatal: The remote end hung up unexpectedly

Which I think comes from the fact that the global config isn't read in  
by
remote.c when running git clone.

Now, I seem to remember there was a good reason for this (both builtin- 
clone
and git-clone.sh have the same behaviour). Also, trying to quickly  
hack in
something failed for me, even though it seems like a trivial change to  
make
for someone more familiar with the code path.

Is there an easy fix for this?

- Pieter

[1] This is not really true. If I try that, I get an error:
Vienna:git pieter$ git fetch -v repo:dscho.git
fatal: I don't handle protocol 'it'

If I change the url to "ggit://.." it does work. It seems there is an  
off-by-one
error somewhere? It does work for another url.insteadOf I have in my  
global
config though..

[PATCH 1/2] clone: respect the settings in $HOME/.gitconfig and /etc/gitconfig

[Johannes Schindelin]

After initializing the config in the newly-created repository, we
need to unset GIT_CONFIG so that the global configs are read again.

Noticed by Pieter de Bie.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	On Fri, 27 Jun 2008, Pieter de Bie wrote:

	> I sometimes use url.insteadOf to create a shortcut to a central 
	> repository. For example, having something like[*1*]
	> 
	> [url "git://repo.or.cz/git/"]
	> 	insteadOf = "repo:"
	> 
	> in my global gitconfig allows me to do a 'git fetch 
	> repo:dscho.git'. I'd also like to use that with git clone :).
	> Currently if I try that, I get
	> 
	> Vienna:~ pieter$ git clone repo:dscho.git
	> Initialize dscho/.git
	> Initialized empty Git repository in /Users/pieter/dscho/.git/
	> ssh: Error resolving hostname repo: nodename nor servname provided,
	>	or not known
	> fatal: The remote end hung up unexpectedly
	> 
	> [...]
	>
	> Is there an easy fix for this?

	Yes ;-)

 builtin-clone.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/builtin-clone.c b/builtin-clone.c
index 17baa20..965b5fc 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -424,6 +424,13 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	fprintf(stderr, "Initialize %s\n", git_dir);
 	init_db(option_template, option_quiet ? INIT_DB_QUIET : 0);
 
+	/*
+	 * At this point, the config exists, so we do not need the
+	 * environment variable.  We actually need to unset it, too, to
+	 * re-enable parsing of the global configs.
+	 */
+	unsetenv(CONFIG_ENVIRONMENT);
+
 	if (option_reference)
 		setup_reference(git_dir);
 
-- 
1.5.6.173.gde14c

[PATCH 2/2] clone: respect url.insteadOf setting in global configs

[Johannes Schindelin]

When we call "git clone" with a url that has a rewrite rule in either
$HOME/.gitconfig or /etc/gitconfig, the URL can be different from
what the command line expects it to be.

So, let's use the URL as the remote structure has it, not the literal
string from the command line.

Noticed by Pieter de Bie.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin-clone.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/builtin-clone.c b/builtin-clone.c
index 965b5fc..8dda52a 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -463,7 +463,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		refs = clone_local(path, git_dir);
 	else {
 		struct remote *remote = remote_get(argv[0]);
-		struct transport *transport = transport_get(remote, argv[0]);
+		struct transport *transport =
+			transport_get(remote, remote->url[0]);
 
 		if (!transport->get_refs_list || !transport->fetch)
 			die("Don't know how to clone %s", transport->url);
-- 
1.5.6.173.gde14c

Re: [PATCH 1/2] clone: respect the settings in $HOME/.gitconfig and /etc/gitconfig

[Daniel Barkalow]

On Fri, 27 Jun 2008, Johannes Schindelin wrote:

> After initializing the config in the newly-created repository, we
> need to unset GIT_CONFIG so that the global configs are read again.

This seems fine to me. OTOH, I'm not sure the environment variable should 
be needed in the first place; I think the config stuff should look in 
git_path("config") without it, and we set the git dir to the one we're 
initializing. So I think the use of the environment variable is just an 
artifact of how the shell script did it and how I was originally calling 
the init_db stuff.

Just removing the "setenv()" line survives all of the tests for me, and I 
remember some of them failing before I'd gotten some sort of solution for 
the config stuff.

> Noticed by Pieter de Bie.

You'd think I would have noticed this, since I mainly decided to convert 
clone to C so that url.insteadOf in global config would let you do "git 
clone work:modulename", but I clearly never tested it.

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> 
> 	On Fri, 27 Jun 2008, Pieter de Bie wrote:
> 
> 	> I sometimes use url.insteadOf to create a shortcut to a central 
> 	> repository. For example, having something like[*1*]
> 	> 
> 	> [url "git://repo.or.cz/git/"]
> 	> 	insteadOf = "repo:"
> 	> 
> 	> in my global gitconfig allows me to do a 'git fetch 
> 	> repo:dscho.git'. I'd also like to use that with git clone :).
> 	> Currently if I try that, I get
> 	> 
> 	> Vienna:~ pieter$ git clone repo:dscho.git
> 	> Initialize dscho/.git
> 	> Initialized empty Git repository in /Users/pieter/dscho/.git/
> 	> ssh: Error resolving hostname repo: nodename nor servname provided,
> 	>	or not known
> 	> fatal: The remote end hung up unexpectedly
> 	> 
> 	> [...]
> 	>
> 	> Is there an easy fix for this?
> 
> 	Yes ;-)
> 
>  builtin-clone.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/builtin-clone.c b/builtin-clone.c
> index 17baa20..965b5fc 100644
> --- a/builtin-clone.c
> +++ b/builtin-clone.c
> @@ -424,6 +424,13 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  	fprintf(stderr, "Initialize %s\n", git_dir);
>  	init_db(option_template, option_quiet ? INIT_DB_QUIET : 0);
>  
> +	/*
> +	 * At this point, the config exists, so we do not need the
> +	 * environment variable.  We actually need to unset it, too, to
> +	 * re-enable parsing of the global configs.
> +	 */
> +	unsetenv(CONFIG_ENVIRONMENT);
> +
>  	if (option_reference)
>  		setup_reference(git_dir);
>  
> -- 
> 1.5.6.173.gde14c
> 
> 

Re: [PATCH 2/2] clone: respect url.insteadOf setting in global configs

[Daniel Barkalow]

On Fri, 27 Jun 2008, Johannes Schindelin wrote:

> When we call "git clone" with a url that has a rewrite rule in either
> $HOME/.gitconfig or /etc/gitconfig, the URL can be different from
> what the command line expects it to be.
> 
> So, let's use the URL as the remote structure has it, not the literal
> string from the command line.

This is how it should be.

Maybe I ought to make transport_get() take the index of the URL in the 
list for the remote, instead of taking the actual URL; any use of a URL 
that's not from that list is going to be oddly wrong in some way, I think.

Thanks for taking care of this.

> Noticed by Pieter de Bie.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin-clone.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/builtin-clone.c b/builtin-clone.c
> index 965b5fc..8dda52a 100644
> --- a/builtin-clone.c
> +++ b/builtin-clone.c
> @@ -463,7 +463,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  		refs = clone_local(path, git_dir);
>  	else {
>  		struct remote *remote = remote_get(argv[0]);
> -		struct transport *transport = transport_get(remote, argv[0]);
> +		struct transport *transport =
> +			transport_get(remote, remote->url[0]);
>  
>  		if (!transport->get_refs_list || !transport->fetch)
>  			die("Don't know how to clone %s", transport->url);
> -- 
> 1.5.6.173.gde14c
> 
> 
> 

Re: Using url.insteadOf in git-clone

[Junio C Hamano]

Pieter de Bie <pdebie@ai.rug.nl> writes:

> [1] This is not really true. If I try that, I get an error:
> Vienna:git pieter$ git fetch -v repo:dscho.git
> fatal: I don't handle protocol 'it'
>
> If I change the url to "ggit://.." it does work. It seems there is an
> off-by-one
> error somewhere? It does work for another url.insteadOf I have in my
> global
> config though..

What version of git is this?

v1.5.5.1 and later contains 60e3aba (Fix config key miscount in
url.*.insteadOf, 2008-04-12).

Re: [PATCH 1/2] clone: respect the settings in $HOME/.gitconfig and /etc/gitconfig

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> On Fri, 27 Jun 2008, Johannes Schindelin wrote:
>
>> After initializing the config in the newly-created repository, we
>> need to unset GIT_CONFIG so that the global configs are read again.
>
> This seems fine to me. OTOH, I'm not sure the environment variable should 
> be needed in the first place; I think the config stuff should look in 
> git_path("config") without it, and we set the git dir to the one we're 
> initializing. So I think the use of the environment variable is just an 
> artifact of how the shell script did it and how I was originally calling 
> the init_db stuff.
>
> Just removing the "setenv()" line survives all of the tests for me, and I 
> remember some of them failing before I'd gotten some sort of solution for 
> the config stuff.

Ok, I take that you are Ok with 2/2 but you have a better replacement
patch coming for this 1/2?

Re: [PATCH 1/2] clone: respect the settings in $HOME/.gitconfig and /etc/gitconfig

[Daniel Barkalow]

On Fri, 27 Jun 2008, Junio C Hamano wrote:

> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > On Fri, 27 Jun 2008, Johannes Schindelin wrote:
> >
> >> After initializing the config in the newly-created repository, we
> >> need to unset GIT_CONFIG so that the global configs are read again.
> >
> > This seems fine to me. OTOH, I'm not sure the environment variable should 
> > be needed in the first place; I think the config stuff should look in 
> > git_path("config") without it, and we set the git dir to the one we're 
> > initializing. So I think the use of the environment variable is just an 
> > artifact of how the shell script did it and how I was originally calling 
> > the init_db stuff.
> >
> > Just removing the "setenv()" line survives all of the tests for me, and I 
> > remember some of them failing before I'd gotten some sort of solution for 
> > the config stuff.
> 
> Ok, I take that you are Ok with 2/2 but you have a better replacement
> patch coming for this 1/2?

I think so. Did we even make a commitment on whether:

GIT_CONFIG=foo git clone bar

must ignore the environment variable, or simply doesn't necessarily obey 
it? IIRC, the test scripts used to set GIT_CONFIG to something 
problematic (and would fail if clone didn't ignore it), but this was 
deemed incorrect usage and fixed. 

If it's okay to obey it (i.e., it gives the location of the config file 
for the clone):

------
Use all of the normal config-file handling in builtin-clone.c

There's no need to use the environment variable to direct the generation 
of the config file in the C version of clone, and having it not defined 
means that global and per-user configuration is available. This is 
necessary for the fetching portion, and could be useful someday for the 
init portion as well. It is unlikely that the user would want the 
repository's configuration in some non-default location, but no less 
likely than with any other git command besides "git config".

Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
---

diff --git a/builtin-clone.c b/builtin-clone.c
index f13845f..6dd58ac 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -415,8 +415,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	atexit(remove_junk);
 	signal(SIGINT, remove_junk_on_signal);
 
-	setenv(CONFIG_ENVIRONMENT, xstrdup(mkpath("%s/config", git_dir)), 1);
-
 	if (safe_create_leading_directories_const(git_dir) < 0)
 		die("could not create leading directories of '%s'", git_dir);
 	set_git_dir(make_absolute_path(git_dir));

Re: Using url.insteadOf in git-clone

[Pieter de Bie]

On 27 jun 2008, at 19:11, Junio C Hamano wrote:

> Pieter de Bie <pdebie@ai.rug.nl> writes:
>
>> [1] This is not really true. If I try that, I get an error:
>> Vienna:git pieter$ git fetch -v repo:dscho.git
>> fatal: I don't handle protocol 'it'
>>
>> If I change the url to "ggit://.." it does work. It seems there is an
>> off-by-one
>> error somewhere? It does work for another url.insteadOf I have in my
>> global
>> config though..
>
> What version of git is this?
>
> v1.5.5.1 and later contains 60e3aba (Fix config key miscount in
> url.*.insteadOf, 2008-04-12).
>

I think I tried this while testing the 1.5.5 clone.sh, so that's  
probably it.
Thanks.


- Pieter

Re: [PATCH 2/2] clone: respect url.insteadOf setting in global configs

[Pieter de Bie]

On 27 jun 2008, at 14:56, Johannes Schindelin wrote:

> When we call "git clone" with a url that has a rewrite rule in either
> $HOME/.gitconfig or /etc/gitconfig, the URL can be different from
> what the command line expects it to be.
>
> So, let's use the URL as the remote structure has it, not the literal
> string from the command line.
>
> Noticed by Pieter de Bie.


This works great, thanks for the quick patch! :)

- Pieter

Re: [PATCH 1/2] clone: respect the settings in $HOME/.gitconfig and /etc/gitconfig

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> There's no need to use the environment variable to direct the generation 
> of the config file in the C version of clone, and having it not defined 
> means that global and per-user configuration is available. This is 
> necessary for the fetching portion, and could be useful someday for the 
> init portion as well. It is unlikely that the user would want the 
> repository's configuration in some non-default location, but no less 
> likely than with any other git command besides "git config".
>
> Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>

Thanks, will queue together with the earlier [2/2] from Dscho.

Can we have updates to the testsuite as well?  We would probably have to
fudge $HOME and have some entries in $HOME/.gitconfig to simulate the
situation that triggered the original discussion.

Re: [PATCH 1/2] clone: respect the settings in $HOME/.gitconfig and /etc/gitconfig

[Johannes Schindelin]

Hi,

On Sun, 29 Jun 2008, Daniel Barkalow wrote:

> Did we even make a commitment on whether:
> 
> GIT_CONFIG=foo git clone bar
> 
> must ignore the environment variable, or simply doesn't necessarily obey 
> it?

I'd rather strongly argue that no matter what is the answer to this 
question, we _HAVE TO_ unsetenv() GIT_CONFIG at some stage, otherwise no 
.git/config will be written.

So, this is a NACK on your patch.

Ciao,
Dscho

Re: [PATCH 2/2] clone: respect url.insteadOf setting in global configs

[Johannes Schindelin]

Hi,

On Sun, 29 Jun 2008, Pieter de Bie wrote:

> On 27 jun 2008, at 14:56, Johannes Schindelin wrote:
> 
> > When we call "git clone" with a url that has a rewrite rule in either
> > $HOME/.gitconfig or /etc/gitconfig, the URL can be different from
> > what the command line expects it to be.
> >
> > So, let's use the URL as the remote structure has it, not the literal
> > string from the command line.
> >
> > Noticed by Pieter de Bie.
> 
> 
> This works great, thanks for the quick patch! :)

Thanks for the quick answer,
Dscho

Re: [PATCH 1/2] clone: respect the settings in $HOME/.gitconfig and /etc/gitconfig

[Daniel Barkalow]

On Sun, 29 Jun 2008, Johannes Schindelin wrote:

> Hi,
> 
> On Sun, 29 Jun 2008, Daniel Barkalow wrote:
> 
> > Did we even make a commitment on whether:
> > 
> > GIT_CONFIG=foo git clone bar
> > 
> > must ignore the environment variable, or simply doesn't necessarily obey 
> > it?
> 
> I'd rather strongly argue that no matter what is the answer to this 
> question, we _HAVE TO_ unsetenv() GIT_CONFIG at some stage, otherwise no 
> .git/config will be written.

Why should .git/config get written? The user is explicitly using a 
different file instead, so .git/config really shouldn't get written, 
unless the user isn't allowed to use the environment variable or the 
environment variable shouldn't apply.

Actually, perhaps the right thing is to remove the code to look at the 
environment variable from config.c and have it in builtin-config.c, since 
only "git config" is actually documented to be affected by GIT_CONFIG at 
all. But I don't really know what the variable is supposed to do, beyond 
what's in the documentation. In any case, I don't think "git clone" is at 
all special with respect to GIT_CONFIG.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH 1/2] clone: respect the settings in $HOME/.gitconfig and /etc/gitconfig

[Johannes Schindelin]

Hi,

On Sun, 29 Jun 2008, Daniel Barkalow wrote:

> On Sun, 29 Jun 2008, Johannes Schindelin wrote:
> 
> > On Sun, 29 Jun 2008, Daniel Barkalow wrote:
> > 
> > > Did we even make a commitment on whether:
> > > 
> > > GIT_CONFIG=foo git clone bar
> > > 
> > > must ignore the environment variable, or simply doesn't necessarily 
> > > obey it?
> > 
> > I'd rather strongly argue that no matter what is the answer to this 
> > question, we _HAVE TO_ unsetenv() GIT_CONFIG at some stage, otherwise 
> > no .git/config will be written.
> 
> Why should .git/config get written?

Because the user asked for a clone, where she reasonably expects a git 
repository with all the [core] and the initial [remote "origin"] settings 
to be written as it should be, _even if_ setting the config to somewhere 
else?  Hmm?

IMITCNVHO it would be a serious mistake to write the config somewhere else 
with "clone".

If that still does not convince you, "git init" also writes to 
".git/config" regardless of the user's (possibly bogus) GIT_CONFIG.

It is just such a basic thing that you must _not_ use GIT_CONFIG for 
writing with git clone or git init.

Ciao,
Dscho

Re: [PATCH 1/2] clone: respect the settings in $HOME/.gitconfig and /etc/gitconfig

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> ... In any case, I don't think "git clone" is at 
> all special with respect to GIT_CONFIG.

I think "git init" and "git clone" are very special with respect to
GIT_CONFIG.

 * When "init" is run to create a new repository and initialize it, the
   user would want the initial set of configuration populated in the
   configuration file _for that repository_.  There however may be some
   customization that affects the way how "init" operates, which might be
   taken from $HOME/.gitconfig.  The meaning of GIT_CONFIG can get fuzzy
   here.  Possibilities:

   (1) Instead of $HOME/.gitconfig (or /etc/gitconfig), the user might
       want such customizations to be read from the file specified by
       GIT_CONFIG.  But the user wants to make the resulting new
       repository usable without any custom GIT_CONFIG set (i.e. its
       $GIT_DIR/config will be the place the configuration is written).

   (2) The user may want to create a new repository that cannot be used
       with GIT_CONFIG (for some strange reason), i.e. no $GIT_DIR/config
       for the repository, and GIT_CONFIG is used to specify where that
       separate configuration file for the new repository is.  The way
       "init" operates does not come from that configuration file that is
       to be created but from elsewhere.

 * When "clone" is run, the same confusion as initializing "init" applies.
   In addition, custom GIT_CONFIG to read customizations for behaviour of
   "init" and "fetch" that is done internally by "clone" would play larger
   role.

 * When "init" is run to reinitialize an existing repository, it is not
   special in any way with respect to GIT_CONFIG, compared to other
   commands.  The GIT_CONFIG names the configuration for that existing
   repository, so we read from it and write to it.

I personally think the case (2) is a very narrow special case that I do
not think of any sane reason to even wanting to do so.  IOW, "you _could_
interpret the presense of GIT_CONFIG that the user may want to do so, but
why?"  (1) is also probably not very sensible but it makes more sense.

I think Dscho's original patch would support the semantics (1) and it is
probably much saner than (2) which is your version does (if I am reading
the two patches correctly).

Re: [PATCH 1/2] clone: respect the settings in $HOME/.gitconfig and /etc/gitconfig

[Daniel Barkalow]

On Mon, 30 Jun 2008, Johannes Schindelin wrote:

> Hi,
> 
> On Sun, 29 Jun 2008, Daniel Barkalow wrote:
> 
> > On Sun, 29 Jun 2008, Johannes Schindelin wrote:
> > 
> > > On Sun, 29 Jun 2008, Daniel Barkalow wrote:
> > > 
> > > > Did we even make a commitment on whether:
> > > > 
> > > > GIT_CONFIG=foo git clone bar
> > > > 
> > > > must ignore the environment variable, or simply doesn't necessarily 
> > > > obey it?
> > > 
> > > I'd rather strongly argue that no matter what is the answer to this 
> > > question, we _HAVE TO_ unsetenv() GIT_CONFIG at some stage, otherwise 
> > > no .git/config will be written.
> > 
> > Why should .git/config get written?
> 
> Because the user asked for a clone, where she reasonably expects a git 
> repository with all the [core] and the initial [remote "origin"] settings 
> to be written as it should be, _even if_ setting the config to somewhere 
> else?  Hmm?

But those should be written to the location of the config file, where 
subsequent commands will find them, which is $GIT_CONFIG if it's set and 
git commands in general use it. I mean:

$ export GIT_CONFIG=/home/barkalow/something
$ git clone git://git.kernel.org/pub/scm/git/git.git
$ cd git
$ git fetch
fatal: 'origin': unable to chdir or not a git archive
fatal: The remote end hung up unexpectedly

(because "git clone" currently ignores GIT_CONFIG, but "git fetch" 
doesn't, so it can't find the initial [remote "origin"] settings).

> IMITCNVHO it would be a serious mistake to write the config somewhere else 
> with "clone".
> 
> If that still does not convince you, "git init" also writes to 
> ".git/config" regardless of the user's (possibly bogus) GIT_CONFIG.

No, "git init" has always written to GIT_CONFIG. In fact, git-clone.sh 
used to depend on it writing to GIT_CONFIG, which is how it caused the 
config file to be written into the new clone.

> It is just such a basic thing that you must _not_ use GIT_CONFIG for 
> writing with git clone or git init.

Surely, then, you must not use GIT_CONFIG when reading the options that 
git clone writes? But I think this reduces to "you must not use GIT_CONFIG 
when using a repository", which pretty much leaves "git config". And I 
think it's only ever *useful* for "git config" anyway.

It doesn't make much sense to ensure that "git clone" works if you have 
GIT_CONFIG set when nothing else works in that situation. I still don't 
know what setting it is good for (and the commit that introduced it 
explained what it did, but not why), but I think we should be consistant 
about whether or not it affects where git expects configuration to be.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH 1/2] clone: respect the settings in $HOME/.gitconfig and /etc/gitconfig

[Daniel Barkalow]

On Sun, 29 Jun 2008, Junio C Hamano wrote:

> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > ... In any case, I don't think "git clone" is at 
> > all special with respect to GIT_CONFIG.
> 
> I think "git init" and "git clone" are very special with respect to
> GIT_CONFIG.
> 
>  * When "init" is run to create a new repository and initialize it, the
>    user would want the initial set of configuration populated in the
>    configuration file _for that repository_.  There however may be some
>    customization that affects the way how "init" operates, which might be
>    taken from $HOME/.gitconfig.  The meaning of GIT_CONFIG can get fuzzy
>    here.  Possibilities:
> 
>    (1) Instead of $HOME/.gitconfig (or /etc/gitconfig), the user might
>        want such customizations to be read from the file specified by
>        GIT_CONFIG.  But the user wants to make the resulting new
>        repository usable without any custom GIT_CONFIG set (i.e. its
>        $GIT_DIR/config will be the place the configuration is written).
> 
>    (2) The user may want to create a new repository that cannot be used
>        with GIT_CONFIG (for some strange reason), i.e. no $GIT_DIR/config
>        for the repository, and GIT_CONFIG is used to specify where that
>        separate configuration file for the new repository is.  The way
>        "init" operates does not come from that configuration file that is
>        to be created but from elsewhere.
> 
>  * When "clone" is run, the same confusion as initializing "init" applies.
>    In addition, custom GIT_CONFIG to read customizations for behaviour of
>    "init" and "fetch" that is done internally by "clone" would play larger
>    role.
> 
>  * When "init" is run to reinitialize an existing repository, it is not
>    special in any way with respect to GIT_CONFIG, compared to other
>    commands.  The GIT_CONFIG names the configuration for that existing
>    repository, so we read from it and write to it.
> 
> I personally think the case (2) is a very narrow special case that I do
> not think of any sane reason to even wanting to do so.  IOW, "you _could_
> interpret the presense of GIT_CONFIG that the user may want to do so, but
> why?"  (1) is also probably not very sensible but it makes more sense.

Actually, (1) has never worked for either clone or init. Init always 
obeyed GIT_CONFIG as for where it put the output (in fact git-clone.sh 
used GIT_CONFIG to cause git-init to write to the created repo, IIRC). 
Clone always replaced GIT_CONFIG with the destination location before it 
read any configuration, so it wouldn't see configuration that was in the 
user's GIT_CONFIG (or in any pre-existing config file at all). 
Historically, these commands don't support (1) and aren't consistant with 
each other.

I think it would make sense if there were some way to provide custom 
configuration to a particular invocation of "git clone", but GIT_CONFIG as 
currently handled by config.c can't do it.

> I think Dscho's original patch would support the semantics (1) and it is
> probably much saner than (2) which is your version does (if I am reading
> the two patches correctly).

My patch makes "git clone" do what "git init; git remote; git fetch" do 
with respect to GIT_CONFIG. I don't think this behavior is really useful 
for any of those programs or for the combination, but at least having them 
match would be consistant. I think the sensible thing would be for 
config.c to always write to git_path("config") (or a variable used by 
"git config"), and read: git_path("config"), $GIT_CONFIG, ~/.gitconfig, 
/etc/gitconfig (in order of decreasing precedence); and further have 
builtin-config.c set the "file to write to" variable based on $GIT_CONFIG 
or "--file", "--global", or "--system".

But I don't really understand what the purpose of the feature was in the 
first place, aside from the narrow case of being able to read and write 
files that use the config file format with "git config".

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH 1/2] clone: respect the settings in $HOME/.gitconfig and /etc/gitconfig

[Daniel Barkalow]

On Sun, 29 Jun 2008, Junio C Hamano wrote:

> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > ... In any case, I don't think "git clone" is at 
> > all special with respect to GIT_CONFIG.
> 
> I think "git init" and "git clone" are very special with respect to
> GIT_CONFIG.
> 
>  * When "init" is run to create a new repository and initialize it, the
>    user would want the initial set of configuration populated in the
>    configuration file _for that repository_.  There however may be some
>    customization that affects the way how "init" operates, which might be
>    taken from $HOME/.gitconfig.  The meaning of GIT_CONFIG can get fuzzy
>    here.  Possibilities:
> 
>    (1) Instead of $HOME/.gitconfig (or /etc/gitconfig), the user might
>        want such customizations to be read from the file specified by
>        GIT_CONFIG.  But the user wants to make the resulting new
>        repository usable without any custom GIT_CONFIG set (i.e. its
>        $GIT_DIR/config will be the place the configuration is written).
> 
>    (2) The user may want to create a new repository that cannot be used
>        with GIT_CONFIG (for some strange reason), i.e. no $GIT_DIR/config
>        for the repository, and GIT_CONFIG is used to specify where that
>        separate configuration file for the new repository is.  The way
>        "init" operates does not come from that configuration file that is
>        to be created but from elsewhere.
> 
>  * When "clone" is run, the same confusion as initializing "init" applies.
>    In addition, custom GIT_CONFIG to read customizations for behaviour of
>    "init" and "fetch" that is done internally by "clone" would play larger
>    role.
> 
>  * When "init" is run to reinitialize an existing repository, it is not
>    special in any way with respect to GIT_CONFIG, compared to other
>    commands.  The GIT_CONFIG names the configuration for that existing
>    repository, so we read from it and write to it.
> 
> I personally think the case (2) is a very narrow special case that I do
> not think of any sane reason to even wanting to do so.  IOW, "you _could_
> interpret the presense of GIT_CONFIG that the user may want to do so, but
> why?"  (1) is also probably not very sensible but it makes more sense.
> 
> I think Dscho's original patch would support the semantics (1) and it is
> probably much saner than (2) which is your version does (if I am reading
> the two patches correctly).

A bit more information on this. There are 3 possible states for 
GIT_CONFIG:

 (1) Not set; things work normally

 (2) Set to ".git/config"; things work normally except that no global or 
     system config is used

 (3) Set to something else; anything expecting to read or write 
     configuration related to the repo will misbehave

Historically, clone worked entirely in state (2) regardless of what the 
user asked for. Everything else left it how the user had it. Dscho's patch 
changes it to initialize the repo in state (2) and fetch in state (1), 
still ignoring what the user set. My patch changes it to leave it how the 
user has it.

Now, clone writes to the config file before reading any configuration, so, 
if it's going to write to ".git/config" instead of $GIT_CONFIG, it can't 
read from $GIT_CONFIG either. So there's no way (outside of redesigning 
config.c) to make GIT_CONFIG useful for "clone" in particular. Other 
commands don't clear it, so they just misbehave if it's set. We can be 
pretty sure, therefore, that users will put it either in (1) or (if they 
want GIT_CONFIG_NO_GLOBAL and don't know about it and aren't worried about 
"clone") in (2). So the practical difference between my patch and Dscho's 
is that, with mine, ~/.gitconfig would affect the initialization step if 
it actually used any config settings.

If you want anything more clever to happen, that requires config.c 
changes of some sort to provide a state where it reads some additional 
file but writes the repo's usual one, and probably *also* *my* patch, so 
that clone doesn't blow away the user's setting before config.c gets it.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH 1/2] clone: respect the settings in $HOME/.gitconfig and /etc/gitconfig

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Sun, 29 Jun 2008, Daniel Barkalow wrote:
>
>> Did we even make a commitment on whether:
>> 
>> GIT_CONFIG=foo git clone bar
>> 
>> must ignore the environment variable, or simply doesn't necessarily obey 
>> it?
>
> I'd rather strongly argue that no matter what is the answer to this 
> question, we _HAVE TO_ unsetenv() GIT_CONFIG at some stage, otherwise no 
> .git/config will be written.
>
> So, this is a NACK on your patch.

True.  We are creating the config file for the new repository, so the
initial setenv() would make sense.  We _could_ save away end user's
GIT_CONFIG and restore it where you unsetenv() in your patch, but I do not
think it would buy us anything other than "be consistent with other
programs that misbehave when end user has GIT_CONFIG".  

Honestly, GIT_CONFIG is purely for scripts like git-svn that muck with
files that are in the config format to have a way to make sure that they
access the file they intend to, and being able to use GIT_CONFIG to keep
git programs from reading from $HOME/.gitconfig is primarily for giving
our test scripts repeatable environment, nothing more.

I think ignoring end-user GIT_CONFIG like this patch does, instead of
doing random nonsense, would be a good bugfix for "git clone".

Re: [PATCH 1/2] clone: respect the settings in $HOME/.gitconfig and /etc/gitconfig

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Honestly, GIT_CONFIG is purely for scripts like git-svn that muck with
> files that are in the config format to have a way to make sure that they
> access the file they intend to, and being able to use GIT_CONFIG to keep
> git programs from reading from $HOME/.gitconfig is primarily for giving
> our test scripts repeatable environment, nothing more.
>
> I think ignoring end-user GIT_CONFIG like this patch does, instead of
> doing random nonsense, would be a good bugfix for "git clone".

Of course, that would have downsides as well.  Now git-clone tests cannot
be sanely written without setting $HOME to somewhere stable; otherwise
they can be randomly affected by buggy $HOME/.gitconfig files the end user
may have.

Re: [PATCH 1/2] clone: respect the settings in $HOME/.gitconfig and /etc/gitconfig

[Jeff King]

On Sun, Jun 29, 2008 at 11:25:05PM -0700, Junio C Hamano wrote:

> > Honestly, GIT_CONFIG is purely for scripts like git-svn that muck with
> > files that are in the config format to have a way to make sure that they
> > access the file they intend to, and being able to use GIT_CONFIG to keep
> > git programs from reading from $HOME/.gitconfig is primarily for giving
> > our test scripts repeatable environment, nothing more.
[...]
> Of course, that would have downsides as well.  Now git-clone tests cannot
> be sanely written without setting $HOME to somewhere stable; otherwise
> they can be randomly affected by buggy $HOME/.gitconfig files the end user
> may have.

I thought we went through all of this before, and it led to 8bfa6bd6.
The test scripts set GIT_CONFIG_NOSYSTEM and GIT_CONFIG_NOGLOBAL to
achieve the same suppression effect.

-Peff

Re: [PATCH 1/2] clone: respect the settings in $HOME/.gitconfig and /etc/gitconfig

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> Of course, that would have downsides as well.  Now git-clone tests cannot
>> be sanely written without setting $HOME to somewhere stable; otherwise
>> they can be randomly affected by buggy $HOME/.gitconfig files the end user
>> may have.
>
> I thought we went through all of this before, and it led to 8bfa6bd6.
> The test scripts set GIT_CONFIG_NOSYSTEM and GIT_CONFIG_NOGLOBAL to
> achieve the same suppression effect.

Ah, thanks.  Actually I was scratching my head trying to figure out why it
does not break after I wrote the above ;-)

Re: [PATCH 1/2] clone: respect the settings in $HOME/.gitconfig and /etc/gitconfig

[Johannes Schindelin]

Hi,

On Sun, 29 Jun 2008, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Sun, 29 Jun 2008, Daniel Barkalow wrote:
> >
> >> Did we even make a commitment on whether:
> >> 
> >> GIT_CONFIG=foo git clone bar
> >> 
> >> must ignore the environment variable, or simply doesn't necessarily 
> >> obey it?
> >
> > I'd rather strongly argue that no matter what is the answer to this 
> > question, we _HAVE TO_ unsetenv() GIT_CONFIG at some stage, otherwise 
> > no .git/config will be written.
> >
> > So, this is a NACK on your patch.
> 
> True.  We are creating the config file for the new repository, so the
> initial setenv() would make sense.  We _could_ save away end user's
> GIT_CONFIG and restore it where you unsetenv() in your patch,

No.  That would break again, since then,

- $HOME/.gitconfig would be ignored again (which was the single issue my 
  patch addressed), and
- the remote information would be written into the wrong file.

Ciao,
Dscho

Re: [PATCH 1/2] clone: respect the settings in $HOME/.gitconfig and /etc/gitconfig

[Johannes Schindelin]

Hi,

On Sun, 29 Jun 2008, Daniel Barkalow wrote:

> Now, clone writes to the config file before reading any configuration, so, 
> if it's going to write to ".git/config" instead of $GIT_CONFIG, it can't 
> read from $GIT_CONFIG either. So there's no way (outside of redesigning 
> config.c) to make GIT_CONFIG useful for "clone" in particular.

Except you could read the config _before_ writing.

Or you could enhance (not redesign, as you suggest) config.c thusly:

-- snip --
diff --git a/cache.h b/cache.h
index 871f6c1..f3ea997 100644
--- a/cache.h
+++ b/cache.h
@@ -742,6 +742,7 @@ extern int git_config_bool(const char *, const char *);
 extern int git_config_string(const char **, const char *, const char *);
 extern int git_config_set(const char *, const char *);
 extern int git_config_set_multivar(const char *, const char *, const char *, int);
+extern int git_config_set_multivar_in_file(const char *, const char *, const char *, int, const char *);
 extern int git_config_rename_section(const char *, const char *);
 extern const char *git_etc_gitconfig(void);
 extern int check_repository_format_version(const char *var, const char *value, void *cb);
diff --git a/config.c b/config.c
index 58749bf..41a35eb 100644
--- a/config.c
+++ b/config.c
@@ -863,23 +863,31 @@ int git_config_set(const char* key, const char* value)
  * - the config file is removed and the lock file rename()d to it.
  *
  */
-int git_config_set_multivar(const char* key, const char* value,
-	const char* value_regex, int multi_replace)
+int git_config_set_multivar(const char *key, const char *value,
+	const char *value_regex, int multi_replace)
+{
+	return git_config_set_multivar_in_file(key, value, value_regex,
+		multi_replace, NULL);
+}
+
+int git_config_set_multivar_in_file(const char *key, const char *value,
+	const char *value_regex, int multi_replace, const char *config_filename)
 {
 	int i, dot;
 	int fd = -1, in_fd;
 	int ret;
-	char* config_filename;
+	char *filename;
 	struct lock_file *lock = NULL;
 	const char* last_dot = strrchr(key, '.');
 
-	config_filename = getenv(CONFIG_ENVIRONMENT);
+	if (!config_filename)
+		config_filename = getenv(CONFIG_ENVIRONMENT);
 	if (!config_filename) {
 		config_filename = getenv(CONFIG_LOCAL_ENVIRONMENT);
 		if (!config_filename)
-			config_filename  = git_path("config");
+			config_filename =
+				filename = xstrdup(git_path("config"));
 	}
-	config_filename = xstrdup(config_filename);
 
 	/*
 	 * Since "key" actually contains the section name and the real
@@ -1091,7 +1099,8 @@ int git_config_set_multivar(const char* key, const char* value,
 out_free:
 	if (lock)
 		rollback_lock_file(lock);
-	free(config_filename);
+	if (filename)
+		free(filename);
 	return ret;
 
 write_err_out:
-- snap --

... and then have something like

        config_filename = xstrdup(mkpath("%s/config", git_dir));

        if (safe_create_leading_directories_const(git_dir) < 0)
                die("could not create leading directories of '%s'", git_dir);
        set_git_dir(make_absolute_path(git_dir));

	[...]

	if (option_bare) {
                strcpy(branch_top, "refs/heads/");

                git_config_set_multivar_in_file("core.bare", "true",
			NULL, 0, config_filename);

	[...]

Of course, you would also have to teach init_db() to use this filename.

But frankly, I do not see the use of your "narrow" special case.  And as I 
stated in another thread, I am pretty opposed to crossing bridges that are 
miles (or an eternity) away.

So unless you present me with a sensible scenario where your "respect 
GIT_CONFIG for _reading_ in GIT_CONFIG=... git clone" makes sense, there 
is nothing to see here, please move along.

Ciao,
Dscho

Re: [PATCH 1/2] clone: respect the settings in $HOME/.gitconfig and /etc/gitconfig

[Daniel Barkalow]

On Mon, 30 Jun 2008, Johannes Schindelin wrote:

> Hi,
> 
> On Sun, 29 Jun 2008, Daniel Barkalow wrote:
> 
> > Now, clone writes to the config file before reading any configuration, so, 
> > if it's going to write to ".git/config" instead of $GIT_CONFIG, it can't 
> > read from $GIT_CONFIG either. So there's no way (outside of redesigning 
> > config.c) to make GIT_CONFIG useful for "clone" in particular.
> 
> Except you could read the config _before_ writing.
> 
> Or you could enhance (not redesign, as you suggest) config.c thusly:
> 
> -- snip --
> diff --git a/cache.h b/cache.h
> index 871f6c1..f3ea997 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -742,6 +742,7 @@ extern int git_config_bool(const char *, const char *);
>  extern int git_config_string(const char **, const char *, const char *);
>  extern int git_config_set(const char *, const char *);
>  extern int git_config_set_multivar(const char *, const char *, const char *, int);
> +extern int git_config_set_multivar_in_file(const char *, const char *, const char *, int, const char *);
>  extern int git_config_rename_section(const char *, const char *);
>  extern const char *git_etc_gitconfig(void);
>  extern int check_repository_format_version(const char *var, const char *value, void *cb);
> diff --git a/config.c b/config.c
> index 58749bf..41a35eb 100644
> --- a/config.c
> +++ b/config.c
> @@ -863,23 +863,31 @@ int git_config_set(const char* key, const char* value)
>   * - the config file is removed and the lock file rename()d to it.
>   *
>   */
> -int git_config_set_multivar(const char* key, const char* value,
> -	const char* value_regex, int multi_replace)
> +int git_config_set_multivar(const char *key, const char *value,
> +	const char *value_regex, int multi_replace)
> +{
> +	return git_config_set_multivar_in_file(key, value, value_regex,
> +		multi_replace, NULL);
> +}
> +
> +int git_config_set_multivar_in_file(const char *key, const char *value,
> +	const char *value_regex, int multi_replace, const char *config_filename)
>  {
>  	int i, dot;
>  	int fd = -1, in_fd;
>  	int ret;
> -	char* config_filename;
> +	char *filename;
>  	struct lock_file *lock = NULL;
>  	const char* last_dot = strrchr(key, '.');
>  
> -	config_filename = getenv(CONFIG_ENVIRONMENT);
> +	if (!config_filename)
> +		config_filename = getenv(CONFIG_ENVIRONMENT);
>  	if (!config_filename) {
>  		config_filename = getenv(CONFIG_LOCAL_ENVIRONMENT);
>  		if (!config_filename)
> -			config_filename  = git_path("config");
> +			config_filename =
> +				filename = xstrdup(git_path("config"));
>  	}
> -	config_filename = xstrdup(config_filename);
>  
>  	/*
>  	 * Since "key" actually contains the section name and the real
> @@ -1091,7 +1099,8 @@ int git_config_set_multivar(const char* key, const char* value,
>  out_free:
>  	if (lock)
>  		rollback_lock_file(lock);
> -	free(config_filename);
> +	if (filename)
> +		free(filename);
>  	return ret;
>  
>  write_err_out:
> -- snap --
> 
> ... and then have something like
> 
>         config_filename = xstrdup(mkpath("%s/config", git_dir));
> 
>         if (safe_create_leading_directories_const(git_dir) < 0)
>                 die("could not create leading directories of '%s'", git_dir);
>         set_git_dir(make_absolute_path(git_dir));
> 
> 	[...]
> 
> 	if (option_bare) {
>                 strcpy(branch_top, "refs/heads/");
> 
>                 git_config_set_multivar_in_file("core.bare", "true",
> 			NULL, 0, config_filename);
> 
> 	[...]
> 
> Of course, you would also have to teach init_db() to use this filename.
> 
> But frankly, I do not see the use of your "narrow" special case.  And as I 
> stated in another thread, I am pretty opposed to crossing bridges that are 
> miles (or an eternity) away.
> 
> So unless you present me with a sensible scenario where your "respect 
> GIT_CONFIG for _reading_ in GIT_CONFIG=... git clone" makes sense, there 
> is nothing to see here, please move along.

I haven't been able to find a sensible scenario for paying attention to 
GIT_CONFIG at all, except for backwards compatibility in builtin-config.c; 
I'd been going on the assumption that there was a sensible scenario for 
having it in the first place, and that "git clone" wouldn't be any 
different from "git init", "git remote", or "git fetch". 

But nobody seems to have any reason for GIT_CONFIG to exist outside of the 
"git config" command-line and environment parsing, so I posted a patch to 
rip it out of the general code, so "git clone" doesn't have to deal with 
it at all.

	-Daniel
*This .sig left intentionally blank*