git cvsimport and case-insensitive config

git cvsimport and case-insensitive config

[Giuseppe Bilotta]

Hello all,

I just noticed that there's an annoying buglet in the way git cvsimport 
stores its configuration and the case-insensitive key lookup done by git.

git cvsimport looks for cvsimport.* single-letter keys that match the 
command-line option (e.g. cvsimport.r for -r). However, since there are some 
single-letter options which only differ by case (a vs A, r vs R) if you set 
either you get annoying messages (and potentially also odd results, although 
I haven't come across these yet).

I come with no patch because I'm not really sure what would be the best 
approach at fixing this. Changing the way cvsimport matches config to 
command-line options? An option to make git-config case-sensitive re. key 
names? Something else?

Re: git cvsimport and case-insensitive config

[Junio C Hamano]

Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:

> git cvsimport looks for cvsimport.* single-letter keys that match the 
> command-line option (e.g. cvsimport.r for -r). However, since there are some 
> single-letter options which only differ by case (a vs A, r vs R) if you set 
> either you get annoying messages (and potentially also odd results, although 
> I haven't come across these yet).

Ouch.

The only sensible solution in the longer term is to eventually rename them
to spell them out e.g. cvsimport.remote vs cvsimport.userevisionmap.

The transition cost would be the same for either approach.

 (1) Introduce long options for cvsimport; the code already uses
     Getopt::Long, so this shouldn't be too bad.

 (2) Add case-sensitive variant of "git config -l" that shows the config
     variable names in the original case.

 (3) Upon startup, use "git config -l -f $GIT_DIR/config" to check for
     historical short name (e.g. "cvsimport.a" or "cvsimport.A").  If
     there are, map them to longer name, remove the short keys and write
     the conversion back to the configuration file.  You might want to do
     the same for "$HOME/.gitconfig" as well.

 (4) Then the rest of the code can stay the same.

Step (2) may look like this.  Note that I made this "list with case"
hidden and inaccessible on purpose, as this is primarily to write a tool
to recover from mistakes like this.

 builtin/config.c |   10 +++++++++-
 cache.h          |    1 +
 config.c         |   12 +++++++++---
 3 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 4bc46b1..3684c3a 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -41,6 +41,7 @@ static int end_null;
 #define ACTION_SET_ALL (1<<12)
 #define ACTION_GET_COLOR (1<<13)
 #define ACTION_GET_COLORBOOL (1<<14)
+#define ACTION_LIST_KEYS_WITH_CASE (1<<15)
 
 #define TYPE_BOOL (1<<0)
 #define TYPE_INT (1<<1)
@@ -73,6 +74,11 @@ static struct option builtin_config_options[] = {
 	OPT_BIT(0, "path", &types, "value is a path (file or directory name)", TYPE_PATH),
 	OPT_GROUP("Other"),
 	OPT_BOOLEAN('z', "null", &end_null, "terminate values with NUL byte"),
+	{
+		OPTION_BIT, 0, "list-keys-with-case", &actions,
+		NULL, "list for conversion", PARSE_OPT_NOARG|PARSE_OPT_HIDDEN,
+		NULL, ACTION_LIST_KEYS_WITH_CASE,
+	},
 	OPT_END(),
 };
 
@@ -397,7 +403,9 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix)
 			usage_with_options(builtin_config_usage, builtin_config_options);
 		}
 
-	if (actions == ACTION_LIST) {
+	if (actions == ACTION_LIST || actions == ACTION_LIST_KEYS_WITH_CASE) {
+		if (actions == ACTION_LIST_KEYS_WITH_CASE)
+			config_return_keys_with_case = 1;
 		check_argc(argc, 0, 0);
 		if (git_config(show_all_config, NULL) < 0) {
 			if (config_exclusive_filename)
diff --git a/cache.h b/cache.h
index 6dcb100..4320288 100644
--- a/cache.h
+++ b/cache.h
@@ -953,6 +953,7 @@ extern int git_config_system(void);
 extern int git_config_global(void);
 extern int config_error_nonbool(const char *);
 extern const char *config_exclusive_filename;
+extern int config_return_keys_with_case;
 
 #define MAX_GITNAME (1000)
 extern char git_default_email[MAX_GITNAME];
diff --git a/config.c b/config.c
index 6963fbe..c548cec 100644
--- a/config.c
+++ b/config.c
@@ -17,6 +17,11 @@ static int config_file_eof;
 static int zlib_compression_seen;
 
 const char *config_exclusive_filename = NULL;
+/*
+ * only used for config --list-case-sensitive-keys for recovering
+ * from mistakes.
+ */
+int config_return_keys_with_case;
 
 static int get_next_char(void)
 {
@@ -123,7 +128,7 @@ static int get_value(config_fn_t fn, void *data, char *name, unsigned int len)
 			break;
 		if (!iskeychar(c))
 			break;
-		name[len++] = tolower(c);
+		name[len++] = config_return_keys_with_case ? c : tolower(c);
 		if (len >= MAXNAME)
 			return -1;
 	}
@@ -193,7 +198,8 @@ static int get_base_var(char *name)
 			return -1;
 		if (baselen > MAXNAME / 2)
 			return -1;
-		name[baselen++] = tolower(c);
+		name[baselen++] = config_return_keys_with_case
+			? c : tolower(c);
 	}
 }
 
@@ -246,7 +252,7 @@ static int git_parse_file(config_fn_t fn, void *data)
 		}
 		if (!isalpha(c))
 			break;
-		var[baselen] = tolower(c);
+		var[baselen] = config_return_keys_with_case ? c : tolower(c);
 		if (get_value(fn, data, var, baselen+1) < 0)
 			break;
 	}

Re: git cvsimport and case-insensitive config

[Giuseppe Bilotta]

On Tue, Mar 30, 2010 at 7:29 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>
>> git cvsimport looks for cvsimport.* single-letter keys that match the
>> command-line option (e.g. cvsimport.r for -r). However, since there are some
>> single-letter options which only differ by case (a vs A, r vs R) if you set
>> either you get annoying messages (and potentially also odd results, although
>> I haven't come across these yet).
>
> Ouch.
>
> The only sensible solution in the longer term is to eventually rename them
> to spell them out e.g. cvsimport.remote vs cvsimport.userevisionmap.
>
> The transition cost would be the same for either approach.
>
>  (1) Introduce long options for cvsimport; the code already uses
>     Getopt::Long, so this shouldn't be too bad.
>
>  (2) Add case-sensitive variant of "git config -l" that shows the config
>     variable names in the original case.
>
>  (3) Upon startup, use "git config -l -f $GIT_DIR/config" to check for
>     historical short name (e.g. "cvsimport.a" or "cvsimport.A").  If
>     there are, map them to longer name, remove the short keys and write
>     the conversion back to the configuration file.  You might want to do
>     the same for "$HOME/.gitconfig" as well.
>
>  (4) Then the rest of the code can stay the same.

We might be able to skip (2) by relying on the fact that if the
lowercase is boolean, the uppercase isn't, and conversely. So when
upgrading we check first for the boolean case, if we get a failure (as
opposed to no value) then we know it's the non-boolean one. Then we
can migrate the values accordingly.

-- 
Giuseppe "Oblomov" Bilotta

Re: git cvsimport and case-insensitive config

[Junio C Hamano]

Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:

> We might be able to skip (2) by relying on the fact that if the
> lowercase is boolean, the uppercase isn't, and conversely.

That was the idea I wrote in an earlier draft of my response that I later
scrapped.  I started with "cvsimport.a?  If it is 'true' then that is -a;
if it names an existing file, then it is -A."  I continued the draft up to
'-p' vs '-P' (the former would begin with a hyphen, the latter likely
wouldn't).  But I don't think you can reliably guess -s/-S (both strings).

A bigger reason is that, if you have _any_ combination that you cannot
reliably guess, you would either need the user to ask for help, or you
need to convert by reading the configuration file case-sensitively
yourself to come up with a reliable conversion.  I opted for the latter.

Re: git cvsimport and case-insensitive config

[Giuseppe Bilotta]

On Tue, Mar 30, 2010 at 11:20 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>
>> We might be able to skip (2) by relying on the fact that if the
>> lowercase is boolean, the uppercase isn't, and conversely.
>
> That was the idea I wrote in an earlier draft of my response that I later
> scrapped.  I started with "cvsimport.a?  If it is 'true' then that is -a;
> if it names an existing file, then it is -A."  I continued the draft up to
> '-p' vs '-P' (the former would begin with a hyphen, the latter likely
> wouldn't).  But I don't think you can reliably guess -s/-S (both strings).

(-s likely has a single character, -S more than one. -S is likely to
have * or ?, -s not.)

> A bigger reason is that, if you have _any_ combination that you cannot
> reliably guess, you would either need the user to ask for help, or you
> need to convert by reading the configuration file case-sensitively
> yourself to come up with a reliable conversion.  I opted for the latter.

Would such a configuration work at all?

-- 
Giuseppe "Oblomov" Bilotta

Re: git cvsimport and case-insensitive config

[Junio C Hamano]

Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:

> (-s likely has a single character, -S more than one. -S is likely to
> have * or ?, -s not.)

The value given to -S can just be 'tests', or even "\.", as the regexp
match is not anchored on either side:

		if ($opt_S && $fn =~ m/$opt_S/) {
		    print "SKIPPING $fn v $rev\n";
		    ...

And "-s" would likely be one or more (but not too many) non-alphanumeric
characters.

But a bigger question is if you can bet on that heuristics, and when the
heuristics does not work, what you would do.

>> A bigger reason is that, if you have _any_ combination that you cannot
>> reliably guess, you would either need the user to ask for help, or you
>> need to convert by reading the configuration file case-sensitively
>> yourself to come up with a reliable conversion.  I opted for the latter.
>
> Would such a configuration work at all?

What configuration?

Re: git cvsimport and case-insensitive config

[Giuseppe Bilotta]

On Wed, Mar 31, 2010 at 1:14 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>
>> (-s likely has a single character, -S more than one. -S is likely to
>> have * or ?, -s not.)
>
> The value given to -S can just be 'tests', or even "\.", as the regexp
> match is not anchored on either side:
>
>                if ($opt_S && $fn =~ m/$opt_S/) {
>                    print "SKIPPING $fn v $rev\n";
>                    ...
>
> And "-s" would likely be one or more (but not too many) non-alphanumeric
> characters.
>
> But a bigger question is if you can bet on that heuristics, and when the
> heuristics does not work, what you would do.

What I wonder is: when would the heuristics not work? Are there git
repositories that have a cvsimport configuration with both -s and -S
specified? I strongly suspect the answer is NO because such a
configuration would not work currently (at least not reliably as the
wrong value would be assigned to at least one of the keys.

>>> A bigger reason is that, if you have _any_ combination that you cannot
>>> reliably guess, you would either need the user to ask for help, or you
>>> need to convert by reading the configuration file case-sensitively
>>> yourself to come up with a reliable conversion.  I opted for the latter.
>>
>> Would such a configuration work at all?
>
> What configuration?

One that specifies either of cvsimport.s and cvsimport.S, or any other
conflicting key, in a way that the present config reading doesn't get
too confused by. I think that for migration it would be sufficient to
just follow the value assignments currently done by the code. With a
big fat WARNING: notice, possibly.


-- 
Giuseppe "Oblomov" Bilotta

Re: git cvsimport and case-insensitive config

[Junio C Hamano]

Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:

> What I wonder is: when would the heuristics not work? Are there git
> repositories that have a cvsimport configuration with both -s and -S
> specified?

You are asking a wrong question.  Setting both -s and -S is not an issue.
Not being able to tell reliably if the value you see for cvsimport.s in
"config -l" output were specified for cvsimport.S or cvsimport.s is only
by inspecting the value is.

> I strongly suspect the answer is NO because such a
> configuration would not work currently (at least not reliably as the
> wrong value would be assigned to at least one of the keys.

So this is a moot point, but people who specified cvsimport.S expecting
that it would stand for -S option are not getting what they want and are
getting -s instead, which is also a case where the configuration is not
working currently.

Silently converting it to an equally non-working configuration is easy;
just treat as if everything were specified for lowercase option.

But that would not help users at all.  I was hoping that you were aiming
for something better than that.  At least that "case-sensitive" patch was.

Re: git cvsimport and case-insensitive config

[Giuseppe Bilotta]

On Wed, Mar 31, 2010 at 11:45 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>
>> What I wonder is: when would the heuristics not work? Are there git
>> repositories that have a cvsimport configuration with both -s and -S
>> specified?
>
> You are asking a wrong question.  Setting both -s and -S is not an issue.
> Not being able to tell reliably if the value you see for cvsimport.s in
> "config -l" output were specified for cvsimport.S or cvsimport.s is only
> by inspecting the value is.

What I'm asking is rahter: is it worth to worry about this?

>> I strongly suspect the answer is NO because such a
>> configuration would not work currently (at least not reliably as the
>> wrong value would be assigned to at least one of the keys.
>
> So this is a moot point, but people who specified cvsimport.S expecting
> that it would stand for -S option are not getting what they want and are
> getting -s instead, which is also a case where the configuration is not
> working currently.
>
> Silently converting it to an equally non-working configuration is easy;
> just treat as if everything were specified for lowercase option.
>
> But that would not help users at all.  I was hoping that you were aiming
> for something better than that.  At least that "case-sensitive" patch was.

Well, I wasn't thinking about a silent conversion for the uncertain
options: in the case of dubious options which we can guess (a vs A, r
vs R), a warning would be given, and for the unguessable case we could
either still give a warning and continue or even abort the process
altogether. Better than the silent conversion (which would not break
anything that isn't broken already anyway), although probably not as
sophisticated as your proposal.

-- 
Giuseppe "Oblomov" Bilotta