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

Michael J Gruber [off-list ref] 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.

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

in preparation of the config parse patch

Signed-off-by: Michael J Gruber <redacted>
---
 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

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

Signed-off-by: Michael J Gruber <redacted>
---
 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

On Wed, Dec 1, 2010 at 7:53 AM, Michael J Gruber
[off-list ref] 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

On Tue, Nov 30, 2010 at 8:43 PM, Junio C Hamano [off-list ref] wrote:

quoted

[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

Martin Langhoff venit, vidit, dixit 01.12.2010 16:02:

On Tue, Nov 30, 2010 at 8:43 PM, Junio C Hamano [off-list ref] wrote:

quoted

quoted

[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

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

On Wed, Dec 1, 2010 at 11:05 AM, Jonathan Nieder [off-list ref] 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

Jonathan Nieder [off-list ref] writes:

Martin Langhoff wrote:

quoted

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

(+cc: Jeff, config parsing wizard)

Jakub Narebski wrote:

Jonathan Nieder [off-list ref] writes:

quoted

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.

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

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

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

quoted

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

quoted

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

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

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 <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
Jeff King wrote:

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

quoted

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

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:

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

-Peff

Jeff King wrote:

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

quoted

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

Thanks again for the help.