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

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.