RFC patch, 5 partsadd "unset.variable" for unsetting previously set variables

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(-)

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(-)

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(-)

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(+)

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(+)

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(+)

Tanay Abhra <tanayabh@gmail.com> writes:

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.

On 10/3/2014 1:39 AM, Junio C Hamano wrote:

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.

Tanay Abhra <tanayabh@gmail.com> writes:

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.

On 10/3/2014 1:53 AM, Junio C Hamano wrote:

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?

Tanay Abhra <tanayabh@gmail.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:

 (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.

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.

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.

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.

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 <Matthieu.Moy@grenoble-inp.fr> writes:

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.

I agree that is one major use 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.

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.

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

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

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

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 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.

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 <Matthieu.Moy@grenoble-inp.fr> writes:

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.

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.

On 10/4/2014 1:42 AM, Junio C Hamano wrote:

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.

On Mon, Oct 6, 2014 at 11:59 AM, Tanay Abhra <tanayabh@gmail.com> wrote:

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).

On 10/7/2014 12:58 AM, Junio C Hamano wrote:

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

Tanay Abhra <tanayabh@gmail.com> writes:

(just this point quick)

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?

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

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

Tanay Abhra <tanayabh@gmail.com> writes:

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.

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

Junio C Hamano wrote:

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 <jnareb@gmail.com> writes:

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.

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

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 <Matthieu.Moy@grenoble-inp.fr> writes:

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.

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

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 <Matthieu.Moy@grenoble-inp.fr> writes:

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.

Yes.

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

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

Jeff King <peff@peff.net> writes:

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...