[PATCH 1/3] cvsimport: partial whitespace cleanup

[PATCH 1/3] cvsimport: partial whitespace cleanup

[Michael J Gruber]

in preparation of the config parse patch

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 git-cvsimport.perl |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/git-cvsimport.perl b/git-cvsimport.perl
index d27abfe..f955295 100755
--- a/git-cvsimport.perl
+++ b/git-cvsimport.perl
@@ -91,8 +91,8 @@ sub write_author_info($) {
 
 # convert getopts specs for use by git config
 sub read_repo_config {
-    # Split the string between characters, unless there is a ':'
-    # So "abc:de" becomes ["a", "b", "c:", "d", "e"]
+	# Split the string between characters, unless there is a ':'
+	# So "abc:de" becomes ["a", "b", "c:", "d", "e"]
 	my @opts = split(/ *(?!:)/, shift);
 	foreach my $o (@opts) {
 		my $key = $o;
@@ -100,13 +100,13 @@ sub read_repo_config {
 		my $arg = 'git config';
 		$arg .= ' --bool' if ($o !~ /:$/);
 
-        chomp(my $tmp = `$arg --get cvsimport.$key`);
+		chomp(my $tmp = `$arg --get cvsimport.$ckey`);
 		if ($tmp && !($arg =~ /--bool/ && $tmp eq 'false')) {
-            no strict 'refs';
-            my $opt_name = "opt_" . $key;
-            if (!$$opt_name) {
-                $$opt_name = $tmp;
-            }
+			no strict 'refs';
+			my $opt_name = "opt_" . $key;
+			if (!$$opt_name) {
+				$$opt_name = $tmp;
+			}
 		}
 	}
 }
-- 
1.7.3.2.614.g03864.dirty

[PATCH 2/3] cvsimport: fix the parsing of uppercase config options

[Michael J Gruber]

The current code leads to

fatal: bad config value for 'cvsimport.r' in .git/config

for a standard use case with cvsimport.r set:

cvsimport sets internal variables by checking the config for each
possible command line option. The problem is that config items are case
insensitive, so config.r and config.R are the same. The ugly error is
due to that fact that cvsimport expects a bool for -R (and thus
config.R) but a remote name for -r (and thus config.r).

Fix this by making cvsimport expect the config item "cvsimport.RR"
for the command line option "-R" etc.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 git-cvsimport.perl |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/git-cvsimport.perl b/git-cvsimport.perl
index f955295..736a7bf 100755
--- a/git-cvsimport.perl
+++ b/git-cvsimport.perl
@@ -97,6 +97,8 @@ sub read_repo_config {
 	foreach my $o (@opts) {
 		my $key = $o;
 		$key =~ s/://g;
+		my $ckey = $key;
+		$ckey .= $ckey if ($key eq uc($key));
 		my $arg = 'git config';
 		$arg .= ' --bool' if ($o !~ /:$/);
 
-- 
1.7.3.2.614.g03864.dirty

Re: [PATCH 2/3] cvsimport: fix the parsing of uppercase config options

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> The current code leads to
>
> fatal: bad config value for 'cvsimport.r' in .git/config
>
> for a standard use case with cvsimport.r set:
>
> cvsimport sets internal variables by checking the config for each
> possible command line option. The problem is that config items are case
> insensitive, so config.r and config.R are the same. The ugly error is
> due to that fact that cvsimport expects a bool for -R (and thus
> config.R) but a remote name for -r (and thus config.r).
>
> Fix this by making cvsimport expect the config item "cvsimport.RR"
> for the command line option "-R" etc.

I do not think this is "fixing" per-se.  Isn't it more like "We didn't
have a way to use the configuration file to specify uppercase option; now
we do thanks to this patch, and here is how"?

And the "here is how" workaround, while it may be a reasonable way out, is
so obscure that it needs to be documented, no?  Ahh, that is what [3/3] is.

The $ckey change from 1/3 needs to be done here, I think.

Re: [PATCH 2/3] cvsimport: fix the parsing of uppercase config options

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 27.11.2010 07:38:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> The current code leads to
>>
>> fatal: bad config value for 'cvsimport.r' in .git/config
>>
>> for a standard use case with cvsimport.r set:
>>
>> cvsimport sets internal variables by checking the config for each
>> possible command line option. The problem is that config items are case
>> insensitive, so config.r and config.R are the same. The ugly error is
>> due to that fact that cvsimport expects a bool for -R (and thus
>> config.R) but a remote name for -r (and thus config.r).
>>
>> Fix this by making cvsimport expect the config item "cvsimport.RR"
>> for the command line option "-R" etc.
> 
> I do not think this is "fixing" per-se.  Isn't it more like "We didn't
> have a way to use the configuration file to specify uppercase option; now
> we do thanks to this patch, and here is how"?

It is a fix because the the existing code base checks for "cvsimport.r"
in order to set "opt_r" and for "cvsimport.R" in order to set "opt_R"
(etc). That is, it sets "opt_R" from a config variable which should have
nothing to do with it (cvsimport.r=cvsimport.R).

In the case when R expects a boolean you'll notice because "config get
--bool" will die, but if both "x" and "X" expect the same type then you
set defaults for "opt_x" and "opt_X" from the single "cvsimport.x".

> And the "here is how" workaround, while it may be a reasonable way out, is
> so obscure that it needs to be documented, no?  Ahh, that is what [3/3] is.

Yes, 3/3. Should I squash this with 2?

> The $ckey change from 1/3 needs to be done here, I think.

Yes, sorry, this was an "add -p" related oversight on my side. I'll resend.

Michael

[PATCHv2 1/3] cvsimport: partial whitespace cleanup

[Michael J Gruber]

in preparation of the config parse patch

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 git-cvsimport.perl |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/git-cvsimport.perl b/git-cvsimport.perl
index d27abfe..7888b77 100755
--- a/git-cvsimport.perl
+++ b/git-cvsimport.perl
@@ -91,8 +91,8 @@ sub write_author_info($) {
 
 # convert getopts specs for use by git config
 sub read_repo_config {
-    # Split the string between characters, unless there is a ':'
-    # So "abc:de" becomes ["a", "b", "c:", "d", "e"]
+	# Split the string between characters, unless there is a ':'
+	# So "abc:de" becomes ["a", "b", "c:", "d", "e"]
 	my @opts = split(/ *(?!:)/, shift);
 	foreach my $o (@opts) {
 		my $key = $o;
@@ -100,13 +100,13 @@ sub read_repo_config {
 		my $arg = 'git config';
 		$arg .= ' --bool' if ($o !~ /:$/);
 
-        chomp(my $tmp = `$arg --get cvsimport.$key`);
+		chomp(my $tmp = `$arg --get cvsimport.$key`);
 		if ($tmp && !($arg =~ /--bool/ && $tmp eq 'false')) {
-            no strict 'refs';
-            my $opt_name = "opt_" . $key;
-            if (!$$opt_name) {
-                $$opt_name = $tmp;
-            }
+			no strict 'refs';
+			my $opt_name = "opt_" . $key;
+			if (!$$opt_name) {
+				$$opt_name = $tmp;
+			}
 		}
 	}
 }
-- 
1.7.3.2.614.g03864.dirty

[PATCHv2 2/3] cvsimport: fix the parsing of uppercase config options

[Michael J Gruber]

The current code leads to

fatal: bad config value for 'cvsimport.r' in .git/config

for a standard use case with cvsimport.r set:

cvsimport sets internal variables by checking the config for each
possible command line option. The problem is that config items are case
insensitive, so config.r and config.R are the same. The ugly error is
due to that fact that cvsimport expects a bool for -R (and thus
config.R) but a remote name for -r (and thus config.r).

Fix this by making cvsimport expect the config item "cvsimport.RR"
for the command line option "-R" etc.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 git-cvsimport.perl |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/git-cvsimport.perl b/git-cvsimport.perl
index 7888b77..736a7bf 100755
--- a/git-cvsimport.perl
+++ b/git-cvsimport.perl
@@ -97,10 +97,12 @@ sub read_repo_config {
 	foreach my $o (@opts) {
 		my $key = $o;
 		$key =~ s/://g;
+		my $ckey = $key;
+		$ckey .= $ckey if ($key eq uc($key));
 		my $arg = 'git config';
 		$arg .= ' --bool' if ($o !~ /:$/);
 
-		chomp(my $tmp = `$arg --get cvsimport.$key`);
+		chomp(my $tmp = `$arg --get cvsimport.$ckey`);
 		if ($tmp && !($arg =~ /--bool/ && $tmp eq 'false')) {
 			no strict 'refs';
 			my $opt_name = "opt_" . $key;
-- 
1.7.3.2.614.g03864.dirty

[PATCHv2 3/3] cvsimport.txt: document the mapping between config and options

[Michael J Gruber]

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 Documentation/git-cvsimport.txt |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/Documentation/git-cvsimport.txt b/Documentation/git-cvsimport.txt
index 608cd63..b5d5b27 100644
--- a/Documentation/git-cvsimport.txt
+++ b/Documentation/git-cvsimport.txt
@@ -176,6 +176,13 @@ messages, bug-tracking systems, email archives, and the like.
 -h::
 	Print a short usage message and exit.
 
+CONFIG
+------
+For any option '-x' you can set the config variable 'cvsimport.x' to the value
+you would specify for '-x', or to 'true' for a boolean option. For an
+uppercase option '-X' use the config variable 'cvsimport.xx' (or
+'cvsimport.XX').
+
 OUTPUT
 ------
 If '-v' is specified, the script reports what it is doing.
-- 
1.7.3.2.614.g03864.dirty

Re: [PATCHv2 3/3] cvsimport.txt: document the mapping between config and options

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> ---
>  Documentation/git-cvsimport.txt |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/git-cvsimport.txt b/Documentation/git-cvsimport.txt
> index 608cd63..b5d5b27 100644
> --- a/Documentation/git-cvsimport.txt
> +++ b/Documentation/git-cvsimport.txt
> @@ -176,6 +176,13 @@ messages, bug-tracking systems, email archives, and the like.
>  -h::
>  	Print a short usage message and exit.
>  
> +CONFIG
> +------
> +For any option '-x' you can set the config variable 'cvsimport.x' to the value
> +you would specify for '-x', or to 'true' for a boolean option. For an
> +uppercase option '-X' use the config variable 'cvsimport.xx' (or
> +'cvsimport.XX').
> +

I still think this is not about fixing "parsing" as 2/3 states but about
"working around the initial design flaw of how configuration variables are
used in cvsimport" in that the initial design didn't take it into account
that the last component of a configuration variable is case insensitive.

While mapping -X to .xx may be a usable workaround, it looks really ugly.
Worse, if we are going to give long command line options to the command
someday, we will really regret it doing it the way your patch does.

Would it be a better alternative to give conflicting but rarely used
uppercase options longer option name synonyms, and have them specified in
the gitconfig file in their full names?  Then we can disambiguate with
something like

    [cvsimport]
    	generate-cvs-revisions = yes
        remote = origin

which would be more readable, no?

Re: [PATCHv2 3/3] cvsimport.txt: document the mapping between config and options

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 29.11.2010 21:23:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
>> ---
>>  Documentation/git-cvsimport.txt |    7 +++++++
>>  1 files changed, 7 insertions(+), 0 deletions(-)
>>
>> diff --git a/Documentation/git-cvsimport.txt b/Documentation/git-cvsimport.txt
>> index 608cd63..b5d5b27 100644
>> --- a/Documentation/git-cvsimport.txt
>> +++ b/Documentation/git-cvsimport.txt
>> @@ -176,6 +176,13 @@ messages, bug-tracking systems, email archives, and the like.
>>  -h::
>>  	Print a short usage message and exit.
>>  
>> +CONFIG
>> +------
>> +For any option '-x' you can set the config variable 'cvsimport.x' to the value
>> +you would specify for '-x', or to 'true' for a boolean option. For an
>> +uppercase option '-X' use the config variable 'cvsimport.xx' (or
>> +'cvsimport.XX').
>> +
> 
> I still think this is not about fixing "parsing" as 2/3 states but about
> "working around the initial design flaw of how configuration variables are
> used in cvsimport" in that the initial design didn't take it into account
> that the last component of a configuration variable is case insensitive.

I don't care too much about the naming. But if I specify a correct
string value for "cvsimport.r" (correct as in correct for "-r", lower
case!) and "git cvsimport" gives me

fatal: bad config value for 'cvsimport.r' in .git/config

then I call this a bug, notwithstanding the fact that cvsimport does use
the value from cvsimport.r for "-r" and continues its operation.

This occurs really without even any attempt at specfiying values for
upper case options.

> While mapping -X to .xx may be a usable workaround, it looks really ugly.
> Worse, if we are going to give long command line options to the command
> someday, we will really regret it doing it the way your patch does.
> 
> Would it be a better alternative to give conflicting but rarely used
> uppercase options longer option name synonyms, and have them specified in
> the gitconfig file in their full names?  Then we can disambiguate with
> something like
> 
>     [cvsimport]
>     	generate-cvs-revisions = yes
>         remote = origin
> 
> which would be more readable, no?
> 

Well, cvsimport does not have any long options now, and given the fact
that most cvsimport related activity lately has been on documenting its
shortcomings and promoting cvs2git, I consider that scenario highly
unlikely. (I'm not hooked on cvsimport - it's simply the only
*incremental* cvs-to-git importer that I know of, and the only one not
requiring local access.)

How about using a naming scheme like:

[cvsimport]
	r = origin
	capital-r = yes

This would be safe against any possible future long-options, quickly
implementable, and we would not have to invent long names now for the
existing one-letter options (and thus hindering any future attempts
also). Whether this is more or less ugly lies in the eye of the
s/beholder/maintainer/ :)

Michael

Re: [PATCHv2 3/3] cvsimport.txt: document the mapping between config and options

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> How about using a naming scheme like:
>
> [cvsimport]
> 	r = origin
> 	capital-r = yes

I think we can live with that.  If it is easier to implement, that's very
good.

[PATCHv3 0/3] uppercase config options for cvsimport

[Michael J Gruber]

v3 changes the naming to "cvsimport.capital-x" for the config variable
corresponding to "-X", and amends the commit message in 2/3 to make it
clear that this "fix" addresses an undocumented feature of cvsimport
(as far as included documentation goes).

Also, v3 adds a cover letter :)

Michael J Gruber (3):
  cvsimport: partial whitespace cleanup
  cvsimport: fix the parsing of uppercase config options
  cvsimport.txt: document the mapping between config and options

 Documentation/git-cvsimport.txt |    7 +++++++
 git-cvsimport.perl              |   18 ++++++++++--------
 2 files changed, 17 insertions(+), 8 deletions(-)

-- 
1.7.3.2.617.g84f63

[PATCHv3 1/3] cvsimport: partial whitespace cleanup

[Michael J Gruber]

in preparation of the config parse patch

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 git-cvsimport.perl |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/git-cvsimport.perl b/git-cvsimport.perl
index d27abfe..7888b77 100755
--- a/git-cvsimport.perl
+++ b/git-cvsimport.perl
@@ -91,8 +91,8 @@ sub write_author_info($) {
 
 # convert getopts specs for use by git config
 sub read_repo_config {
-    # Split the string between characters, unless there is a ':'
-    # So "abc:de" becomes ["a", "b", "c:", "d", "e"]
+	# Split the string between characters, unless there is a ':'
+	# So "abc:de" becomes ["a", "b", "c:", "d", "e"]
 	my @opts = split(/ *(?!:)/, shift);
 	foreach my $o (@opts) {
 		my $key = $o;
@@ -100,13 +100,13 @@ sub read_repo_config {
 		my $arg = 'git config';
 		$arg .= ' --bool' if ($o !~ /:$/);
 
-        chomp(my $tmp = `$arg --get cvsimport.$key`);
+		chomp(my $tmp = `$arg --get cvsimport.$key`);
 		if ($tmp && !($arg =~ /--bool/ && $tmp eq 'false')) {
-            no strict 'refs';
-            my $opt_name = "opt_" . $key;
-            if (!$$opt_name) {
-                $$opt_name = $tmp;
-            }
+			no strict 'refs';
+			my $opt_name = "opt_" . $key;
+			if (!$$opt_name) {
+				$$opt_name = $tmp;
+			}
 		}
 	}
 }
-- 
1.7.3.2.617.g84f63

[PATCHv3 2/3] cvsimport: fix the parsing of uppercase config options

[Michael J Gruber]

The current code leads to

fatal: bad config value for 'cvsimport.r' in .git/config

for a standard use case with cvsimport.r set:

cvsimport sets internal variables by checking the config for each
possible command line option. The problem is that config items are case
insensitive, so config.r and config.R are the same. The ugly error is
due to that fact that cvsimport expects a bool for -R (and thus
config.R) but a remote name for -r (and thus config.r).

Fix this by making cvsimport expect the config item "cvsimport.capital-r"
for the command line option "-R" etc.

(config options for cvsimport have been undocumented so far, though
present in the code and advertised in several tutorials. So one may read
"enhance" for "fix".)

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 git-cvsimport.perl |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/git-cvsimport.perl b/git-cvsimport.perl
index 7888b77..0bb5e32 100755
--- a/git-cvsimport.perl
+++ b/git-cvsimport.perl
@@ -97,10 +97,12 @@ sub read_repo_config {
 	foreach my $o (@opts) {
 		my $key = $o;
 		$key =~ s/://g;
+		my $ckey = $key;
+		$ckey = 'capital-' . $ckey if ($key eq uc($key));
 		my $arg = 'git config';
 		$arg .= ' --bool' if ($o !~ /:$/);
 
-		chomp(my $tmp = `$arg --get cvsimport.$key`);
+		chomp(my $tmp = `$arg --get cvsimport.$ckey`);
 		if ($tmp && !($arg =~ /--bool/ && $tmp eq 'false')) {
 			no strict 'refs';
 			my $opt_name = "opt_" . $key;
-- 
1.7.3.2.617.g84f63

Re: [PATCHv3 2/3] cvsimport: fix the parsing of uppercase config options

[Martin Langhoff]

On Wed, Dec 1, 2010 at 7:53 AM, Michael J Gruber
<git@drmicha.warpmail.net> wrote:
> Fix this by making cvsimport expect the config item "cvsimport.capital-r"
> for the command line option "-R" etc.

Good point to fix this, thanks. But 'capital-r' will muddy the waters
further. When accepting configuration from git-config, make all these
values the full expanded name.

So cvsimport.remote (for -r) and cvsimport.revisions (or
trackrevisions perhaps) seem more appropriate.

cheers,


m
-- 
 martin.langhoff@gmail.com
 martin@laptop.org -- School Server Architect
 - ask interesting questions
 - don't get distracted with shiny stuff  - working code first
 - http://wiki.laptop.org/go/User:Martinlanghoff

Re: [PATCHv3 2/3] cvsimport: fix the parsing of uppercase config options

[Jonathan Nieder]

Martin Langhoff wrote:

> So cvsimport.remote (for -r) and cvsimport.revisions (or
> trackrevisions perhaps) seem more appropriate.

I somewhat like this idea.  So let's build a full table, shall
we?  The embedded dashes are meant for the command-line options
rather than the config file.

	-v	verbosity
	-d	cvsroot
	-C	[doesn't make sense in a config file; you've already
		 found where to read the configuration from, right?]
	-r	remote
	-o	mainline
	-i	import-only
	-k	kill-keywords
	-u	replace-underscores
	-s	replace-slashes
	-p	cvsps-options
	-z	fuzz
	-P	[doesn't make much sense in a config file; for one-shot use]
	-m	detect-merges
	-M	merge-regex
	-S	ignore-paths
	-a	import-all
	-L	max-commits
	-A	authors-file
	-R	track-revisions
	-h	[doesn't make sense in a config file]

Hmm?
Jonathan

Re: [PATCHv3 2/3] cvsimport: fix the parsing of uppercase config options

[Martin Langhoff]

On Wed, Dec 1, 2010 at 11:05 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> I somewhat like this idea.  So let's build a full table, shall
> we?  The embedded dashes are meant for the command-line options
> rather than the config file.

The suggested expansions sound good to me, with one comment:

>        -o      mainline

I am not sure about this one. The 'o' originally stood for 'origin'
branch (back when git didn't use 'origin/master', or maybe I was stuck
with Cogito's mental model).

It is the branchname for CVS HEAD; it defaults to 'origin' -- given
today's conventions it should be 'master'.

I would call it cvshead.




m
-- 
 martin.langhoff@gmail.com
 martin@laptop.org -- School Server Architect
 - ask interesting questions
 - don't get distracted with shiny stuff  - working code first
 - http://wiki.laptop.org/go/User:Martinlanghoff

Re: [PATCHv3 2/3] cvsimport: fix the parsing of uppercase config options

[Jakub Narebski]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Martin Langhoff wrote:
> 
> > So cvsimport.remote (for -r) and cvsimport.revisions (or
> > trackrevisions perhaps) seem more appropriate.
> 
> I somewhat like this idea.  So let's build a full table, shall
> we?  The embedded dashes are meant for the command-line options
> rather than the config file.
> 
> 	-v	verbosity
> 	-d	cvsroot
> 	-C	[doesn't make sense in a config file; you've already
> 		 found where to read the configuration from, right?]
> 	-r	remote
> 	-o	mainline
> 	-i	import-only
> 	-k	kill-keywords
> 	-u	replace-underscores
> 	-s	replace-slashes
> 	-p	cvsps-options
> 	-z	fuzz
> 	-P	[doesn't make much sense in a config file; for one-shot use]
> 	-m	detect-merges
> 	-M	merge-regex
> 	-S	ignore-paths
> 	-a	import-all
> 	-L	max-commits
> 	-A	authors-file
> 	-R	track-revisions
> 	-h	[doesn't make sense in a config file]
> 
> Hmm?

Good idea, though I'd rather we avoid new convention for multi word
names separated with '-' (multi-word), but rather use camel case
(multiWord).

Currently we have only unfortunate exception of `add.ignore-errors',
all others use either same case (`core.ignorecase') or camelCase
(`core.ignoreStat`).

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCHv3 2/3] cvsimport: fix the parsing of uppercase config options

[Jonathan Nieder]

(+cc: Jeff, config parsing wizard)

Jakub Narebski wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> I somewhat like this idea.  So let's build a full table, shall
>> we?  The embedded dashes are meant for the command-line options
>> rather than the config file.
[...]
> Good idea, though I'd rather we avoid new convention for multi word
> names separated with '-' (multi-word), but rather use camel case
> (multiWord).

No disagreement there. :)

> Currently we have only unfortunate exception of `add.ignore-errors',

Maybe we can teach the config file parser to ignore dashes in addition
to case (except in the names of [genus "species"] headings)?  That
would be an incompatible change for third-party tools, though, so
maybe "git config" would have to take an explicit --strip-dashes
flag to do it.

Re: [PATCHv3 2/3] cvsimport: fix the parsing of uppercase config options

[Michael J Gruber]

So you guys are going to break current behaviour (for "cvsimport.r" etc.)?

I hate it when simple things get held up like this. Time to go home for
today... Food deprivation makes grumpy.

Michael

Re: [PATCHv3 2/3] cvsimport: fix the parsing of uppercase config options

[Jonathan Nieder]

Michael J Gruber wrote:

> So you guys are going to break current behaviour (for "cvsimport.r" etc.)?

Actual git cvsimport users get the real vote.

But yes, I would like to break current behavior for cvsimport.r, since
the current behavior is insane.  On the other hand, I think it is fine
to preserve the current behavior for cvsimport.d.

Meanwhile we would get better documentation and self-describing
command lines:

	git cvsimport --cvshead=master --authors-file=$(pwd)/.git/cvs-authors \
		... etc ...

> I hate it when simple things get held up like this.

Perhaps it is a case of everyone knowing what an ugly shed is and thus
spending the time to make it a little better.

Anyway, feel free to ignore me in this case if you want.
Jonathan

Re: [PATCHv3 2/3] cvsimport: fix the parsing of uppercase config options

[Jeff King]

On Wed, Dec 01, 2010 at 05:52:06PM +0100, Michael J Gruber wrote:

> So you guys are going to break current behaviour (for "cvsimport.r" etc.)?

I have not been following this thread, but as I understand it,
"cvsimport.r" is somewhat broken already, isn't it? I thought the
original problem was that it got parsed as both "-r" and "-R", and one
of them tried to enforce some type semantics on the value.

If there are short config options that work, though, we should probably
keep them. It surely can't be that hard for the perl code to accept both
"cvsimport.r" and "cvsimport.remote" for "-r" and
"cvsimport.generaterevisions" (or whatever) for "-R"?

-Peff

Re: [PATCHv3 2/3] cvsimport: fix the parsing of uppercase config options

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> If there are short config options that work, though, we should probably
> keep them. It surely can't be that hard for the perl code to accept both
> "cvsimport.r" and "cvsimport.remote" for "-r" and
> "cvsimport.generaterevisions" (or whatever) for "-R"?

I'd agree.  The current parameter set we give to GetOptions is:

    haivmkuo:d:p:r:C:z:s:M:P:A:S:L:R

and I notice that we cannot sanely access A, M, P, R, S and their
lowercase counterparts.

It shouldn't be too hard to interpret the single letter options for the
currently supported set but only for lowercase letters plus '-C <arg>' and
'-L <arg>' (because there are no lowercase ones that crash with them), and
give long config names to at least the ones inaccessible with the above.

Wouldn't that give us a regression-free solution to the immediate problem
at hand?

Supporting long options on the command line, and giving long config name
synonyms to the lowercase ones, would be a plus for consistency's sake,
but that probably is a separate topic.

Re: [PATCHv3 2/3] cvsimport: fix the parsing of uppercase config options

[Junio C Hamano]

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

> Jeff King <peff@peff.net> writes:
>
>> If there are short config options that work, though, we should probably
>> keep them. It surely can't be that hard for the perl code to accept both
>> "cvsimport.r" and "cvsimport.remote" for "-r" and
>> "cvsimport.generaterevisions" (or whatever) for "-R"?
>
> I'd agree.  The current parameter set we give to GetOptions is:
>
>     haivmkuo:d:p:r:C:z:s:M:P:A:S:L:R
>
> and I notice that we cannot sanely access A, M, P, R, S and their
> lowercase counterparts.
>
> It shouldn't be too hard to interpret the single letter options for the
> currently supported set but only for lowercase letters plus '-C <arg>' and
> '-L <arg>' (because there are no lowercase ones that crash with them), and
> give long config names to at least the ones inaccessible with the above.
>
> Wouldn't that give us a regression-free solution to the immediate problem
> at hand?
>
> Supporting long options on the command line, and giving long config name
> synonyms to the lowercase ones, would be a plus for consistency's sake,
> but that probably is a separate topic.

So here is an attempt to do that first step, taking the list of long names
from Jonathan's message earlier in the thread.  If we start giving set of
long options, long option names may come from a different table which
would likely to have names in the dashed form, so this code already
expects names with dashes, e.g. "track-revisions", and contracts them to
names for configuration files, e.g. "trackRevisions".

 git-cvsimport.perl   |   19 ++++++++++++++++++-
 t/t9600-cvsimport.sh |    7 +++++--
 2 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/git-cvsimport.perl b/git-cvsimport.perl
index 2b07c72..8c916d9 100755
--- a/git-cvsimport.perl
+++ b/git-cvsimport.perl
@@ -89,6 +89,14 @@ sub write_author_info($) {
 }
 
 # convert getopts specs for use by git config
+my %longmap = (
+	'A:' => 'authors-file',
+	'M:' => 'merge-regex',
+	'P:' => undef,
+	'R' => 'track-revisions',
+	'S:' => 'ignore-paths',
+);
+
 sub read_repo_config {
 	# Split the string between characters, unless there is a ':'
 	# So "abc:de" becomes ["a", "b", "c:", "d", "e"]
@@ -98,8 +106,17 @@ sub read_repo_config {
 		$key =~ s/://g;
 		my $arg = 'git config';
 		$arg .= ' --bool' if ($o !~ /:$/);
-
-		chomp(my $tmp = `$arg --get cvsimport.$key`);
+		my $ckey = $key;
+
+		if (exists $longmap{$o}) {
+			# An uppercase option like -R cannot be
+			# expressed in the configuration, as the
+			# variable names are downcased.
+			$ckey = $longmap{$o};
+			next if (! defined $ckey);
+			$ckey =~ s/-//g;
+		}
+		chomp(my $tmp = `$arg --get cvsimport.$ckey`);
 		if ($tmp && !($arg =~ /--bool/ && $tmp eq 'false')) {
 			no strict 'refs';
 			my $opt_name = "opt_" . $key;
diff --git a/t/t9600-cvsimport.sh b/t/t9600-cvsimport.sh
index b572ce3..6ede84c 100755
--- a/t/t9600-cvsimport.sh
+++ b/t/t9600-cvsimport.sh
@@ -93,7 +93,8 @@ EOF
 test_expect_success 'update git module' '
 
 	cd module-git &&
-	git cvsimport -a -R -z 0 module &&
+	git config cvsimport.trackRevisions true &&
+	git cvsimport -a -z 0 module &&
 	git merge origin &&
 	cd .. &&
 	test_cmp module-cvs/o_fortuna module-git/o_fortuna
@@ -122,7 +123,8 @@ test_expect_success 'cvsimport.module config works' '
 
 	cd module-git &&
 		git config cvsimport.module module &&
-		git cvsimport -a -R -z0 &&
+		git config cvsimport.trackRevisions true &&
+		git cvsimport -a -z0 &&
 		git merge origin &&
 	cd .. &&
 	test_cmp module-cvs/tick module-git/tick
@@ -142,6 +144,7 @@ test_expect_success 'import from a CVS working tree' '
 
 	$CVS co -d import-from-wt module &&
 	cd import-from-wt &&
+		git config cvsimport.trackRevisions false &&
 		git cvsimport -a -z0 &&
 		echo 1 >expect &&
 		git log -1 --pretty=format:%s%n >actual &&

Re: [PATCHv3 2/3] cvsimport: fix the parsing of uppercase config options

[Jeff King]

On Wed, Dec 01, 2010 at 10:34:06AM -0600, Jonathan Nieder wrote:

> (+cc: Jeff, config parsing wizard)

Ugh, that is not a title that I aspire to. :)

> > Good idea, though I'd rather we avoid new convention for multi word
> > names separated with '-' (multi-word), but rather use camel case
> > (multiWord).
> 
> No disagreement there. :)
> 
> > Currently we have only unfortunate exception of `add.ignore-errors',

At some point there was talk of making this add.ignoreErrors in the
docs, and just keeping the add.ignore-errors alias forever for backwards
compatibility.

> Maybe we can teach the config file parser to ignore dashes in addition
> to case (except in the names of [genus "species"] headings)?  That
> would be an incompatible change for third-party tools, though, so
> maybe "git config" would have to take an explicit --strip-dashes
> flag to do it.

But if you require --strip-dashes, then you get potentially differing
behavior for the same set of options (i.e., one tool may accept "foobar"
but the other requires "foo-bar", because the latter has not been
updated to --strip-dashes).

Naively, such a patch would look like:

diff --git a/builtin/add.c b/builtin/add.c
index 22c6329..944c54f 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -331,7 +331,7 @@ static struct option builtin_add_options[] = {
 
 static int add_config(const char *var, const char *value, void *cb)
 {
-	if (!strcasecmp(var, "add.ignore-errors")) {
+	if (!strcasecmp(var, "add.ignoreerrors")) {
 		ignore_add_errors = git_config_bool(var, value);
 		return 0;
 	}
diff --git a/config.c b/config.c
index d8ce653..e5cb5f9 100644
--- a/config.c
+++ b/config.c
@@ -211,6 +211,8 @@ static int get_value(config_fn_t fn, void *data, char *name, unsigned int len)
 			break;
 		if (!iskeychar(c))
 			break;
+		if (c == '-')
+			continue;
 		name[len++] = tolower(c);
 		if (len >= MAXNAME)
 			return -1;

but there are a lot of corner cases. What happens with --get-regexp?
What happens with headings like '[foo-bar "baz"]'. When we use
"git-config" to edit, do we properly preserve dashes?

I started to think about all of these things, and then realized it
probably just isn't worth it. If we care about no-dashes, then let's
enforce no-dashes in new options. If we care about the existing
add.ignore-errors, then let's fix it but keep the old version there for
backwards compatibility. Those are easy to do, and then the problem just
goes away.

-Peff

[PATCH] add: introduce add.ignoreerrors synonym for add.ignore-errors

[Jonathan Nieder]

The "[add] ignore-errors" tweakable introduced by v1.5.6-rc0~30^2 (Add
a config option to ignore errors for git-add, 2008-05-12) does not
follow the usual convention for naming values in the git configuration
file.

What convention?  Glad you asked.

	The section name indicates the affected subsystem.

	The subsection name, if any, indicates which of
	an unbound set of things to set the value for.

	The variable name describes the effect of tweaking
	this knob.

	The section and variable names can be broken into
	words using bumpyCaps in documentation as a hint to
	the reader.  These word breaks are not significant
	at the level of code, since the section and variable
	names are not case sensitive.

The name "add.ignore-errors" includes a dash, meaning a naive
configuration file like

	[add]
		ignoreerrors

does not have any effect.  Avoid such confusion by renaming to the
more consistent add.ignoreErrors, but keep the old version for
backwards compatibility.

Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Jeff King wrote:
> On Wed, Dec 01, 2010 at 10:34:06AM -0600, Jonathan Nieder wrote:

>> (+cc: Jeff, config parsing wizard)
>
> Ugh, that is not a title that I aspire to. :)

Yes, one day the code will be clean enough that wizardry is not
needed. :)

[...]
> But if you require --strip-dashes, then you get potentially differing
> behavior for the same set of options (i.e., one tool may accept "foobar"
> but the other requires "foo-bar", because the latter has not been
> updated to --strip-dashes).

It was a bad idea, so let's do the simple thing.  Like this?

 Documentation/config.txt |    1 +
 builtin/add.c            |    3 ++-
 2 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 6a6c0b5..c609de4 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -553,6 +553,7 @@ core.sparseCheckout::
 	Enable "sparse checkout" feature. See section "Sparse checkout" in
 	linkgit:git-read-tree[1] for more information.
 
+add.ignoreErrors::
 add.ignore-errors::
 	Tells 'git add' to continue adding files when some files cannot be
 	added due to indexing errors. Equivalent to the '--ignore-errors'
diff --git a/builtin/add.c b/builtin/add.c
index 71f9b04..21dc1f7 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -331,7 +331,8 @@ static struct option builtin_add_options[] = {
 
 static int add_config(const char *var, const char *value, void *cb)
 {
-	if (!strcasecmp(var, "add.ignore-errors")) {
+	if (!strcasecmp(var, "add.ignoreerrors") ||
+	    !strcasecmp(var, "add.ignore-errors")) {
 		ignore_add_errors = git_config_bool(var, value);
 		return 0;
 	}
-- 
1.7.2.3

Re: [PATCH] add: introduce add.ignoreerrors synonym for add.ignore-errors

[Jeff King]

On Wed, Dec 01, 2010 at 12:36:15PM -0600, Jonathan Nieder wrote:

> It was a bad idea, so let's do the simple thing.  Like this?
> 
>  Documentation/config.txt |    1 +
>  builtin/add.c            |    3 ++-

Yes, looks good to me. You could potentially drop the old one from the
config:

> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -553,6 +553,7 @@ core.sparseCheckout::
>  	Enable "sparse checkout" feature. See section "Sparse checkout" in
>  	linkgit:git-read-tree[1] for more information.
>  
> +add.ignoreErrors::
>  add.ignore-errors::
>  	Tells 'git add' to continue adding files when some files cannot be
>  	added due to indexing errors. Equivalent to the '--ignore-errors'

which may be less confusing to new users (who might ask "is there a
difference between the two?").

But I don't have a strong feeling on it, so either way:

Acked-by: Jeff King <peff@peff.net>

-Peff

Re: [PATCH] add: introduce add.ignoreerrors synonym for add.ignore-errors

[Jonathan Nieder]

Jeff King wrote:

> Yes, looks good to me. You could potentially drop the old one from the
> config:
>
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -553,6 +553,7 @@ core.sparseCheckout::
>>  	Enable "sparse checkout" feature. See section "Sparse checkout" in
>>  	linkgit:git-read-tree[1] for more information.
>>  
>> +add.ignoreErrors::
>>  add.ignore-errors::
>>  	Tells 'git add' to continue adding files when some files cannot be
>>  	added due to indexing errors. Equivalent to the '--ignore-errors'
>
> which may be less confusing to new users (who might ask "is there a
> difference between the two?").

Right, I prefer to keep it documented for the old and forgetful users
(who might ask "why does this configuration work?").

> But I don't have a strong feeling on it, so either way:
>
> Acked-by: Jeff King <peff@peff.net>

Thanks again for the help.

Re: [PATCH] add: introduce add.ignoreerrors synonym for add.ignore-errors

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Jeff King wrote:
>
>> Yes, looks good to me. You could potentially drop the old one from the
>> config:
>>
>>> --- a/Documentation/config.txt
>>> +++ b/Documentation/config.txt
>>> @@ -553,6 +553,7 @@ core.sparseCheckout::
>>>  	Enable "sparse checkout" feature. See section "Sparse checkout" in
>>>  	linkgit:git-read-tree[1] for more information.
>>>  
>>> +add.ignoreErrors::
>>>  add.ignore-errors::
>>>  	Tells 'git add' to continue adding files when some files cannot be
>>>  	added due to indexing errors. Equivalent to the '--ignore-errors'
>>
>> which may be less confusing to new users (who might ask "is there a
>> difference between the two?").
>
> Right, I prefer to keep it documented for the old and forgetful users
> (who might ask "why does this configuration work?").
>
>> But I don't have a strong feeling on it, so either way:
>>
>> Acked-by: Jeff King <peff@peff.net>
>
> Thanks again for the help.

Thanks, will queue.

Re: [PATCH] add: introduce add.ignoreerrors synonym for add.ignore-errors

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Jeff King wrote:
>
>> Yes, looks good to me. You could potentially drop the old one from the
>> config:
>> ...
>> which may be less confusing to new users (who might ask "is there a
>> difference between the two?").
>
> Right, I prefer to keep it documented for the old and forgetful users
> (who might ask "why does this configuration work?").

We actually need to stress the fact that _only_ newer git will understand
"add.ignoreErrors", so people with older git (especially the ones who need
to use both older and newer versions of git) may need to keep using the
older form in their configuration, no?

Perhaps phrasing it like this, and then issue v1.7.{0,1,2,3}.X maintenance
release to reduce the chance of inconveniencing the users?

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1df0f65..5b6f667 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -539,9 +539,12 @@ core.sparseCheckout::
 	linkgit:git-read-tree[1] for more information.
 
 add.ignore-errors::
+add.ignoreErrors::
 	Tells 'git add' to continue adding files when some files cannot be
 	added due to indexing errors. Equivalent to the '--ignore-errors'
-	option of linkgit:git-add[1].
+	option of linkgit:git-add[1].  Older versions of git accept only
+	`add.ignore-errors`, whose name goes against the convention, so
+	newer versions of git honor `add.ignoreErrors` as well.
 
 alias.*::
 	Command aliases for the linkgit:git[1] command wrapper - e.g.

Re: [PATCH] add: introduce add.ignoreerrors synonym for add.ignore-errors

[Jeff King]

On Wed, Dec 01, 2010 at 12:09:12PM -0800, Junio C Hamano wrote:

> We actually need to stress the fact that _only_ newer git will understand
> "add.ignoreErrors", so people with older git (especially the ones who need
> to use both older and newer versions of git) may need to keep using the
> older form in their configuration, no?
> 
> Perhaps phrasing it like this, and then issue v1.7.{0,1,2,3}.X maintenance
> release to reduce the chance of inconveniencing the users?

Yeah, I think it is better to be explicit.

>  add.ignore-errors::
> +add.ignoreErrors::
>  	Tells 'git add' to continue adding files when some files cannot be
>  	added due to indexing errors. Equivalent to the '--ignore-errors'
> -	option of linkgit:git-add[1].
> +	option of linkgit:git-add[1].  Older versions of git accept only
> +	`add.ignore-errors`, whose name goes against the convention, so
> +	newer versions of git honor `add.ignoreErrors` as well.

Nit: Somehow "goes against the convention" doesn't sound quite right to
me. Astute users can probably figure out which convention, but perhaps
we should be more explicit like:

  Older versions of git accept only `add.ignore-errors`, which does not
  follow the usual naming convention for configuration variable. Newer
  versions of git honor `add.ignoreErrors` as well.

-Peff

Re: [PATCH] add: introduce add.ignoreerrors synonym for add.ignore-errors

[Junio C Hamano]

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

> Perhaps phrasing it like this, and then issue v1.7.{0,1,2,3}.X maintenance
> release to reduce the chance of inconveniencing the users?

I ended up cutting the maintenance releases for 1.7.[012].X series, which
costed a few more hours than I planned to spend on this X-<.

[PATCHv3 3/3] cvsimport.txt: document the mapping between config and options

[Michael J Gruber]

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 Documentation/git-cvsimport.txt |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/Documentation/git-cvsimport.txt b/Documentation/git-cvsimport.txt
index 608cd63..fc77a3b 100644
--- a/Documentation/git-cvsimport.txt
+++ b/Documentation/git-cvsimport.txt
@@ -176,6 +176,13 @@ messages, bug-tracking systems, email archives, and the like.
 -h::
 	Print a short usage message and exit.
 
+CONFIG
+------
+For any option '-x' you can set the config variable 'cvsimport.x' to the value
+you would specify for '-x', or to 'true' for a boolean option. For an
+uppercase option '-X' use the config variable 'cvsimport.capital-x' (or
+'cvsimport.capital-X').
+
 OUTPUT
 ------
 If '-v' is specified, the script reports what it is doing.
-- 
1.7.3.2.617.g84f63

Re: [PATCHv2 3/3] cvsimport.txt: document the mapping between config and options

[Martin Langhoff]

On Tue, Nov 30, 2010 at 8:43 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> [cvsimport]
>>       r = origin
>>       capital-r = yes
>
> I think we can live with that.  If it is easier to implement, that's very
> good.

Sorry, had not seen this discussion. Though I will obviously defer to
you guys, I don't like it -- not short term, not long term.

Short-name opts should not auto-read from git-config -- it's a misfire.



m
-- 
 martin.langhoff@gmail.com
 martin@laptop.org -- School Server Architect
 - ask interesting questions
 - don't get distracted with shiny stuff  - working code first
 - http://wiki.laptop.org/go/User:Martinlanghoff

Re: [PATCHv2 3/3] cvsimport.txt: document the mapping between config and options

[Michael J Gruber]

Martin Langhoff venit, vidit, dixit 01.12.2010 16:02:
> On Tue, Nov 30, 2010 at 8:43 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> [cvsimport]
>>>       r = origin
>>>       capital-r = yes
>>
>> I think we can live with that.  If it is easier to implement, that's very
>> good.
> 
> Sorry, had not seen this discussion. Though I will obviously defer to
> you guys, I don't like it -- not short term, not long term.
> 
> Short-name opts should not auto-read from git-config -- it's a misfire.

Well, you're free to change that. There *are no* long options for
cvsimport right now. And it does accept short config variables (though
undocumented) right now. That pretty much describes the two obstacles
you'd be running into.

Michael

[PATCH 3/3] cvsimport.txt: document the mapping between config and options

[Michael J Gruber]

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 Documentation/git-cvsimport.txt |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/Documentation/git-cvsimport.txt b/Documentation/git-cvsimport.txt
index 608cd63..b5d5b27 100644
--- a/Documentation/git-cvsimport.txt
+++ b/Documentation/git-cvsimport.txt
@@ -176,6 +176,13 @@ messages, bug-tracking systems, email archives, and the like.
 -h::
 	Print a short usage message and exit.
 
+CONFIG
+------
+For any option '-x' you can set the config variable 'cvsimport.x' to the value
+you would specify for '-x', or to 'true' for a boolean option. For an
+uppercase option '-X' use the config variable 'cvsimport.xx' (or
+'cvsimport.XX').
+
 OUTPUT
 ------
 If '-v' is specified, the script reports what it is doing.
-- 
1.7.3.2.614.g03864.dirty

Re: [PATCH 1/3] cvsimport: partial whitespace cleanup

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> @@ -100,13 +100,13 @@ sub read_repo_config {
>  		my $arg = 'git config';
>  		$arg .= ' --bool' if ($o !~ /:$/);
>  
> -        chomp(my $tmp = `$arg --get cvsimport.$key`);
> +		chomp(my $tmp = `$arg --get cvsimport.$ckey`);

At this point in the series you haven't introduced $ckey yet, hence this
will break, no?