[(Apple,Git),12/13] Enable support for Xcode.app-bundled gitconfig
diff mbox series

Message ID 20190129193818.8645-13-jeremyhu@apple.com
State New
Headers show
Series
  • Differences between git-2.20.1 and Apple Git-116
Related show

Commit Message

Jeremy Sequoia Jan. 29, 2019, 7:38 p.m. UTC
Useful for setting up osxkeychain in Xcode.app's gitconfig

Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
---
 config.c | 13 +++++++++++++
 config.h |  2 ++
 2 files changed, 15 insertions(+)

Comments

Junio C Hamano Jan. 29, 2019, 11:10 p.m. UTC | #1
Jeremy Huddleston Sequoia <jeremyhu@apple.com> writes:

> Useful for setting up osxkeychain in Xcode.app's gitconfig
>
> Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
> ---

A concern shared with 13/13 is this.

While it may not hurt too much to look at one extra location even on
non-Apple platform, it probably is a mistake to have this xcode
specific change in generic part of the system like config.c or
attr.c.  For that matter, would it make sense to force Apple uses to
look at one extra location in the first place?  In other words, we
already have "system wide" location (i.e. system_path(ETC_GITCONFIG))
defined so system owners can give reasonable default to its users.
The value of not using that facility and instead adding yet another
place is dubious.


  




>  config.c | 13 +++++++++++++
>  config.h |  2 ++
>  2 files changed, 15 insertions(+)
>
> diff --git a/config.c b/config.c
> index ff521eb27a..656bfef8ab 100644
> --- a/config.c
> +++ b/config.c
> @@ -1631,6 +1631,14 @@ const char *git_etc_gitconfig(void)
>  	return system_wide;
>  }
>  
> +const char *git_xcode_gitconfig(void)
> +{
> +	static const char *xcode_config;
> +	if (!xcode_config)
> +		xcode_config = system_path("share/git-core/gitconfig");
> +	return xcode_config;
> +}
> +
>  /*
>   * Parse environment variable 'k' as a boolean (in various
>   * possible spellings); if missing, use the default value 'def'.
> @@ -1673,6 +1681,11 @@ static int do_git_config_sequence(const struct config_options *opts,
>  	else
>  		repo_config = NULL;
>  
> +	current_parsing_scope = CONFIG_SCOPE_XCODE;
> +	if (git_config_system() && git_xcode_gitconfig() && !access_or_die(git_xcode_gitconfig(), R_OK, 0))
> +		ret += git_config_from_file(fn, git_xcode_gitconfig(),
> +					    data);
> +
>  	current_parsing_scope = CONFIG_SCOPE_SYSTEM;
>  	if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0))
>  		ret += git_config_from_file(fn, git_etc_gitconfig(),
> diff --git a/config.h b/config.h
> index ee5d3fa7b4..f848423d28 100644
> --- a/config.h
> +++ b/config.h
> @@ -115,6 +115,7 @@ extern int git_config_rename_section_in_file(const char *, const char *, const c
>  extern int git_config_copy_section(const char *, const char *);
>  extern int git_config_copy_section_in_file(const char *, const char *, const char *);
>  extern const char *git_etc_gitconfig(void);
> +extern const char *git_xcode_gitconfig(void);
>  extern int git_env_bool(const char *, int);
>  extern unsigned long git_env_ulong(const char *, unsigned long);
>  extern int git_config_system(void);
> @@ -131,6 +132,7 @@ enum config_scope {
>  	CONFIG_SCOPE_GLOBAL,
>  	CONFIG_SCOPE_REPO,
>  	CONFIG_SCOPE_CMDLINE,
> +	CONFIG_SCOPE_XCODE,
>  };
>  
>  extern enum config_scope current_config_scope(void);
Jeremy Sequoia Jan. 29, 2019, 11:51 p.m. UTC | #2
> On Jan 29, 2019, at 3:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Jeremy Huddleston Sequoia <jeremyhu@apple.com> writes:
> 
>> Useful for setting up osxkeychain in Xcode.app's gitconfig
>> 
>> Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
>> ---
> 
> A concern shared with 13/13 is this.
> 
> While it may not hurt too much to look at one extra location even on
> non-Apple platform, it probably is a mistake to have this xcode
> specific change in generic part of the system like config.c or
> attr.c.  For that matter, would it make sense to force Apple uses to
> look at one extra location in the first place?  In other words, we
> already have "system wide" location (i.e. system_path(ETC_GITCONFIG))
> defined so system owners can give reasonable default to its users.
> The value of not using that facility and instead adding yet another
> place is dubious.

This allows for per-distribution configuration and could be useful for other applications as well that want customizations specific to their install of git.  For our specific use case, we do not want to munge the system policy when installing Xcode.  Prior to doing things this way, we were just changing the default in our distributed git binary, but this seems a bit more flexible.

> 
> 
> 
> 
> 
> 
> 
>> config.c | 13 +++++++++++++
>> config.h |  2 ++
>> 2 files changed, 15 insertions(+)
>> 
>> diff --git a/config.c b/config.c
>> index ff521eb27a..656bfef8ab 100644
>> --- a/config.c
>> +++ b/config.c
>> @@ -1631,6 +1631,14 @@ const char *git_etc_gitconfig(void)
>> 	return system_wide;
>> }
>> 
>> +const char *git_xcode_gitconfig(void)
>> +{
>> +	static const char *xcode_config;
>> +	if (!xcode_config)
>> +		xcode_config = system_path("share/git-core/gitconfig");
>> +	return xcode_config;
>> +}
>> +
>> /*
>>  * Parse environment variable 'k' as a boolean (in various
>>  * possible spellings); if missing, use the default value 'def'.
>> @@ -1673,6 +1681,11 @@ static int do_git_config_sequence(const struct config_options *opts,
>> 	else
>> 		repo_config = NULL;
>> 
>> +	current_parsing_scope = CONFIG_SCOPE_XCODE;
>> +	if (git_config_system() && git_xcode_gitconfig() && !access_or_die(git_xcode_gitconfig(), R_OK, 0))
>> +		ret += git_config_from_file(fn, git_xcode_gitconfig(),
>> +					    data);
>> +
>> 	current_parsing_scope = CONFIG_SCOPE_SYSTEM;
>> 	if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0))
>> 		ret += git_config_from_file(fn, git_etc_gitconfig(),
>> diff --git a/config.h b/config.h
>> index ee5d3fa7b4..f848423d28 100644
>> --- a/config.h
>> +++ b/config.h
>> @@ -115,6 +115,7 @@ extern int git_config_rename_section_in_file(const char *, const char *, const c
>> extern int git_config_copy_section(const char *, const char *);
>> extern int git_config_copy_section_in_file(const char *, const char *, const char *);
>> extern const char *git_etc_gitconfig(void);
>> +extern const char *git_xcode_gitconfig(void);
>> extern int git_env_bool(const char *, int);
>> extern unsigned long git_env_ulong(const char *, unsigned long);
>> extern int git_config_system(void);
>> @@ -131,6 +132,7 @@ enum config_scope {
>> 	CONFIG_SCOPE_GLOBAL,
>> 	CONFIG_SCOPE_REPO,
>> 	CONFIG_SCOPE_CMDLINE,
>> +	CONFIG_SCOPE_XCODE,
>> };
>> 
>> extern enum config_scope current_config_scope(void);
brian m. carlson Jan. 30, 2019, 9:44 a.m. UTC | #3
On Tue, Jan 29, 2019 at 03:10:30PM -0800, Junio C Hamano wrote:
> Jeremy Huddleston Sequoia <jeremyhu@apple.com> writes:
> 
> > Useful for setting up osxkeychain in Xcode.app's gitconfig
> >
> > Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
> > ---
> 
> A concern shared with 13/13 is this.
> 
> While it may not hurt too much to look at one extra location even on
> non-Apple platform, it probably is a mistake to have this xcode
> specific change in generic part of the system like config.c or
> attr.c.  For that matter, would it make sense to force Apple uses to
> look at one extra location in the first place?  In other words, we
> already have "system wide" location (i.e. system_path(ETC_GITCONFIG))
> defined so system owners can give reasonable default to its users.
> The value of not using that facility and instead adding yet another
> place is dubious.

I think it's relevant in this case if I point out what the gitconfig and
gitattributes files contain on macOS. The gitconfig file sets up the
default credential helper, and the gitattributes file sets up default
diff helpers for Objective C and Swift.

To my knowledge, I believe what other distributors (including Homebrew)
do is they provide the system configuration file with default options if
one is missing, allowing the user to retain or delete these options as
the administrator sees fit. Apple may or may not want to do that, but I
believe that's probably the way that we'll choose to support.

While distributors will of course customize Git in whatever way seems
most appropriate, I think it's better if those customizations are
editable by the user, since that makes it easier to write per-user
configuration files that are consistent across platforms. For a tool
like Git, that can be quite helpful.
Johannes Schindelin Jan. 30, 2019, 7:32 p.m. UTC | #4
Hi Jeremy,

On Tue, 29 Jan 2019, Jeremy Huddleston Sequoia wrote:

> > On Jan 29, 2019, at 3:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
> > 
> > Jeremy Huddleston Sequoia <jeremyhu@apple.com> writes:
> > 
> >> Useful for setting up osxkeychain in Xcode.app's gitconfig
> >> 
> >> Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
> >> ---
> > 
> > A concern shared with 13/13 is this.
> > 
> > While it may not hurt too much to look at one extra location even on
> > non-Apple platform, it probably is a mistake to have this xcode
> > specific change in generic part of the system like config.c or
> > attr.c.  For that matter, would it make sense to force Apple uses to
> > look at one extra location in the first place?  In other words, we
> > already have "system wide" location (i.e. system_path(ETC_GITCONFIG))
> > defined so system owners can give reasonable default to its users.
> > The value of not using that facility and instead adding yet another
> > place is dubious.
> 
> This allows for per-distribution configuration and could be useful for
> other applications as well that want customizations specific to their
> install of git.  For our specific use case, we do not want to munge the
> system policy when installing Xcode.  Prior to doing things this way, we
> were just changing the default in our distributed git binary, but this
> seems a bit more flexible.

I think you misunderstood Junio, thinking that he referred to
/etc/gitconfig. He did not. system_path(ETC_GITCONFIG) refers to
<prefix>/etc/gitconfig, where <prefix> is that runtime prefix when
compiled with RUNTIME_PREFIX.

So you can definitely have your own per-distribution configuration: it
lives in that very <prefix>/etc/gitconfig where the portable Git is
installed.

And since we have that nice facility, I agree with Junio that we probably
do not even need an extra config, certainly not one just introduced for
XCode.

Ciao,
Johannes

> 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> >> config.c | 13 +++++++++++++
> >> config.h |  2 ++
> >> 2 files changed, 15 insertions(+)
> >> 
> >> diff --git a/config.c b/config.c
> >> index ff521eb27a..656bfef8ab 100644
> >> --- a/config.c
> >> +++ b/config.c
> >> @@ -1631,6 +1631,14 @@ const char *git_etc_gitconfig(void)
> >> 	return system_wide;
> >> }
> >> 
> >> +const char *git_xcode_gitconfig(void)
> >> +{
> >> +	static const char *xcode_config;
> >> +	if (!xcode_config)
> >> +		xcode_config = system_path("share/git-core/gitconfig");
> >> +	return xcode_config;
> >> +}
> >> +
> >> /*
> >>  * Parse environment variable 'k' as a boolean (in various
> >>  * possible spellings); if missing, use the default value 'def'.
> >> @@ -1673,6 +1681,11 @@ static int do_git_config_sequence(const struct config_options *opts,
> >> 	else
> >> 		repo_config = NULL;
> >> 
> >> +	current_parsing_scope = CONFIG_SCOPE_XCODE;
> >> +	if (git_config_system() && git_xcode_gitconfig() && !access_or_die(git_xcode_gitconfig(), R_OK, 0))
> >> +		ret += git_config_from_file(fn, git_xcode_gitconfig(),
> >> +					    data);
> >> +
> >> 	current_parsing_scope = CONFIG_SCOPE_SYSTEM;
> >> 	if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0))
> >> 		ret += git_config_from_file(fn, git_etc_gitconfig(),
> >> diff --git a/config.h b/config.h
> >> index ee5d3fa7b4..f848423d28 100644
> >> --- a/config.h
> >> +++ b/config.h
> >> @@ -115,6 +115,7 @@ extern int git_config_rename_section_in_file(const char *, const char *, const c
> >> extern int git_config_copy_section(const char *, const char *);
> >> extern int git_config_copy_section_in_file(const char *, const char *, const char *);
> >> extern const char *git_etc_gitconfig(void);
> >> +extern const char *git_xcode_gitconfig(void);
> >> extern int git_env_bool(const char *, int);
> >> extern unsigned long git_env_ulong(const char *, unsigned long);
> >> extern int git_config_system(void);
> >> @@ -131,6 +132,7 @@ enum config_scope {
> >> 	CONFIG_SCOPE_GLOBAL,
> >> 	CONFIG_SCOPE_REPO,
> >> 	CONFIG_SCOPE_CMDLINE,
> >> +	CONFIG_SCOPE_XCODE,
> >> };
> >> 
> >> extern enum config_scope current_config_scope(void);
> 
>
Jeremy Sequoia Jan. 30, 2019, 9:09 p.m. UTC | #5
> On Jan 30, 2019, at 11:32, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> 
> Hi Jeremy,
> 
> On Tue, 29 Jan 2019, Jeremy Huddleston Sequoia wrote:
> 
>>> On Jan 29, 2019, at 3:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> 
>>> Jeremy Huddleston Sequoia <jeremyhu@apple.com> writes:
>>> 
>>>> Useful for setting up osxkeychain in Xcode.app's gitconfig
>>>> 
>>>> Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
>>>> ---
>>> 
>>> A concern shared with 13/13 is this.
>>> 
>>> While it may not hurt too much to look at one extra location even on
>>> non-Apple platform, it probably is a mistake to have this xcode
>>> specific change in generic part of the system like config.c or
>>> attr.c.  For that matter, would it make sense to force Apple uses to
>>> look at one extra location in the first place?  In other words, we
>>> already have "system wide" location (i.e. system_path(ETC_GITCONFIG))
>>> defined so system owners can give reasonable default to its users.
>>> The value of not using that facility and instead adding yet another
>>> place is dubious.
>> 
>> This allows for per-distribution configuration and could be useful for
>> other applications as well that want customizations specific to their
>> install of git.  For our specific use case, we do not want to munge the
>> system policy when installing Xcode.  Prior to doing things this way, we
>> were just changing the default in our distributed git binary, but this
>> seems a bit more flexible.
> 
> I think you misunderstood Junio, thinking that he referred to
> /etc/gitconfig. He did not. system_path(ETC_GITCONFIG) refers to
> <prefix>/etc/gitconfig, where <prefix> is that runtime prefix when
> compiled with RUNTIME_PREFIX.

Oh!  Awesome.  I didn't even notice this was a thing.  That would exactly solve our use case.  I'll give that a whirl.  That likely allows us to eliminate these two patches completely!

> So you can definitely have your own per-distribution configuration: it
> lives in that very <prefix>/etc/gitconfig where the portable Git is
> installed.
> 
> And since we have that nice facility, I agree with Junio that we probably
> do not even need an extra config, certainly not one just introduced for
> XCode.
> 
> Ciao,
> Johannes
> 
>> 
>>> 
>>> 
>>> 
>>> 
>>> 
>>> 
>>> 
>>>> config.c | 13 +++++++++++++
>>>> config.h |  2 ++
>>>> 2 files changed, 15 insertions(+)
>>>> 
>>>> diff --git a/config.c b/config.c
>>>> index ff521eb27a..656bfef8ab 100644
>>>> --- a/config.c
>>>> +++ b/config.c
>>>> @@ -1631,6 +1631,14 @@ const char *git_etc_gitconfig(void)
>>>> 	return system_wide;
>>>> }
>>>> 
>>>> +const char *git_xcode_gitconfig(void)
>>>> +{
>>>> +	static const char *xcode_config;
>>>> +	if (!xcode_config)
>>>> +		xcode_config = system_path("share/git-core/gitconfig");
>>>> +	return xcode_config;
>>>> +}
>>>> +
>>>> /*
>>>> * Parse environment variable 'k' as a boolean (in various
>>>> * possible spellings); if missing, use the default value 'def'.
>>>> @@ -1673,6 +1681,11 @@ static int do_git_config_sequence(const struct config_options *opts,
>>>> 	else
>>>> 		repo_config = NULL;
>>>> 
>>>> +	current_parsing_scope = CONFIG_SCOPE_XCODE;
>>>> +	if (git_config_system() && git_xcode_gitconfig() && !access_or_die(git_xcode_gitconfig(), R_OK, 0))
>>>> +		ret += git_config_from_file(fn, git_xcode_gitconfig(),
>>>> +					    data);
>>>> +
>>>> 	current_parsing_scope = CONFIG_SCOPE_SYSTEM;
>>>> 	if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0))
>>>> 		ret += git_config_from_file(fn, git_etc_gitconfig(),
>>>> diff --git a/config.h b/config.h
>>>> index ee5d3fa7b4..f848423d28 100644
>>>> --- a/config.h
>>>> +++ b/config.h
>>>> @@ -115,6 +115,7 @@ extern int git_config_rename_section_in_file(const char *, const char *, const c
>>>> extern int git_config_copy_section(const char *, const char *);
>>>> extern int git_config_copy_section_in_file(const char *, const char *, const char *);
>>>> extern const char *git_etc_gitconfig(void);
>>>> +extern const char *git_xcode_gitconfig(void);
>>>> extern int git_env_bool(const char *, int);
>>>> extern unsigned long git_env_ulong(const char *, unsigned long);
>>>> extern int git_config_system(void);
>>>> @@ -131,6 +132,7 @@ enum config_scope {
>>>> 	CONFIG_SCOPE_GLOBAL,
>>>> 	CONFIG_SCOPE_REPO,
>>>> 	CONFIG_SCOPE_CMDLINE,
>>>> +	CONFIG_SCOPE_XCODE,
>>>> };
>>>> 
>>>> extern enum config_scope current_config_scope(void);
>> 
>>
Jeremy Sequoia Jan. 30, 2019, 10:01 p.m. UTC | #6
> On Jan 30, 2019, at 13:09, Jeremy Huddleston Sequoia <jeremyhu@apple.com> wrote:
> 
> 
> 
>> On Jan 30, 2019, at 11:32, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>> 
>> Hi Jeremy,
>> 
>> On Tue, 29 Jan 2019, Jeremy Huddleston Sequoia wrote:
>> 
>>>> On Jan 29, 2019, at 3:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>>> 
>>>> Jeremy Huddleston Sequoia <jeremyhu@apple.com> writes:
>>>> 
>>>>> Useful for setting up osxkeychain in Xcode.app's gitconfig
>>>>> 
>>>>> Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
>>>>> ---
>>>> 
>>>> A concern shared with 13/13 is this.
>>>> 
>>>> While it may not hurt too much to look at one extra location even on
>>>> non-Apple platform, it probably is a mistake to have this xcode
>>>> specific change in generic part of the system like config.c or
>>>> attr.c.  For that matter, would it make sense to force Apple uses to
>>>> look at one extra location in the first place?  In other words, we
>>>> already have "system wide" location (i.e. system_path(ETC_GITCONFIG))
>>>> defined so system owners can give reasonable default to its users.
>>>> The value of not using that facility and instead adding yet another
>>>> place is dubious.
>>> 
>>> This allows for per-distribution configuration and could be useful for
>>> other applications as well that want customizations specific to their
>>> install of git.  For our specific use case, we do not want to munge the
>>> system policy when installing Xcode.  Prior to doing things this way, we
>>> were just changing the default in our distributed git binary, but this
>>> seems a bit more flexible.
>> 
>> I think you misunderstood Junio, thinking that he referred to
>> /etc/gitconfig. He did not. system_path(ETC_GITCONFIG) refers to
>> <prefix>/etc/gitconfig, where <prefix> is that runtime prefix when
>> compiled with RUNTIME_PREFIX.
> 
> Oh!  Awesome.  I didn't even notice this was a thing.  That would exactly solve our use case.  I'll give that a whirl.  That likely allows us to eliminate these two patches completely!

Unfortunately, I was quick to celebrate.  This picks up the bundled file instead of a system-wide file.  I'd love it if we could still honor system-wide config/attributes in addition to RUNTIME_PREFIX-relative ones (eg: user overrides system which overrides distribution).  I worry that as is, we'd stop referencing the system-wide configs which might confuse users.

Is that something you'd be interested in, or should we just continue to maintain our separate patches?

> 
>> So you can definitely have your own per-distribution configuration: it
>> lives in that very <prefix>/etc/gitconfig where the portable Git is
>> installed.
>> 
>> And since we have that nice facility, I agree with Junio that we probably
>> do not even need an extra config, certainly not one just introduced for
>> XCode.
>> 
>> Ciao,
>> Johannes
>> 
>>> 
>>>> 
>>>> 
>>>> 
>>>> 
>>>> 
>>>> 
>>>> 
>>>>> config.c | 13 +++++++++++++
>>>>> config.h |  2 ++
>>>>> 2 files changed, 15 insertions(+)
>>>>> 
>>>>> diff --git a/config.c b/config.c
>>>>> index ff521eb27a..656bfef8ab 100644
>>>>> --- a/config.c
>>>>> +++ b/config.c
>>>>> @@ -1631,6 +1631,14 @@ const char *git_etc_gitconfig(void)
>>>>> 	return system_wide;
>>>>> }
>>>>> 
>>>>> +const char *git_xcode_gitconfig(void)
>>>>> +{
>>>>> +	static const char *xcode_config;
>>>>> +	if (!xcode_config)
>>>>> +		xcode_config = system_path("share/git-core/gitconfig");
>>>>> +	return xcode_config;
>>>>> +}
>>>>> +
>>>>> /*
>>>>> * Parse environment variable 'k' as a boolean (in various
>>>>> * possible spellings); if missing, use the default value 'def'.
>>>>> @@ -1673,6 +1681,11 @@ static int do_git_config_sequence(const struct config_options *opts,
>>>>> 	else
>>>>> 		repo_config = NULL;
>>>>> 
>>>>> +	current_parsing_scope = CONFIG_SCOPE_XCODE;
>>>>> +	if (git_config_system() && git_xcode_gitconfig() && !access_or_die(git_xcode_gitconfig(), R_OK, 0))
>>>>> +		ret += git_config_from_file(fn, git_xcode_gitconfig(),
>>>>> +					    data);
>>>>> +
>>>>> 	current_parsing_scope = CONFIG_SCOPE_SYSTEM;
>>>>> 	if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0))
>>>>> 		ret += git_config_from_file(fn, git_etc_gitconfig(),
>>>>> diff --git a/config.h b/config.h
>>>>> index ee5d3fa7b4..f848423d28 100644
>>>>> --- a/config.h
>>>>> +++ b/config.h
>>>>> @@ -115,6 +115,7 @@ extern int git_config_rename_section_in_file(const char *, const char *, const c
>>>>> extern int git_config_copy_section(const char *, const char *);
>>>>> extern int git_config_copy_section_in_file(const char *, const char *, const char *);
>>>>> extern const char *git_etc_gitconfig(void);
>>>>> +extern const char *git_xcode_gitconfig(void);
>>>>> extern int git_env_bool(const char *, int);
>>>>> extern unsigned long git_env_ulong(const char *, unsigned long);
>>>>> extern int git_config_system(void);
>>>>> @@ -131,6 +132,7 @@ enum config_scope {
>>>>> 	CONFIG_SCOPE_GLOBAL,
>>>>> 	CONFIG_SCOPE_REPO,
>>>>> 	CONFIG_SCOPE_CMDLINE,
>>>>> +	CONFIG_SCOPE_XCODE,
>>>>> };
>>>>> 
>>>>> extern enum config_scope current_config_scope(void);
>>> 
>>> 
>
Jonathan Nieder Jan. 31, 2019, 12:01 a.m. UTC | #7
Hi,

Jeremy Huddleston Sequoia wrote:

> Unfortunately, I was quick to celebrate.  This picks up the bundled
> file instead of a system-wide file.  I'd love it if we could still
> honor system-wide config/attributes in addition to
> RUNTIME_PREFIX-relative ones (eg: user overrides system which
> overrides distribution).  I worry that as is, we'd stop referencing
> the system-wide configs which might confuse users.
> 
> Is that something you'd be interested in, or should we just continue
> to maintain our separate patches?

For the internal deployment at Google, what we've done is to put an
[include] path directive in the global gitconfig:

	[include]
		path = /usr/share/git-core/config

Users can edit the global git config in etc, but the distributed
config at /usr/share/git-core/config is read-only as part of the
distributed package.

We considered making an upstream change to bake in the distributed
config in the git binary but decided that this way is a little
nicer since it lets people comment out the include.path setting if
they want to e.g. for experimentation.  It's also more explicit
(hence easier to understand).

Would a similar approach work for your setup?  Can you say a little
more about how you'd like things to work from an end-user pov?

Thanks,
Jonathan
Jeremy Sequoia Jan. 31, 2019, 8:29 a.m. UTC | #8
> On Jan 30, 2019, at 16:01, Jonathan Nieder <jrnieder@gmail.com> wrote:
> 
> Hi,
> 
> Jeremy Huddleston Sequoia wrote:
> 
>> Unfortunately, I was quick to celebrate.  This picks up the bundled
>> file instead of a system-wide file.  I'd love it if we could still
>> honor system-wide config/attributes in addition to
>> RUNTIME_PREFIX-relative ones (eg: user overrides system which
>> overrides distribution).  I worry that as is, we'd stop referencing
>> the system-wide configs which might confuse users.
>> 
>> Is that something you'd be interested in, or should we just continue
>> to maintain our separate patches?
> 
> For the internal deployment at Google, what we've done is to put an
> [include] path directive in the global gitconfig:
> 
> 	[include]
> 		path = /usr/share/git-core/config
> 
> Users can edit the global git config in etc, but the distributed
> config at /usr/share/git-core/config is read-only as part of the
> distributed package.
> 
> We considered making an upstream change to bake in the distributed
> config in the git binary but decided that this way is a little
> nicer since it lets people comment out the include.path setting if
> they want to e.g. for experimentation.  It's also more explicit
> (hence easier to understand).
> 
> Would a similar approach work for your setup?  Can you say a little
> more about how you'd like things to work from an end-user pov?

That might work.  I could put this in the Xcode.app gitconfig:

	[include]
		path = /private/etc/gitconfig

Would that result in /private/etc/gitconfig's taking precedence or Xcode.app's?
Is there anything analogous I could do for gitattributes?
Junio C Hamano Jan. 31, 2019, 6:06 p.m. UTC | #9
Jeremy Huddleston Sequoia <jeremyhu@apple.com> writes:

>>>> A concern shared with 13/13 is this.
>>>> 
>>>> While it may not hurt too much to look at one extra location even on
>>>> non-Apple platform, it probably is a mistake to have this xcode
>>>> specific change in generic part of the system like config.c or
>>>> attr.c.  For that matter, would it make sense to force Apple uses to
>>>> look at one extra location in the first place?  In other words, we
>>>> already have "system wide" location (i.e. system_path(ETC_GITCONFIG))
>>>> defined so system owners can give reasonable default to its users.
>>>> The value of not using that facility and instead adding yet another
>>>> place is dubious.
>>> 
>>> This allows for per-distribution configuration and could be useful for
>>> other applications as well that want customizations specific to their
>>> install of git.  For our specific use case, we do not want to munge the
>>> system policy when installing Xcode.  Prior to doing things this way, we
>>> were just changing the default in our distributed git binary, but this
>>> seems a bit more flexible.
>> 
>> I think you misunderstood Junio, thinking that he referred to
>> /etc/gitconfig. He did not. system_path(ETC_GITCONFIG) refers to
>> <prefix>/etc/gitconfig, where <prefix> is that runtime prefix when
>> compiled with RUNTIME_PREFIX.
>
> Oh!  Awesome.

I do not think you misunderstood.  system_path(ETC_GITCONFIG) may be
in <prefix>/etc/gitconfig when building with RUNTIME_PREFIX, but
then I do not think /etc/gitconfig (without <prefix>) comes into the
picture.  So as long as you want to add "a forced by distribution,
not editable by end user to set a global policy for the entire box"
configuration, that goes against the design of the configuration
system, which wants to have three levels (i.e. per repository, per
user and per box).

I think the arrangement jrnieder illustrates in a message in this
thread to use the inclusion of distro-provided file from
/etc/gitconfig, which documents what is happening clearly and still
allows the user to disable the distro-provided one if needed, is
probably the best solution under the current design.

Patch
diff mbox series

diff --git a/config.c b/config.c
index ff521eb27a..656bfef8ab 100644
--- a/config.c
+++ b/config.c
@@ -1631,6 +1631,14 @@  const char *git_etc_gitconfig(void)
 	return system_wide;
 }
 
+const char *git_xcode_gitconfig(void)
+{
+	static const char *xcode_config;
+	if (!xcode_config)
+		xcode_config = system_path("share/git-core/gitconfig");
+	return xcode_config;
+}
+
 /*
  * Parse environment variable 'k' as a boolean (in various
  * possible spellings); if missing, use the default value 'def'.
@@ -1673,6 +1681,11 @@  static int do_git_config_sequence(const struct config_options *opts,
 	else
 		repo_config = NULL;
 
+	current_parsing_scope = CONFIG_SCOPE_XCODE;
+	if (git_config_system() && git_xcode_gitconfig() && !access_or_die(git_xcode_gitconfig(), R_OK, 0))
+		ret += git_config_from_file(fn, git_xcode_gitconfig(),
+					    data);
+
 	current_parsing_scope = CONFIG_SCOPE_SYSTEM;
 	if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0))
 		ret += git_config_from_file(fn, git_etc_gitconfig(),
diff --git a/config.h b/config.h
index ee5d3fa7b4..f848423d28 100644
--- a/config.h
+++ b/config.h
@@ -115,6 +115,7 @@  extern int git_config_rename_section_in_file(const char *, const char *, const c
 extern int git_config_copy_section(const char *, const char *);
 extern int git_config_copy_section_in_file(const char *, const char *, const char *);
 extern const char *git_etc_gitconfig(void);
+extern const char *git_xcode_gitconfig(void);
 extern int git_env_bool(const char *, int);
 extern unsigned long git_env_ulong(const char *, unsigned long);
 extern int git_config_system(void);
@@ -131,6 +132,7 @@  enum config_scope {
 	CONFIG_SCOPE_GLOBAL,
 	CONFIG_SCOPE_REPO,
 	CONFIG_SCOPE_CMDLINE,
+	CONFIG_SCOPE_XCODE,
 };
 
 extern enum config_scope current_config_scope(void);