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

Matthieu Moy [off-list ref] writes:

Junio C Hamano [off-list ref] writes:
...

quoted

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.

quoted

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

quoted

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