[PATCH/RFC 0/5] add "unset.variable" for unsetting previously set variables

[PATCH/RFC 0/5] add "unset.variable" for unsetting previously set variables

[Tanay Abhra]

Hi,

This series aims to add a method to filter previously set variables.
The patch series can be best described by the 3/5 log message
which I have pasted below verbatim.

"
Add a new config variable "unset.variable" which unsets previously set
variables. It affects `git_config()` and `git_config_get_*()` family
of functions. It removes the matching variables from the `configset`
which were added previously. Those matching variables which come after
the "unset.variable" in parsing order will not be deleted and will
be left untouched.

It affects the result of "git config -l" and similar calls.
It may be used in cases where the user can not access the config files,
for example, the system wide config files may be only accessible to
the system administrator. We can unset an unwanted variable declared in
the system config file by using "unset.variable" in a local config file.

for example, /etc/gitconfig may look like this,
	[foo]
		bar = baz

in the repo config file, we will write,
	[unset]
		variable  = foo.bar
to unset foo.bar previously declared in system wide config file.
"

Now, I have some points of
contention which I like to clarify,

1> The name of the variable, I could not decide between "unset.variable"
and "config.unset", or may be some other name would be more appropriate.

2> It affects both the C git_config() calls and, git config shell
invocations. Due to this some variables may be absent from the git config -l
result which might confuse the user.

3> I also have an another implementation for this series which just marks the config
variables instead of deleting them from the configset. This can be used to
provide two versions of git_config(), one with filtered variables other without
it.

4> While hacking on this series, I saw that git_config_int() does not print
the file name of the invalid variable when values are fed by the configset.
I will correct this regression in another patch.

Cheers,
Tanay


 Documentation/config.txt | 12 +++++++
 config.c                 | 93 +++++++++++++++++++++++++++++++++++++-----------
 t/t1300-repo-config.sh   | 56 ++++++++++++++++++++++++++++-
 3 files changed, 139 insertions(+), 22 deletions(-)

-- 
1.9.0.GIT

[PATCH/RFC 1/5] config.c : move configset_iter() to an appropriate position

[Tanay Abhra]

Move configset_iter() to an appropriate position where it
can be called by git_config_*() family without putting
a forward declaration for it. 

Helped-by: Matthieu Moy <Matthieu.Moy@imag.fr>
Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 config.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/config.c b/config.c
index a677eb6..cb474b2 100644
--- a/config.c
+++ b/config.c
@@ -1150,6 +1150,25 @@ int git_config_system(void)
 	return !git_env_bool("GIT_CONFIG_NOSYSTEM", 0);
 }
 
+static void configset_iter(struct config_set *cs, config_fn_t fn, void *data)
+{
+	int i, value_index;
+	struct string_list *values;
+	struct config_set_element *entry;
+	struct configset_list *list = &cs->list;
+	struct key_value_info *kv_info;
+
+	for (i = 0; i < list->nr; i++) {
+		entry = list->items[i].e;
+		value_index = list->items[i].value_index;
+		values = &entry->value_list;
+		if (fn(entry->key, values->items[value_index].string, data) < 0) {
+			kv_info = values->items[value_index].util;
+			git_die_config_linenr(entry->key, kv_info->filename, kv_info->linenr);
+		}
+	}
+}
+
 int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 {
 	int ret = 0, found = 0;
@@ -1245,25 +1264,6 @@ static void git_config_raw(config_fn_t fn, void *data)
 		die(_("unknown error occured while reading the configuration files"));
 }
 
-static void configset_iter(struct config_set *cs, config_fn_t fn, void *data)
-{
-	int i, value_index;
-	struct string_list *values;
-	struct config_set_element *entry;
-	struct configset_list *list = &cs->list;
-	struct key_value_info *kv_info;
-
-	for (i = 0; i < list->nr; i++) {
-		entry = list->items[i].e;
-		value_index = list->items[i].value_index;
-		values = &entry->value_list;
-		if (fn(entry->key, values->items[value_index].string, data) < 0) {
-			kv_info = values->items[value_index].util;
-			git_die_config_linenr(entry->key, kv_info->filename, kv_info->linenr);
-		}
-	}
-}
-
 static void git_config_check_init(void);
 
 void git_config(config_fn_t fn, void *data)
-- 
1.9.0.GIT

[PATCH/RFC 2/5] make git_config_with_options() to use a configset

[Tanay Abhra]

Make git_config_with_options() to use a configset to feed values
in the callback function. This change gives us the power to filter
variables we feed to the callback using custom constraints.

A slight behaviour change, git_config_int() loses the ability to
print the file name of the invalid variable while dying.

Helped-by: Matthieu Moy <Matthieu.Moy@imag.fr>
Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 config.c               | 21 +++++++++++++++++++--
 t/t1300-repo-config.sh |  2 +-
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/config.c b/config.c
index cb474b2..09cf009 100644
--- a/config.c
+++ b/config.c
@@ -1214,7 +1214,7 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 	return ret == 0 ? found : ret;
 }
 
-int git_config_with_options(config_fn_t fn, void *data,
+static int git_config_with_options_raw(config_fn_t fn, void *data,
 			    struct git_config_source *config_source,
 			    int respect_includes)
 {
@@ -1247,9 +1247,26 @@ int git_config_with_options(config_fn_t fn, void *data,
 	return ret;
 }
 
+static int config_set_callback(const char *key, const char *value, void *cb);
+
+int git_config_with_options(config_fn_t fn, void *data,
+			    struct git_config_source *config_source,
+			    int respect_includes)
+{
+	int ret;
+	struct config_set options_config;
+	git_configset_init(&options_config);
+	ret = git_config_with_options_raw(config_set_callback, &options_config,
+					  config_source, respect_includes);
+	if (ret >= 0)
+		configset_iter(&options_config, fn, data);
+	git_configset_clear(&options_config);
+	return ret;
+}
+
 static void git_config_raw(config_fn_t fn, void *data)
 {
-	if (git_config_with_options(fn, data, NULL, 1) < 0)
+	if (git_config_with_options_raw(fn, data, NULL, 1) < 0)
 		/*
 		 * git_config_with_options() normally returns only
 		 * positive values, as most errors are fatal, and
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 938fc8b..ce5ea01 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -678,7 +678,7 @@ test_expect_success 'invalid unit' '
 	git config aninvalid.unit >actual &&
 	test_cmp expect actual &&
 	cat >expect <<-\EOF
-	fatal: bad numeric config value '\''1auto'\'' for '\''aninvalid.unit'\'' in .git/config: invalid unit
+	fatal: bad numeric config value '\''1auto'\'' for '\''aninvalid.unit'\'': invalid unit
 	EOF
 	test_must_fail git config --int --get aninvalid.unit 2>actual &&
 	test_i18ncmp expect actual
-- 
1.9.0.GIT

[PATCH/RFC 3/5] add "unset.variable" for unsetting previously set variables

[Tanay Abhra]

Add a new config variable "unset.variable" which unsets previously set
variables. It affects `git_config()` and `git_config_get_*()` family
of functions. It removes the matching variables from the `configset`
which were added previously. Those matching variables which come after
the "unset.variable" in parsing order will not be deleted and will
be left untouched.

It affects the result of "git config -l" and similar calls.
It may be used in cases where the user can not access the config files,
for example, the system wide config files may be only accessible to
the system administrator. We can unset an unwanted variable declared in
the system config file by using "unset.variable" in a local config file.

for example, /etc/gitconfig may look like this,
	[foo]
		bar = baz

in the repo config file, we will write,
	[unset]
		variable  = foo.bar
to unset foo.bar previously declared in system wide config file.

Helped-by: Matthieu Moy <Matthieu.Moy@imag.fr>
Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 config.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/config.c b/config.c
index 09cf009..a80832d 100644
--- a/config.c
+++ b/config.c
@@ -1311,6 +1311,38 @@ static struct config_set_element *configset_find_element(struct config_set *cs,
 	return found_entry;
 }
 
+static void delete_config_variable(struct config_set *cs, const char *key, const char *value)
+{
+	char *normalized_value;
+	struct config_set_element *e = NULL;
+	int ret, current = 0, updated = 0;
+	struct configset_list *list = &cs->list;
+	/*
+	 * if we find a key value pair with key as "unset.variable", unset all variables
+	 * in the configset with keys equivalent to the value in "unset.variable".
+	 * unsetting a variable means that the variable is permanently deleted from the
+	 * configset.
+	 */
+	ret = git_config_parse_key(value, &normalized_value, NULL);
+	if (!ret) {
+		/* first remove matching variables from the configset_list */
+		while (current < list->nr) {
+			if (!strcmp(list->items[current].e->key, normalized_value))
+				current++;
+			else
+				list->items[updated++] = list->items[current++];
+		}
+		list->nr = updated;
+		/* then delete the matching entry from the configset hashmap */
+		e = configset_find_element(cs, normalized_value);
+		if (e) {
+			free(e->key);
+			string_list_clear(&e->value_list, 1);
+			hashmap_remove(&cs->config_hash, e, NULL);
+		}
+	}
+}
+
 static int configset_add_value(struct config_set *cs, const char *key, const char *value)
 {
 	struct config_set_element *e;
@@ -1331,6 +1363,8 @@ static int configset_add_value(struct config_set *cs, const char *key, const cha
 		hashmap_add(&cs->config_hash, e);
 	}
 	si = string_list_append_nodup(&e->value_list, value ? xstrdup(value) : NULL);
+	if (!strcmp(key, "unset.variable"))
+		delete_config_variable(cs, key, value);
 
 	ALLOC_GROW(cs->list.items, cs->list.nr + 1, cs->list.alloc);
 	l_item = &cs->list.items[cs->list.nr++];
-- 
1.9.0.GIT

[PATCH/RFC 4/5] document the new "unset.variable" variable

[Tanay Abhra]

Helped-by: Matthieu Moy <Matthieu.Moy@imag.fr>
Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 Documentation/config.txt | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 3b5b24a..7f36d35 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2382,6 +2382,18 @@ transfer.unpackLimit::
 	not set, the value of this variable is used instead.
 	The default value is 100.
 
+unset.variable::
+	This variable can be used to unset previously set variables
+	which had been already declared in files of lower priority
+	or declared before in the same file. It does not unset
+	matching variables declared after its position in the file
+	or in files of higher priority. It can be used to unset
+	pesky variables declared in files which the user might not
+	be able to open due to not having the required security
+	privileges, for example, system wide configuration file
+	`/etc/gitconfig` which may be accessible to the system
+	administrator only.
+
 uploadarchive.allowUnreachable::
 	If true, allow clients to use `git archive --remote` to request
 	any tree, whether reachable from the ref tips or not. See the
-- 
1.9.0.GIT

[PATCH/RFC 5/5] add tests for checking the behaviour of "unset.variable"

[Tanay Abhra]

Helped-by: Matthieu Moy <Matthieu.Moy@imag.fr>
Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 t/t1300-repo-config.sh | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index ce5ea01..f75c001 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1179,4 +1179,58 @@ test_expect_success POSIXPERM,PERL 'preserves existing permissions' '
 	  "die q(badrename) if ((stat(q(.git/config)))[2] & 07777) != 0600"
 '
 
+test_expect_success 'unset.variable unsets all previous matching keys' '
+	cat >.git/config <<-\EOF &&
+	[alias]
+		checkconfig = -c foo.check=baz config foo.check
+		checkconfig = -c foo.check=bar config foo.check
+	[unset]
+		variable = alias.checkconfig
+	EOF
+
+	test_expect_code 1 git checkconfig
+'
+
+test_expect_success 'unset.variable does not touch all matching keys after it' '
+	cat >.git/config <<-\EOF &&
+	[alias]
+		checkconfig = -c foo.check=foo config foo.check
+	[unset]
+		variable = alias.checkconfig
+	[alias]
+		checkconfig = -c foo.check=baz config foo.check
+		checkconfig = -c foo.check=bar config foo.check
+	EOF
+
+	cat >expect <<-\EOF &&
+	bar
+	EOF
+
+	test_expect_code 0 git checkconfig >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'document how unset.variable will behave in shell scripts' '
+	rm -f .git/config &&
+	cat >expect <<-\EOF &&
+	EOF
+	git config foo.bar boz1 &&
+	git config --add foo.bar boz2 &&
+	git config unset.variable foo.bar &&
+	git config --add foo.bar boz3 &&
+	test_must_fail git config --get-all foo.bar >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'unset.variable declared after in shell scripts' '
+	rm -f .git/config &&
+	cat >expect <<-\EOF &&
+	EOF
+	git config foo.bar boz1 &&
+	git config --add foo.bar boz2 &&
+	git config unset.variable foo.bar &&
+	test_must_fail git config --get-all foo.bar >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
1.9.0.GIT

Re: [PATCH/RFC 5/5] add tests for checking the behaviour of "unset.variable"

[Junio C Hamano]

Tanay Abhra <tanayabh@gmail.com> writes:

> +test_expect_success 'document how unset.variable will behave in shell scripts' '
> +	rm -f .git/config &&
> +	cat >expect <<-\EOF &&
> +	EOF
> +	git config foo.bar boz1 &&
> +	git config --add foo.bar boz2 &&
> +	git config unset.variable foo.bar &&
> +	git config --add foo.bar boz3 &&
> +	test_must_fail git config --get-all foo.bar >actual &&

You make foo.bar a multi-valued one, then you unset it, so I would
imagine that the value given after that, 'boz3', would be the only
value foo.bar has.  Why should --get-all fail?

I am having a hard time imagining how this behaviour can make any
sense.

Re: [PATCH/RFC 5/5] add tests for checking the behaviour of "unset.variable"

[Tanay Abhra]

On 10/3/2014 1:39 AM, Junio C Hamano wrote:
> Tanay Abhra <tanayabh@gmail.com> writes:
> 
>> +test_expect_success 'document how unset.variable will behave in shell scripts' '
>> +	rm -f .git/config &&
>> +	cat >expect <<-\EOF &&
>> +	EOF
>> +	git config foo.bar boz1 &&
>> +	git config --add foo.bar boz2 &&
>> +	git config unset.variable foo.bar &&
>> +	git config --add foo.bar boz3 &&
>> +	test_must_fail git config --get-all foo.bar >actual &&
> 
> You make foo.bar a multi-valued one, then you unset it, so I would
> imagine that the value given after that, 'boz3', would be the only
> value foo.bar has.  Why should --get-all fail?
>
> I am having a hard time imagining how this behaviour can make any
> sense.
> 

git config -add appends the value to a existing header, after these
two commands have executed the config file would look like,

git config foo.bar boz1 &&
git config --add foo.bar boz2 &&

[foo]
	bar = boz1
	bar = boz2

After git config unset.variable foo.bar,

[foo]
	bar = boz1
	bar = boz2
[unset]
	variable = foo.bar

Now the tricky part, git config --add foo.bar boz3 append to the
existing header,

[foo]
	bar = boz1
	bar = boz2
	bar = boz3
[unset]
	variable = foo.bar

Since unset.variable unsets all previous set values in parsing order,
git config --get-all foo.bar gives us nothing in result.

Re: [PATCH/RFC 5/5] add tests for checking the behaviour of "unset.variable"

[Junio C Hamano]

Tanay Abhra <tanayabh@gmail.com> writes:

> On 10/3/2014 1:39 AM, Junio C Hamano wrote:
>> Tanay Abhra <tanayabh@gmail.com> writes:
>> 
>>> +test_expect_success 'document how unset.variable will behave in shell scripts' '
>>> +	rm -f .git/config &&
>>> +	cat >expect <<-\EOF &&
>>> +	EOF
>>> +	git config foo.bar boz1 &&
>>> +	git config --add foo.bar boz2 &&
>>> +	git config unset.variable foo.bar &&
>>> +	git config --add foo.bar boz3 &&
>>> +	test_must_fail git config --get-all foo.bar >actual &&
>> 
>> You make foo.bar a multi-valued one, then you unset it, so I would
>> imagine that the value given after that, 'boz3', would be the only
>> value foo.bar has.  Why should --get-all fail?
>>
>> I am having a hard time imagining how this behaviour can make any
>> sense.
>> 
>
> git config -add appends the value to a existing header, after these
> two commands have executed the config file would look like,
> ...

I *know* how it is implemented before the changes of this series.
And if the original implementation of "add" is left as-is, I can
imagine how such a behaviour that is unintuitive to end-users can
arise.

I was and am having a hard time how this behaviour can make any
sense from an end-user's point of view.

Re: [PATCH/RFC 5/5] add tests for checking the behaviour of "unset.variable"

[Tanay Abhra]

On 10/3/2014 1:53 AM, Junio C Hamano wrote:
> Tanay Abhra <tanayabh@gmail.com> writes:
> 
>> On 10/3/2014 1:39 AM, Junio C Hamano wrote:
>>> Tanay Abhra <tanayabh@gmail.com> writes:
>>>
>>>> +test_expect_success 'document how unset.variable will behave in shell scripts' '
>>>> +	rm -f .git/config &&
>>>> +	cat >expect <<-\EOF &&
>>>> +	EOF
>>>> +	git config foo.bar boz1 &&
>>>> +	git config --add foo.bar boz2 &&
>>>> +	git config unset.variable foo.bar &&
>>>> +	git config --add foo.bar boz3 &&
>>>> +	test_must_fail git config --get-all foo.bar >actual &&
>>>
>>> You make foo.bar a multi-valued one, then you unset it, so I would
>>> imagine that the value given after that, 'boz3', would be the only
>>> value foo.bar has.  Why should --get-all fail?
>>>
>>> I am having a hard time imagining how this behaviour can make any
>>> sense.
>>>
>>
>> git config -add appends the value to a existing header, after these
>> two commands have executed the config file would look like,
>> ...
> 
> I *know* how it is implemented before the changes of this series.
> And if the original implementation of "add" is left as-is, I can
> imagine how such a behaviour that is unintuitive to end-users can
> arise.
> 
> I was and am having a hard time how this behaviour can make any
> sense from an end-user's point of view.
>

That's what I was trying to document. I think that it makes no sense
to use the feature as I had shown above. I had envisioned "unset.variable" to
be explicitly typed in on the appropriate place as can be seen in the first
two tests but Matthieu had suggested to add tests using git config too. This
is when I had discovered these inconsistencies.

I can think of two solutions, one leave it as it is and advertise it to be
explicitly typed in the config files at the appropriate position or to change
the behavior of unset.variable to unset all matching variables in that file,
before and after. We could also change git config --add to append at the end
of the file regardless the variable exists or not. Which course of action
do you think would be best?

Re: [PATCH/RFC 5/5] add tests for checking the behaviour of "unset.variable"

[Junio C Hamano]

Tanay Abhra <tanayabh@gmail.com> writes:

> I can think of two solutions, one leave it as it is and advertise it to be
> explicitly typed in the config files at the appropriate position or to change
> the behavior of unset.variable to unset all matching variables in that file,
> before and after. We could also change git config --add to append at the end
> of the file regardless the variable exists or not. Which course of action
> do you think would be best?

Off the top of my head, from an end-user's point of view, something
like this would give a behaviour that is at least understandable:

 (1) forbid "git config" command line from touching "unset.var", as
     there is no way for a user to control where a new unset.var
     goes.  And

 (2) When adding or appending section.var (it may also apply to
     removing one--you need to think about it deeper), ignore
     everything that comes before the last appearance of "unset.var"
     that unsets the "section.var" variable.

That way, if you do not have "[section]" after "[unset] variable =
section.var", you would end up adding a new "[section] var = value",
and if you already have "[section]", you would add a "var = value"
in that existing "[section]" that appears after the last unset of
the variable, so eerything will be kept neat.

Alternatively, if the syntax to unset a "section.var" were not

	[unset]
        	variable = section.var

but rather

	[section]
		! variable

or soemthing, then the current "find the section and append at the
end" code may work as-is.

Re: [PATCH/RFC 5/5] add tests for checking the behaviour of "unset.variable"

[Matthieu Moy]

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

> Tanay Abhra <tanayabh@gmail.com> writes:
>
>> I can think of two solutions, one leave it as it is and advertise it to be
>> explicitly typed in the config files at the appropriate position or to change
>> the behavior of unset.variable to unset all matching variables in that file,
>> before and after. We could also change git config --add to append at the end
>> of the file regardless the variable exists or not. Which course of action
>> do you think would be best?
>
> Off the top of my head, from an end-user's point of view, something
> like this would give a behaviour that is at least understandable:
>
>  (1) forbid "git config" command line from touching "unset.var", as
>      there is no way for a user to control where a new unset.var
>      goes.  And

Well, the normal use-case for unset.variable is to put it in a local
config file, to unset a variable set in another, lower-priority file.

This common use-case works with the command-line "git config", and it
would be a pity to forbid the common use-case because of a particular,
unusual case.

>  (2) When adding or appending section.var (it may also apply to
>      removing one--you need to think about it deeper), ignore
>      everything that comes before the last appearance of "unset.var"
>      that unsets the "section.var" variable.

That would probably be the best option from a user's point of view, but
I'd say the implementation complexity is not worth the trouble.

> Alternatively, if the syntax to unset a "section.var" were not
>
> 	[unset]
>         	variable = section.var
>
> but rather
>
> 	[section]
> 		! variable
>
> or soemthing, then the current "find the section and append at the
> end" code may work as-is.

But that would break backward compatibility rather badly: old git's
would stop working completely in repositories using this syntax.

Well, perhaps we can also consider that this is acceptable: just don't
use the feature for a few years if you care about backward
compatibility.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH/RFC 5/5] add tests for checking the behaviour of "unset.variable"

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Junio C Hamano <gitster@pobox.com> writes:
> ...
>> Off the top of my head, from an end-user's point of view, something
>> like this would give a behaviour that is at least understandable:

Let's make sure we have the same starting point (i.e. understanding
of the limitation of the current code) in mind.

The "git config [--add] section.var value" UI, which does not give
the user any control over where the new definition goes in the
resulting file, and the current implementation, which finds the last
existing "section" and adds the "var = value" definition at the end
(or adds a "section" at the end and then adds the definition there),
are adequate for ordinary variables.

It is fine for single-valued ones that follow "the last one wins"
semantics; "git config" would add the new definition at the end and
that definition will win.

It is manageable for multi-valued variables, too.  For uses of a
multi-valued variable to track unordered set of values, by
definition the order does not matter.

	Side note.  Otherwise, the scripter needs to read the
	existing ones, --unset-all them, and then add the elements
	of the final list in desired order.  It is cumbersome, but
	for a single multi-valued variable it is manageable.

But the "unset.variable" by nature wants a lot finer-grained control
over the order in which other ordinary variables are defined and it
itself is placed.  No matter what improvements you attempt to make
to the implementation, because the UI is not getting enough
information from the user to learn where exactly the user wants to
add a new variable or "unset.variable", it would not give you enough
flexibility to do the job.

>>  (1) forbid "git config" command line from touching "unset.var", as
>>      there is no way for a user to control where a new unset.var
>>      goes.  And
>
> Well, the normal use-case for unset.variable is to put it in a local
> config file, to unset a variable set in another, lower-priority file.

I agree that is one major use case.

> This common use-case works with the command-line "git config", and it
> would be a pity to forbid the common use-case because of a particular,
> unusual case.

Either you are being incoherent or I am not reading you right.  If
you said "If this common use-case worked with the command-line 'git
config', it would be nice, but it would be a pity because it does
not", I would understand.

If you use the command line 'git config', i.e.

	git config unset.variable xyzzy.frotz

in a repository whose .git/config does not have any unset.variable,
you will add that _at the end_, which would undo what you did in
your configuration file, not just what came before yours.  Even if
you ignore more exotic cases, the command line is *not* working.

That is why I said "unset.variable" is unworkable with existing "git
config" command line.  Always appending at the end is usable for
ordinary variables, but for unset.variable, it is most likely the
least useful thing to do.  You can explain "among 47 different
things it could do, we chose to do the most useless thing, because
that is _consistent_ with how the ordinary variables are placed in
the cofiguration file" in the documentation but it forgets to
question if unset.variable should be treated the same way as
ordinary variables in the first place.

Another use case would be to override what includes gave us.  I.e.

	[unset]
        	variable = ... nullify some /etc/gitconfig values ...
	[include]
        	path = ~/.gitcommon
	[unset]
		variable = ... nullify some ~/.gitcommon values ...
	[xyzzy]
		frotz = nitfol

Special-casing unset.variable and always adding them at the
beginning of the output *might* turn it from totally useless to
slightly usable.  At least it supports the "nullify previous ones",
even though it does not help "nullify after include".

I doubt if such a change to add unset.variable always at the top is
worth it, though.

>>  (2) When adding or appending section.var (it may also apply to
>>      removing one--you need to think about it deeper), ignore
>>      everything that comes before the last appearance of "unset.var"
>>      that unsets the "section.var" variable.
>
> That would probably be the best option from a user's point of view, but
> I'd say the implementation complexity is not worth the trouble.

test_expect_success declares a particular behaviour as "the right
thing to happen" in order to protect that right behaviour from
future changes.  It is OK for you to illustrate what illogical thing
the implementation does as a caveat, and admit that the behaviour
comes because we consider the change is not worth the trouble and we
punted.  But pretending as if that is the right behaviour and the
system has to promise that the broken behaviour will be kept forever
by casting it in stone is the worst thing you can do, I am afraid.

Unfortunately, as I already said, there is no sensible behaviour to
add "unset.variable" from the command line with the current "git
config" UI, so it is not even feasible to define "the right thing to
happen" and to document it as something other people may want to fix
later, e.g.

	test_expect_failure 'natural but not working yet' '
		git config xyzzy.frotz 1
                git config --add xyzzy.frotz 2
                git config --add xyzzy.frotz 3
                : a magic command to add
                :  [unset] variable = xyzzy.frotz 
                : between 2 and 3
		git config xyzzy.frotz >actual
                echo 3 >expect
                test_expect_success expect actual
	'

However, you should be able to arrange in the test to do the
following sequence:

    - Define "[xyzzy] frotz 1" in $HOME/.gitconfig (I think $HOME
      defaults to your trash directory).

    - Verify that "git config xyzzy.frotz" gives "1".

    - Define "[unset] variable = xyzzy.frotz" in .git/config (it is
      OK to use "git config unset.variable xyzzy.frotz" here).

    - Verify that "git config xyzzy.frotz" does not find anything.

    - Define "[xyzzy] frotz 2" in .git/config (again, it is OK to
      use "git config xyzzy.frotz 2" here).

    - Verify that "git config xyzzy.frotz" gives "2".

I think we can agree that the above sequence is something we would
want to support, regardless of how we will change/fix the underlying
config-writer implementation.  Which means that something like the
above can safely be cast in stone with test_expect_success.

Re: [PATCH/RFC 5/5] add tests for checking the behaviour of "unset.variable"

[Junio C Hamano]

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

> That is why I said "unset.variable" is unworkable with existing "git
> config" command line.  Always appending at the end is usable for
> ordinary variables, but for unset.variable, it is most likely the
> least useful thing to do.  You can explain "among 47 different
> things it could do, we chose to do the most useless thing, because
> that is _consistent_ with how the ordinary variables are placed in
> the cofiguration file" in the documentation but it forgets to
> question if unset.variable should be treated the same way as
> ordinary variables in the first place.

This is a tangent, but the above also applies to the "include.path".

Re: [PATCH/RFC 5/5] add tests for checking the behaviour of "unset.variable"

[Matthieu Moy]

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

> The "git config [--add] section.var value" UI, [...] finds the "var = value"
> definition at the end (or adds a "section" at the end and then adds
> [...]
>
> It is fine for single-valued ones that follow "the last one wins"
> semantics; "git config" would add the new definition at the end and
> that definition will win.

Not always.

git config foo.bar old-value
git config unset.variable foo.bar
git config foo.bar new-value

One could expect the new value to be taken into account, but it is not.

>> Well, the normal use-case for unset.variable is to put it in a local
>> config file, to unset a variable set in another, lower-priority file.
>
> I agree that is one major use case.
>
>> This common use-case works with the command-line "git config", and it
>> would be a pity to forbid the common use-case because of a particular,
>> unusual case.
>
> Either you are being incoherent or I am not reading you right.  If
> you said "If this common use-case worked with the command-line 'git
> config', it would be nice, but it would be a pity because it does
> not", I would understand.

I think you missed the "another" in my sentence above. The normal
use-case is to have foo.bar and unset.variable=foo.bar in different
files. In this case, you do not care about the position in file.

> in a repository whose .git/config does not have any unset.variable,
> you will add that _at the end_, which would undo what you did in
> your configuration file, not just what came before yours.  Even if
> you ignore more exotic cases, the command line is *not* working.

If my sysadmin has set foo.bar=boz in /etc/gitconfig, I can use

  git config [--global] unset.variable foo.bar

and it does work. Always.

Playing with the order of variables in-file is essentially useless OTOH
except for the include case you mentionned (if I want to unset a
variable in a file, I'll just delete or comment out the variable and I
don't need unset.variable).

Really, I don't see the point in making any complex plans to support the
useless part of the unset.variable feature. The reason it was designed
for already works, and $EDITOR does the job for other cases.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH/RFC 5/5] add tests for checking the behaviour of "unset.variable"

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> The "git config [--add] section.var value" UI, [...] finds the "var = value"
>> definition at the end (or adds a "section" at the end and then adds
>> [...]
>>
>> It is fine for single-valued ones that follow "the last one wins"
>> semantics; "git config" would add the new definition at the end and
>> that definition will win.
>
> Not always.
>
> git config foo.bar old-value
> git config unset.variable foo.bar
> git config foo.bar new-value
>
> One could expect the new value to be taken into account, but it is not.

I think you misunderstood what I said.  With ordinary variables,
everything works fine, that is, without unset.variable, which this
series is trying to pretend as if it were just another ordinary
variable, but it is not.  You are just showing how broken it is to
treat unset.variable as just another ordinary variable, which is
what I've been telling you.

So we are in agreement.

>>> Well, the normal use-case for unset.variable is to put it in a local
>>> config file, to unset a variable set in another, lower-priority file.
>>
>> I agree that is one major use case.
>>
>>> This common use-case works with the command-line "git config", and it
>>> would be a pity to forbid the common use-case because of a particular,
>>> unusual case.
>>
>> Either you are being incoherent or I am not reading you right.  If
>> you said "If this common use-case worked with the command-line 'git
>> config', it would be nice, but it would be a pity because it does
>> not", I would understand.
>
> I think you missed the "another" in my sentence above. The normal
> use-case is to have foo.bar and unset.variable=foo.bar in different
> files. In this case, you do not care about the position in file.

I didn't miss anything.  The reason you want to have "unset foo.bar"
in your .git/config is to override a "foo.bar = z" you have in
another place, e.g. ~/.gitconfig; which would let you sanely do
another "foo.bar = x" in .git/config for multi-value foo.bar, *if*
the unset comes before foo.bar.

But what if you have this in your .git/config file

	[core]
		bare = false
            ... standard stuff left by git init ...
	[foo]
        	bar = x

before you add "unset foo.bar" there?

And that is not a contribed example.

The likely reason why you want to resort to "unset foo.bar" is that
you found that you get an unwanted foo.bar=z in addition to desired
foo.bar=z in a repository that has the above in .git/config, and at
that point you would want to say "I want to unset the outside
influence".

And there is no "[unset] variable = foo.bar" in there yet; without
some special casing, if you treated unset.variable as if it were
just another ordinary variable, it would go after you define your
own foo.bar=x, which is not what you want.

Re: [PATCH/RFC 5/5] add tests for checking the behaviour of "unset.variable"

[Tanay Abhra]

On 10/4/2014 1:42 AM, Junio C Hamano wrote:
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> 
>> Junio C Hamano <gitster@pobox.com> writes:
>>>> Well, the normal use-case for unset.variable is to put it in a local
>>>> config file, to unset a variable set in another, lower-priority file.
>>>
>>> I agree that is one major use case.
>>>
>>>> This common use-case works with the command-line "git config", and it
>>>> would be a pity to forbid the common use-case because of a particular,
>>>> unusual case.
>>>
>>> Either you are being incoherent or I am not reading you right.  If
>>> you said "If this common use-case worked with the command-line 'git
>>> config', it would be nice, but it would be a pity because it does
>>> not", I would understand.
>>
>> I think you missed the "another" in my sentence above. The normal
>> use-case is to have foo.bar and unset.variable=foo.bar in different
>> files. In this case, you do not care about the position in file.
> 
> I didn't miss anything.  The reason you want to have "unset foo.bar"
> in your .git/config is to override a "foo.bar = z" you have in
> another place, e.g. ~/.gitconfig; which would let you sanely do
> another "foo.bar = x" in .git/config for multi-value foo.bar, *if*
> the unset comes before foo.bar.
> 
> But what if you have this in your .git/config file
> 
> 	[core]
> 		bare = false
>             ... standard stuff left by git init ...
> 	[foo]
>         	bar = x
> 
> before you add "unset foo.bar" there?
> 
> And that is not a contribed example.
> 
> The likely reason why you want to resort to "unset foo.bar" is that
> you found that you get an unwanted foo.bar=z in addition to desired
> foo.bar=z in a repository that has the above in .git/config, and at
> that point you would want to say "I want to unset the outside
> influence".
> 
> And there is no "[unset] variable = foo.bar" in there yet; without
> some special casing, if you treated unset.variable as if it were
> just another ordinary variable, it would go after you define your
> own foo.bar=x, which is not what you want.

I have made some conclusions after reading the whole thread,

1> Add some tests for this use case which seems the most appropriate
use case for this feature,

(Copied from Junio's mail)

    - Define "[xyzzy] frotz 1" in $HOME/.gitconfig (I think $HOME
      defaults to your trash directory).

    - Verify that "git config xyzzy.frotz" gives "1".

    - Define "[unset] variable = xyzzy.frotz" in .git/config (it is
      OK to use "git config unset.variable xyzzy.frotz" here).

    - Verify that "git config xyzzy.frotz" does not find anything.

    - Define "[xyzzy] frotz 2" in .git/config (again, it is OK to
      use "git config xyzzy.frotz 2" here).

    - Verify that "git config xyzzy.frotz" gives "2".

2> Leave the internal implementation as it is, as it helps in manually
writing unset.variable in its appropriate place by using an editor,
i.e this use case,

	[unset]
        	variable = ... nullify some /etc/gitconfig values ...
	[include]
        	path = ~/.gitcommon
	[unset]
		variable = ... nullify some ~/.gitcommon values ...
	[xyzzy]
		frotz = nitfol

3> Special case "unset.variable", so that git config unset.variable foo.baz
pastes it on the top of the file. The implementation should be trivial, but,
Junio had said in an earlier mail that he doesn't think this approach would
do much good.

Other than this approach, Junio had suggested to append it after the last mention
of "foo.baz", but I don't think if it would be worth it, but still I will cook up
the code for it.

4> Change the name to config.unset or something.

I will make the above changes in the next revision.

Thanks.

Re: [PATCH/RFC 5/5] add tests for checking the behaviour of "unset.variable"

[Junio C Hamano]

On Mon, Oct 6, 2014 at 11:59 AM, Tanay Abhra <tanayabh@gmail.com> wrote:
> 3> Special case "unset.variable", so that git config unset.variable foo.baz
> pastes it on the top of the file. The implementation should be trivial, but,
> Junio had said in an earlier mail that he doesn't think this approach would
> do much good.
>
> Other than this approach, Junio had suggested to append it after the last mention
> of "foo.baz",...

Just to make sure there is no misunderstanding.

"it" in "append it" above does not refer to unset.variable, and
"the last mention of "foo.baz"" is not "[foo]baz=value"

The point is to prevent"git config --add foo.baz anothervalue" starting from

--- --- ---
[foo]
  bar = some
[unset] variable = foo.baz
--- --- ---

from adding foo.baz next to existing foo.bar. We would want to end up with

--- --- ---
[foo]
  bar = some
[unset] variable = foo.baz
[foo]
  baz = anothervalue
--- --- ---

So "When dealing with foo.baz, ignore everything above the last unset.variable
that unsets foo.baz" is what I meant (and I think that is how I wrote).

Re: [PATCH/RFC 5/5] add tests for checking the behaviour of "unset.variable"

[Tanay Abhra]

On 10/7/2014 12:58 AM, Junio C Hamano wrote:
> 
> The point is to prevent"git config --add foo.baz anothervalue" starting from
> 
> --- --- ---
> [foo]
>   bar = some
> [unset] variable = foo.baz
> --- --- ---
> 
> from adding foo.baz next to existing foo.bar. We would want to end up with
> 
> --- --- ---
> [foo]
>   bar = some
> [unset] variable = foo.baz
> [foo]
>   baz = anothervalue
> --- --- ---
> 
> So "When dealing with foo.baz, ignore everything above the last unset.variable
> that unsets foo.baz" is what I meant (and I think that is how I wrote).
>


Yes, that was what I inferred too, I should have worded it more carefully, thanks.

Re: [PATCH/RFC 0/5] add "unset.variable" for unsetting previously set variables

[Junio C Hamano]

Tanay Abhra <tanayabh@gmail.com> writes:

(just this point quick)

> 1> The name of the variable, I could not decide between "unset.variable"
> and "config.unset", or may be some other name would be more appropriate.

I'd prefer to see this as [config] something.

I wish we did the include as "[config] include = path/to/filename",
not as "[include] path = path/to/filename".  Perhaps we can deprecate
and move it over time?

Re: [PATCH/RFC 0/5] add "unset.variable" for unsetting previously set variables

[Jeff King]

On Thu, Oct 02, 2014 at 12:29:14PM -0700, Junio C Hamano wrote:

> Tanay Abhra <tanayabh@gmail.com> writes:
> 
> (just this point quick)
> 
> > 1> The name of the variable, I could not decide between "unset.variable"
> > and "config.unset", or may be some other name would be more appropriate.
> 
> I'd prefer to see this as [config] something.
> 
> I wish we did the include as "[config] include = path/to/filename",
> not as "[include] path = path/to/filename".  Perhaps we can deprecate
> and move it over time?

I chose [include] because I had intended there to be multiple include
variables (include.path, include.ref, etc). The others were shot down
for now. If we put it under [config], I'd still prefer to leave room by
calling it:

  [config]
  includePath = path/to/filename

I also wanted [include] as a section name to leave room for
conditional includes. We've sometimes discussed things like:

  [include "has-some-git-feature"]
  path = ...

to allow conditional inclusion only when git supports a certain
feature-set (so that your config doesn't cause git to blow up when you
use an old version of git). It's possible that I'm the only person in
the world who really wants that, because I run old git versions all the
time for testing and debugging. And it is kind of gross as a syntax. But
it would still be nice to leave room for it.

I don't think there's a reason we couldn't allow:

  [config "condition..."]
  includePath = ...

in the same way if we wanted to (though aside from includes, I do not
know of any other feature that would want the condition).

So I'm not _opposed_ to adding [config], deprecating [include], and
waiting a bit before dropping [include]. But I also don't really see the
current name as a particularly bad thing.

-Peff

Re: [PATCH/RFC 0/5] add "unset.variable" for unsetting previously set variables

[Junio C Hamano]

Tanay Abhra <tanayabh@gmail.com> writes:

> 2> It affects both the C git_config() calls and, git config shell
> invocations. Due to this some variables may be absent from the git config -l
> result which might confuse the user.

I am not sure what you mean by this.  If you process variable
definitions as they come, and you read

	[some]
		variable = set to some value initially
                ...
	[unset]
		variable = some.variable
		...
	[some]
		variable = set to some other value

then a user might be able to see

	$ git config -l
        some.variable=set to some value initially
        !some.variable
	some.variable=set to some other value

(here, I am using an imaginary "!variable.name" to denote "this
variable is unset at this point in the reading sequence").

I would imagine if the result comes from a caching layer, the user
would see

	$ git config -l
	some.variable=set to some other value

and nothing else.  Is that what you are referring to?

I would think that the latter is probably desirable; otherwise we
would need to come up with a way to say "forget about everything we
said about this variable so far" (i.e. my "!some.variable" above)
and also the scripts that parse "git config -l" output need to code
the logic to forget and start afresh.

> 3> I also have an another implementation for this series which just marks the config
> variables instead of deleting them from the configset. This can be used to
> provide two versions of git_config(), one with filtered variables other without
> it.

I do not see a value in the filtered version.

What worries me _more_ is why people may want to put things in
system-wide global, and if it is a wise thing to do to allow users
to override.

We may later want to add ways to mark variables in various ways,
e.g. (thinking aloud)

 - "[config] sealed = section.variable" will prevent the variable
   from being reset, modified or appended.  If an administrator
   wants to enforce a certain setting in /etc/gitconfig, she may
   mark sensitive variables as cofnig.sealed at the end of the file:

	[security]
        	enforced = true
                ...
	[config]
        	sealed = security.enforced

   and we would ignore 

	[config]
        	unset = security.enforce
        [security]
		enforce = false

   written in .git/config or ~/.gitconfig, perhaps?

 - "[config] safeInclude = path" will allow a configuration file to
   be included safely from the project's working tree.  The path
   given as the value must be a relative path and it is relative to
   the top level of the project's working tree.

 - "[config] safe = section.variable" will list variables that can
   be included with the config.safeInclude mechanism.  Any variable
   that is not marked as config.safe that appears in the file
   included by the config.safeInclude mechanism will be ignored.

   The 'frotz' project might have a .frotz-gitconfig file at the
   root level of its working tree that stores this:

	[diff]
        	renames = true

   and your .git/config may have

	[config]
        	safe = diff.renames
                safeInclude = .frotz-gitconfig

   You will not have to worry about a malicious participant
   committing a

	[diff]
        	external = rm -fr .

   to .frotz-gitconfig and pushing it to the project because you do
   not mark diff.external as config.safe in your .git/config

Re: [PATCH/RFC 0/5] add "unset.variable" for unsetting previously set variables

[Jakub Narębski]

Junio C Hamano wrote:

>   - "[config] safe = section.variable" will list variables that can
>     be included with the config.safeInclude mechanism.  Any variable
>     that is not marked as config.safe that appears in the file
>     included by the config.safeInclude mechanism will be ignored.

Why user must know which variables are safe, why it cannot be left to 
Git to know which configuration variables can call external scripts?

-- 
Jakub Narębski

Re: [PATCH/RFC 0/5] add "unset.variable" for unsetting previously set variables

[Junio C Hamano]

Jakub Narębski <jnareb@gmail.com> writes:

> Junio C Hamano wrote:
>
>>   - "[config] safe = section.variable" will list variables that can
>>     be included with the config.safeInclude mechanism.  Any variable
>>     that is not marked as config.safe that appears in the file
>>     included by the config.safeInclude mechanism will be ignored.
>
> Why user must know which variables are safe, why it cannot be left to
> Git to know which configuration variables can call external scripts?

That's a fallback to let them take responsibility for variables we
do not mark as "safe"; and having that fallback mechanism lets us
keep the set of variables we by default mark as safe to the absolute
minimum.

Re: [PATCH/RFC 0/5] add "unset.variable" for unsetting previously set variables

[Matthieu Moy]

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

> Jakub Narębski <jnareb@gmail.com> writes:
>
>> Junio C Hamano wrote:
>>
>>>   - "[config] safe = section.variable" will list variables that can
>>>     be included with the config.safeInclude mechanism.  Any variable
>>>     that is not marked as config.safe that appears in the file
>>>     included by the config.safeInclude mechanism will be ignored.
>>
>> Why user must know which variables are safe, why it cannot be left to
>> Git to know which configuration variables can call external scripts?
>
> That's a fallback to let them take responsibility for variables we
> do not mark as "safe"; and having that fallback mechanism lets us
> keep the set of variables we by default mark as safe to the absolute
> minimum.

Perhaps this would need a way to say "this value is safe for this
variable" too. I don't have a real use-case, but one could say something
like "I'm OK with the file overriding core.editor, but the only values I
accept are nano, vim and emacs".

It doesn't seem to be a prerequisite to implement the safeInclude
feature, but we should live room in the namespace for the day we want to
add it.

I don't have really good idea for it. The first I could think of was

[config "safe"]
    core.editor = nano
    core.editor = vim
    core.editor = emacs

but it's not accepted by the current parser, hence not backward
compatible.

Emacs has such mechanism for -*- ... -*- local variables in files for
example.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH/RFC 0/5] add "unset.variable" for unsetting previously set variables

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Jakub Narębski <jnareb@gmail.com> writes:
>>
>>> Junio C Hamano wrote:
>>>
>>>>   - "[config] safe = section.variable" will list variables that can
>>>>     be included with the config.safeInclude mechanism.  Any variable
>>>>     that is not marked as config.safe that appears in the file
>>>>     included by the config.safeInclude mechanism will be ignored.
>>>
>>> Why user must know which variables are safe, why it cannot be left to
>>> Git to know which configuration variables can call external scripts?
>>
>> That's a fallback to let them take responsibility for variables we
>> do not mark as "safe"; and having that fallback mechanism lets us
>> keep the set of variables we by default mark as safe to the absolute
>> minimum.
>
> Perhaps this would need a way to say "this value is safe for this
> variable" too. I don't have a real use-case, but one could say something
> like "I'm OK with the file overriding core.editor, but the only values I
> accept are nano, vim and emacs".
>
> It doesn't seem to be a prerequisite to implement the safeInclude
> feature, but we should live room in the namespace for the day we want to
> add it.
>
> I don't have really good idea for it. The first I could think of was
>
> [config "safe"]
>     core.editor = nano
>     core.editor = vim
>     core.editor = emacs
>
> but it's not accepted by the current parser, hence not backward
> compatible.

Interesting thought (I've cc'ed Rasmus who did an RFC patchset on
the safe include feature).  I do not offhand think of a good example
of an variable that we may want to allow overriding but still want
to limit its values myself.  Almost all variables I would rather not
to see in-tree .gitconfig to touch at all, and the ones that I may
want to allow to be futzed with I can think of offhand are booleans.
With more people and time we might find a better example to illustrate
why we may want to have such a feature added.

Thanks.

Re: [PATCH/RFC 0/5] add "unset.variable" for unsetting previously set variables

[Matthieu Moy]

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

> I do not offhand think of a good example of an variable that we may
> want to allow overriding but still want to limit its values myself.

I just thought of a semi-realistic use-case : diff.*.{command,textconv}.

One may want to allow per-project sets of diff drivers, but these
variables contain actual commands, so clearly we can't allow any
value for these variables.

"semi-realistic" only because I never needed a per-project diff driver,
I have my per-user preference and I'm happy with it.

Anyway, the feature does not seem vital to me, but if someone comes up
with a clever way to keep room for it in the namespace, that would be
cool.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH/RFC 0/5] add "unset.variable" for unsetting previously set variables

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> I do not offhand think of a good example of an variable that we may
>> want to allow overriding but still want to limit its values myself.
>
> I just thought of a semi-realistic use-case : diff.*.{command,textconv}.
>
> One may want to allow per-project sets of diff drivers, but these
> variables contain actual commands, so clearly we can't allow any
> value for these variables.
>
> "semi-realistic" only because I never needed a per-project diff driver,
> I have my per-user preference and I'm happy with it.

This may open another aspect of the discussion, actually.

The whole reason why the actualy diff.*.command and textconv
commands are defined in .git/config while the filetype label is
assigned by in-tree .gitattributes is because these commands are
platform dependant.  So textconv on Linux, BSD and Windows may want
to be different commands, and the project that ships an in-tree
.gitconfig to be safe-included may want to not "set" the variable to
one specific value, but stop at offering a suggestion, i.e. "there
are these possibilities, perhaps you may want to pick one of them?"
without actually making the choice for the user.

And on the receiving side (i.e. [config "safe"] in .git/config), it
is unlikely that you would list textconv choices that are plausible
on different platforms.  Rather, you would say "I would want this
value to be set on textconv and not others".

But at that point, if you have to be that informed to set up the
[config "safe"] to list allowed values, I wonder why a user chooses
to do so and safe-include in-tree .gitconfig, instead of explicitly
setting her preferred textconv in .git/config herself, without
bothering to include anything.

> Anyway, the feature does not seem vital to me, but if someone comes up
> with a clever way to keep room for it in the namespace, that would be
> cool.

Yes.

Re: [PATCH/RFC 0/5] add "unset.variable" for unsetting previously set variables

[Jeff King]

On Wed, Oct 08, 2014 at 12:52:24PM -0700, Junio C Hamano wrote:

> The whole reason why the actualy diff.*.command and textconv
> commands are defined in .git/config while the filetype label is
> assigned by in-tree .gitattributes is because these commands are
> platform dependant.  So textconv on Linux, BSD and Windows may want
> to be different commands, and the project that ships an in-tree
> .gitconfig to be safe-included may want to not "set" the variable to
> one specific value, but stop at offering a suggestion, i.e. "there
> are these possibilities, perhaps you may want to pick one of them?"
> without actually making the choice for the user.

Or it could even auto-detect a sensible version based on the user's
filesystem. Which makes me wonder if safe-include is really helping that
much versus a project shipping a shell script that munges the repository
config. The latter is less safe (you are, after all, running code, but
you would at least have the chance to examine it), but is way more
flexible. And the safety is comparable to running "make" on a cloned
project.

I dunno. I do not have anything against the safe-include idea, but each
time it comes up, I think we are often left guessing about exactly which
config options projects would want to set, and to what values.

-Peff

Re: [PATCH/RFC 0/5] add "unset.variable" for unsetting previously set variables

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> ... Which makes me wonder if safe-include is really helping that
> much versus a project shipping a shell script that munges the repository
> config. The latter is less safe (you are, after all, running code, but
> you would at least have the chance to examine it), but is way more
> flexible. And the safety is comparable to running "make" on a cloned
> project.
>
> I dunno. I do not have anything against the safe-include idea, but each
> time it comes up, I think we are often left guessing about exactly which
> config options projects would want to set, and to what values.

I tend to agree.  Every time somebody says "a project wants to give
its participants suggested settings", we seem to tell them to ship
an instruction to their participants, either in BUILDING or setup.sh
or whatever.  It certainly is simpler and more flexible.

The only real difference it might make is an attempt to push to an
unattended place and automatically making the changes to take effect,
aka "push to deploy", which is not what we encourage anyway, so...