{"thread":{"id":"23250","subject":"git cvsimport and case-insensitive config","startedAt":"2010-03-30T08:32:37Z","lastAt":"2010-03-31T11:17:01Z","messageCount":9,"participants":["Giuseppe Bilotta","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"138168","messageId":"hoscv7$hmn$1@dough.gmane.org","threadId":"23250","inReplyTo":null,"subject":"git cvsimport and case-insensitive config","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-03-30T08:32:37Z","receivedAt":"2010-03-30T08:32:37Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"Hello all,\n\nI just noticed that there's an annoying buglet in the way git cvsimport \nstores its configuration and the case-insensitive key lookup done by git.\n\ngit cvsimport looks for cvsimport.* single-letter keys that match the \ncommand-line option (e.g. cvsimport.r for -r). However, since there are some \nsingle-letter options which only differ by case (a vs A, r vs R) if you set \neither you get annoying messages (and potentially also odd results, although \nI haven't come across these yet).\n\nI come with no patch because I'm not really sure what would be the best \napproach at fixing this. Changing the way cvsimport matches config to \ncommand-line options? An option to make git-config case-sensitive re. key \nnames? Something else?\n"},{"id":"138195","messageId":"7vy6h9vhuk.fsf@alter.siamese.dyndns.org","threadId":"23250","inReplyTo":"hoscv7$hmn$1@dough.gmane.org","subject":"Re: git cvsimport and case-insensitive config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-03-30T17:29:07Z","receivedAt":"2010-03-30T17:29:07Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Giuseppe Bilotta writes:\n\n> git cvsimport looks for cvsimport.* single-letter keys that match the \n> command-line option (e.g. cvsimport.r for -r). However, since there are some \n> single-letter options which only differ by case (a vs A, r vs R) if you set \n> either you get annoying messages (and potentially also odd results, although \n> I haven't come across these yet).\n\nOuch.\n\nThe only sensible solution in the longer term is to eventually rename them\nto spell them out e.g. cvsimport.remote vs cvsimport.userevisionmap.\n\nThe transition cost would be the same for either approach.\n\n (1) Introduce long options for cvsimport; the code already uses\n Getopt::Long, so this shouldn't be too bad.\n\n (2) Add case-sensitive variant of \"git config -l\" that shows the config\n variable names in the original case.\n\n (3) Upon startup, use \"git config -l -f $GIT_DIR/config\" to check for\n historical short name (e.g. \"cvsimport.a\" or \"cvsimport.A\"). If\n there are, map them to longer name, remove the short keys and write\n the conversion back to the configuration file. You might want to do\n the same for \"$HOME/.gitconfig\" as well.\n\n (4) Then the rest of the code can stay the same.\n\nStep (2) may look like this. Note that I made this \"list with case\"\nhidden and inaccessible on purpose, as this is primarily to write a tool\nto recover from mistakes like this.\n\n builtin/config.c | 10 +++++++++-\n cache.h | 1 +\n config.c | 12 +++++++++---\n 3 files changed, 19 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 4bc46b1..3684c3a 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -41,6 +41,7 @@ static int end_null;\n #define ACTION_SET_ALL (1<<12)\n #define ACTION_GET_COLOR (1<<13)\n #define ACTION_GET_COLORBOOL (1<<14)\n+#define ACTION_LIST_KEYS_WITH_CASE (1<<15)\n \n #define TYPE_BOOL (1<<0)\n #define TYPE_INT (1<<1)\n@@ -73,6 +74,11 @@ static struct option builtin_config_options[] = {\n \tOPT_BIT(0, \"path\", &types, \"value is a path (file or directory name)\", TYPE_PATH),\n \tOPT_GROUP(\"Other\"),\n \tOPT_BOOLEAN('z', \"null\", &end_null, \"terminate values with NUL byte\"),\n+\t{\n+\t\tOPTION_BIT, 0, \"list-keys-with-case\", &actions,\n+\t\tNULL, \"list for conversion\", PARSE_OPT_NOARG|PARSE_OPT_HIDDEN,\n+\t\tNULL, ACTION_LIST_KEYS_WITH_CASE,\n+\t},\n \tOPT_END(),\n };\n \n@@ -397,7 +403,9 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix)\n \t\t\tusage_with_options(builtin_config_usage, builtin_config_options);\n \t\t}\n \n-\tif (actions == ACTION_LIST) {\n+\tif (actions == ACTION_LIST || actions == ACTION_LIST_KEYS_WITH_CASE) {\n+\t\tif (actions == ACTION_LIST_KEYS_WITH_CASE)\n+\t\t\tconfig_return_keys_with_case = 1;\n \t\tcheck_argc(argc, 0, 0);\n \t\tif (git_config(show_all_config, NULL) < 0) {\n \t\t\tif (config_exclusive_filename)\ndiff --git a/cache.h b/cache.h\nindex 6dcb100..4320288 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -953,6 +953,7 @@ extern int git_config_system(void);\n extern int git_config_global(void);\n extern int config_error_nonbool(const char *);\n extern const char *config_exclusive_filename;\n+extern int config_return_keys_with_case;\n \n #define MAX_GITNAME (1000)\n extern char git_default_email[MAX_GITNAME];\ndiff --git a/config.c b/config.c\nindex 6963fbe..c548cec 100644\n--- a/config.c\n+++ b/config.c\n@@ -17,6 +17,11 @@ static int config_file_eof;\n static int zlib_compression_seen;\n \n const char *config_exclusive_filename = NULL;\n+/*\n+ * only used for config --list-case-sensitive-keys for recovering\n+ * from mistakes.\n+ */\n+int config_return_keys_with_case;\n \n static int get_next_char(void)\n {\n@@ -123,7 +128,7 @@ static int get_value(config_fn_t fn, void *data, char *name, unsigned int len)\n \t\t\tbreak;\n \t\tif (!iskeychar(c))\n \t\t\tbreak;\n-\t\tname[len++] = tolower(c);\n+\t\tname[len++] = config_return_keys_with_case ? c : tolower(c);\n \t\tif (len >= MAXNAME)\n \t\t\treturn -1;\n \t}\n@@ -193,7 +198,8 @@ static int get_base_var(char *name)\n \t\t\treturn -1;\n \t\tif (baselen > MAXNAME / 2)\n \t\t\treturn -1;\n-\t\tname[baselen++] = tolower(c);\n+\t\tname[baselen++] = config_return_keys_with_case\n+\t\t\t? c : tolower(c);\n \t}\n }\n \n@@ -246,7 +252,7 @@ static int git_parse_file(config_fn_t fn, void *data)\n \t\t}\n \t\tif (!isalpha(c))\n \t\t\tbreak;\n-\t\tvar[baselen] = tolower(c);\n+\t\tvar[baselen] = config_return_keys_with_case ? c : tolower(c);\n \t\tif (get_value(fn, data, var, baselen+1) < 0)\n \t\t\tbreak;\n \t}\n"},{"id":"138199","messageId":"cb7bb73a1003301105v691624sdcdadf6809c50b89@mail.gmail.com","threadId":"23250","inReplyTo":"7vy6h9vhuk.fsf@alter.siamese.dyndns.org","subject":"Re: git cvsimport and case-insensitive config","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-03-30T18:05:57Z","receivedAt":"2010-03-30T18:05:57Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Tue, Mar 30, 2010 at 7:29 PM, Junio C Hamano wrote:\n> Giuseppe Bilotta writes:\n>\n>> git cvsimport looks for cvsimport.* single-letter keys that match the\n>> command-line option (e.g. cvsimport.r for -r). However, since there are some\n>> single-letter options which only differ by case (a vs A, r vs R) if you set\n>> either you get annoying messages (and potentially also odd results, although\n>> I haven't come across these yet).\n>\n> Ouch.\n>\n> The only sensible solution in the longer term is to eventually rename them\n> to spell them out e.g. cvsimport.remote vs cvsimport.userevisionmap.\n>\n> The transition cost would be the same for either approach.\n>\n>  (1) Introduce long options for cvsimport; the code already uses\n>     Getopt::Long, so this shouldn't be too bad.\n>\n>  (2) Add case-sensitive variant of \"git config -l\" that shows the config\n>     variable names in the original case.\n>\n>  (3) Upon startup, use \"git config -l -f $GIT_DIR/config\" to check for\n>     historical short name (e.g. \"cvsimport.a\" or \"cvsimport.A\").  If\n>     there are, map them to longer name, remove the short keys and write\n>     the conversion back to the configuration file.  You might want to do\n>     the same for \"$HOME/.gitconfig\" as well.\n>\n>  (4) Then the rest of the code can stay the same.\n\nWe might be able to skip (2) by relying on the fact that if the\nlowercase is boolean, the uppercase isn't, and conversely. So when\nupgrading we check first for the boolean case, if we get a failure (as\nopposed to no value) then we know it's the non-boolean one. Then we\ncan migrate the values accordingly.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"138216","messageId":"7vr5n1v74x.fsf@alter.siamese.dyndns.org","threadId":"23250","inReplyTo":"cb7bb73a1003301105v691624sdcdadf6809c50b89@mail.gmail.com","subject":"Re: git cvsimport and case-insensitive config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-03-30T21:20:30Z","receivedAt":"2010-03-30T21:20:30Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Giuseppe Bilotta writes:\n\n> We might be able to skip (2) by relying on the fact that if the\n> lowercase is boolean, the uppercase isn't, and conversely.\n\nThat was the idea I wrote in an earlier draft of my response that I later\nscrapped. I started with \"cvsimport.a? If it is 'true' then that is -a;\nif it names an existing file, then it is -A.\" I continued the draft up to\n'-p' vs '-P' (the former would begin with a hyphen, the latter likely\nwouldn't). But I don't think you can reliably guess -s/-S (both strings).\n\nA bigger reason is that, if you have _any_ combination that you cannot\nreliably guess, you would either need the user to ask for help, or you\nneed to convert by reading the configuration file case-sensitively\nyourself to come up with a reliable conversion. I opted for the latter.\n"},{"id":"138224","messageId":"cb7bb73a1003301517r29aed329j4a5f38fbc9b515ca@mail.gmail.com","threadId":"23250","inReplyTo":"7vr5n1v74x.fsf@alter.siamese.dyndns.org","subject":"Re: git cvsimport and case-insensitive config","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-03-30T22:17:31Z","receivedAt":"2010-03-30T22:17:31Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Tue, Mar 30, 2010 at 11:20 PM, Junio C Hamano wrote:\n> Giuseppe Bilotta writes:\n>\n>> We might be able to skip (2) by relying on the fact that if the\n>> lowercase is boolean, the uppercase isn't, and conversely.\n>\n> That was the idea I wrote in an earlier draft of my response that I later\n> scrapped.  I started with \"cvsimport.a?  If it is 'true' then that is -a;\n> if it names an existing file, then it is -A.\"  I continued the draft up to\n> '-p' vs '-P' (the former would begin with a hyphen, the latter likely\n> wouldn't).  But I don't think you can reliably guess -s/-S (both strings).\n\n(-s likely has a single character, -S more than one. -S is likely to\nhave * or ?, -s not.)\n\n> A bigger reason is that, if you have _any_ combination that you cannot\n> reliably guess, you would either need the user to ask for help, or you\n> need to convert by reading the configuration file case-sensitively\n> yourself to come up with a reliable conversion.  I opted for the latter.\n\nWould such a configuration work at all?\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"138235","messageId":"7vd3yls8pi.fsf@alter.siamese.dyndns.org","threadId":"23250","inReplyTo":"cb7bb73a1003301517r29aed329j4a5f38fbc9b515ca@mail.gmail.com","subject":"Re: git cvsimport and case-insensitive config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-03-30T23:14:49Z","receivedAt":"2010-03-30T23:14:49Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Giuseppe Bilotta writes:\n\n> (-s likely has a single character, -S more than one. -S is likely to\n> have * or ?, -s not.)\n\nThe value given to -S can just be 'tests', or even \"\\.\", as the regexp\nmatch is not anchored on either side:\n\n\t\tif ($opt_S && $fn =~ m/$opt_S/) {\n\t\t print \"SKIPPING $fn v $rev\\n\";\n\t\t ...\n\nAnd \"-s\" would likely be one or more (but not too many) non-alphanumeric\ncharacters.\n\nBut a bigger question is if you can bet on that heuristics, and when the\nheuristics does not work, what you would do.\n\n>> A bigger reason is that, if you have _any_ combination that you cannot\n>> reliably guess, you would either need the user to ask for help, or you\n>> need to convert by reading the configuration file case-sensitively\n>> yourself to come up with a reliable conversion.  I opted for the latter.\n>\n> Would such a configuration work at all?\n\nWhat configuration?\n"},{"id":"138261","messageId":"y2kcb7bb73a1003302354wea191dd4iced4360b95d31a2a@mail.gmail.com","threadId":"23250","inReplyTo":"7vd3yls8pi.fsf@alter.siamese.dyndns.org","subject":"Re: git cvsimport and case-insensitive config","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-03-31T06:54:50Z","receivedAt":"2010-03-31T06:54:50Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Wed, Mar 31, 2010 at 1:14 AM, Junio C Hamano wrote:\n> Giuseppe Bilotta writes:\n>\n>> (-s likely has a single character, -S more than one. -S is likely to\n>> have * or ?, -s not.)\n>\n> The value given to -S can just be 'tests', or even \"\\.\", as the regexp\n> match is not anchored on either side:\n>\n>                if ($opt_S && $fn =~ m/$opt_S/) {\n>                    print \"SKIPPING $fn v $rev\\n\";\n>                    ...\n>\n> And \"-s\" would likely be one or more (but not too many) non-alphanumeric\n> characters.\n>\n> But a bigger question is if you can bet on that heuristics, and when the\n> heuristics does not work, what you would do.\n\nWhat I wonder is: when would the heuristics not work? Are there git\nrepositories that have a cvsimport configuration with both -s and -S\nspecified? I strongly suspect the answer is NO because such a\nconfiguration would not work currently (at least not reliably as the\nwrong value would be assigned to at least one of the keys.\n\n>>> A bigger reason is that, if you have _any_ combination that you cannot\n>>> reliably guess, you would either need the user to ask for help, or you\n>>> need to convert by reading the configuration file case-sensitively\n>>> yourself to come up with a reliable conversion.  I opted for the latter.\n>>\n>> Would such a configuration work at all?\n>\n> What configuration?\n\nOne that specifies either of cvsimport.s and cvsimport.S, or any other\nconflicting key, in a way that the present config reading doesn't get\ntoo confused by. I think that for migration it would be sufficient to\njust follow the value assignments currently done by the code. With a\nbig fat WARNING: notice, possibly.\n\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"138269","messageId":"7v634cq0yj.fsf@alter.siamese.dyndns.org","threadId":"23250","inReplyTo":"y2kcb7bb73a1003302354wea191dd4iced4360b95d31a2a@mail.gmail.com","subject":"Re: git cvsimport and case-insensitive config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-03-31T09:45:08Z","receivedAt":"2010-03-31T09:45:08Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Giuseppe Bilotta writes:\n\n> What I wonder is: when would the heuristics not work? Are there git\n> repositories that have a cvsimport configuration with both -s and -S\n> specified?\n\nYou are asking a wrong question. Setting both -s and -S is not an issue.\nNot being able to tell reliably if the value you see for cvsimport.s in\n\"config -l\" output were specified for cvsimport.S or cvsimport.s is only\nby inspecting the value is.\n\n> I strongly suspect the answer is NO because such a\n> configuration would not work currently (at least not reliably as the\n> wrong value would be assigned to at least one of the keys.\n\nSo this is a moot point, but people who specified cvsimport.S expecting\nthat it would stand for -S option are not getting what they want and are\ngetting -s instead, which is also a case where the configuration is not\nworking currently.\n\nSilently converting it to an equally non-working configuration is easy;\njust treat as if everything were specified for lowercase option.\n\nBut that would not help users at all. I was hoping that you were aiming\nfor something better than that. At least that \"case-sensitive\" patch was.\n"},{"id":"138273","messageId":"p2mcb7bb73a1003310417u3821f8b1hd3df79e8b41502c3@mail.gmail.com","threadId":"23250","inReplyTo":"7v634cq0yj.fsf@alter.siamese.dyndns.org","subject":"Re: git cvsimport and case-insensitive config","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-03-31T11:17:01Z","receivedAt":"2010-03-31T11:17:01Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Wed, Mar 31, 2010 at 11:45 AM, Junio C Hamano wrote:\n> Giuseppe Bilotta writes:\n>\n>> What I wonder is: when would the heuristics not work? Are there git\n>> repositories that have a cvsimport configuration with both -s and -S\n>> specified?\n>\n> You are asking a wrong question.  Setting both -s and -S is not an issue.\n> Not being able to tell reliably if the value you see for cvsimport.s in\n> \"config -l\" output were specified for cvsimport.S or cvsimport.s is only\n> by inspecting the value is.\n\nWhat I'm asking is rahter: is it worth to worry about this?\n\n>> I strongly suspect the answer is NO because such a\n>> configuration would not work currently (at least not reliably as the\n>> wrong value would be assigned to at least one of the keys.\n>\n> So this is a moot point, but people who specified cvsimport.S expecting\n> that it would stand for -S option are not getting what they want and are\n> getting -s instead, which is also a case where the configuration is not\n> working currently.\n>\n> Silently converting it to an equally non-working configuration is easy;\n> just treat as if everything were specified for lowercase option.\n>\n> But that would not help users at all.  I was hoping that you were aiming\n> for something better than that.  At least that \"case-sensitive\" patch was.\n\nWell, I wasn't thinking about a silent conversion for the uncertain\noptions: in the case of dubious options which we can guess (a vs A, r\nvs R), a warning would be given, and for the unguessable case we could\neither still give a warning and continue or even abort the process\naltogether. Better than the silent conversion (which would not break\nanything that isn't broken already anyway), although probably not as\nsophisticated as your proposal.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"}]}