Re: [PATCH/RFC 5/5] add tests for checking the behaviour of unset.variable
On 10/4/2014 1:42 AM, Junio C Hamano wrote: Matthieu Moy matthieu@grenoble-inp.fr writes: Junio C Hamano gits...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC 5/5] add tests for checking the behaviour of unset.variable
On Mon, Oct 6, 2014 at 11:59 AM, Tanay Abhra tanay...@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 preventgit 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). -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC 5/5] add tests for checking the behaviour of unset.variable
On 10/7/2014 12:58 AM, Junio C Hamano wrote: The point is to preventgit 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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC 5/5] add tests for checking the behaviour of unset.variable
Junio C Hamano gits...@pobox.com writes: Tanay Abhra tanay...@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/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC 5/5] add tests for checking the behaviour of unset.variable
Matthieu Moy matthieu@grenoble-inp.fr writes: Junio C Hamano gits...@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
Re: [PATCH/RFC 5/5] add tests for checking the behaviour of unset.variable
Junio C Hamano gits...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC 5/5] add tests for checking the behaviour of unset.variable
Junio C Hamano gits...@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/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC 5/5] add tests for checking the behaviour of unset.variable
Matthieu Moy matthieu@grenoble-inp.fr writes: Junio C Hamano gits...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH/RFC 5/5] add tests for checking the behaviour of unset.variable
Helped-by: Matthieu Moy matthieu@imag.fr Signed-off-by: Tanay Abhra tanay...@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] 0) != 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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC 5/5] add tests for checking the behaviour of unset.variable
Tanay Abhra tanay...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC 5/5] add tests for checking the behaviour of unset.variable
On 10/3/2014 1:39 AM, Junio C Hamano wrote: Tanay Abhra tanay...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC 5/5] add tests for checking the behaviour of unset.variable
Tanay Abhra tanay...@gmail.com writes: On 10/3/2014 1:39 AM, Junio C Hamano wrote: Tanay Abhra tanay...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC 5/5] add tests for checking the behaviour of unset.variable
On 10/3/2014 1:53 AM, Junio C Hamano wrote: Tanay Abhra tanay...@gmail.com writes: On 10/3/2014 1:39 AM, Junio C Hamano wrote: Tanay Abhra tanay...@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? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC 5/5] add tests for checking the behaviour of unset.variable
Tanay Abhra tanay...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
