* git cvsimport and case-insensitive config @ 2010-03-30 8:32 Giuseppe Bilotta 2010-03-30 17:29 ` Junio C Hamano 0 siblings, 1 reply; 9+ messages in thread From: Giuseppe Bilotta @ 2010-03-30 8:32 UTC (permalink / raw) To: git 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? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: git cvsimport and case-insensitive config 2010-03-30 8:32 git cvsimport and case-insensitive config Giuseppe Bilotta @ 2010-03-30 17:29 ` Junio C Hamano 2010-03-30 18:05 ` Giuseppe Bilotta 0 siblings, 1 reply; 9+ messages in thread From: Junio C Hamano @ 2010-03-30 17:29 UTC (permalink / raw) To: Giuseppe Bilotta; +Cc: git 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; } ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: git cvsimport and case-insensitive config 2010-03-30 17:29 ` Junio C Hamano @ 2010-03-30 18:05 ` Giuseppe Bilotta 2010-03-30 21:20 ` Junio C Hamano 0 siblings, 1 reply; 9+ messages in thread From: Giuseppe Bilotta @ 2010-03-30 18:05 UTC (permalink / raw) To: Junio C Hamano; +Cc: git 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: git cvsimport and case-insensitive config 2010-03-30 18:05 ` Giuseppe Bilotta @ 2010-03-30 21:20 ` Junio C Hamano 2010-03-30 22:17 ` Giuseppe Bilotta 0 siblings, 1 reply; 9+ messages in thread From: Junio C Hamano @ 2010-03-30 21:20 UTC (permalink / raw) To: Giuseppe Bilotta; +Cc: git 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: git cvsimport and case-insensitive config 2010-03-30 21:20 ` Junio C Hamano @ 2010-03-30 22:17 ` Giuseppe Bilotta 2010-03-30 23:14 ` Junio C Hamano 0 siblings, 1 reply; 9+ messages in thread From: Giuseppe Bilotta @ 2010-03-30 22:17 UTC (permalink / raw) To: Junio C Hamano; +Cc: git 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: git cvsimport and case-insensitive config 2010-03-30 22:17 ` Giuseppe Bilotta @ 2010-03-30 23:14 ` Junio C Hamano 2010-03-31 6:54 ` Giuseppe Bilotta 0 siblings, 1 reply; 9+ messages in thread From: Junio C Hamano @ 2010-03-30 23:14 UTC (permalink / raw) To: Giuseppe Bilotta; +Cc: Junio C Hamano, git 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? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: git cvsimport and case-insensitive config 2010-03-30 23:14 ` Junio C Hamano @ 2010-03-31 6:54 ` Giuseppe Bilotta 2010-03-31 9:45 ` Junio C Hamano 0 siblings, 1 reply; 9+ messages in thread From: Giuseppe Bilotta @ 2010-03-31 6:54 UTC (permalink / raw) To: Junio C Hamano; +Cc: git 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: git cvsimport and case-insensitive config 2010-03-31 6:54 ` Giuseppe Bilotta @ 2010-03-31 9:45 ` Junio C Hamano 2010-03-31 11:17 ` Giuseppe Bilotta 0 siblings, 1 reply; 9+ messages in thread From: Junio C Hamano @ 2010-03-31 9:45 UTC (permalink / raw) To: Giuseppe Bilotta; +Cc: git 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: git cvsimport and case-insensitive config 2010-03-31 9:45 ` Junio C Hamano @ 2010-03-31 11:17 ` Giuseppe Bilotta 0 siblings, 0 replies; 9+ messages in thread From: Giuseppe Bilotta @ 2010-03-31 11:17 UTC (permalink / raw) To: Junio C Hamano; +Cc: git 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2010-03-31 11:17 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2010-03-30 8:32 git cvsimport and case-insensitive config Giuseppe Bilotta 2010-03-30 17:29 ` Junio C Hamano 2010-03-30 18:05 ` Giuseppe Bilotta 2010-03-30 21:20 ` Junio C Hamano 2010-03-30 22:17 ` Giuseppe Bilotta 2010-03-30 23:14 ` Junio C Hamano 2010-03-31 6:54 ` Giuseppe Bilotta 2010-03-31 9:45 ` Junio C Hamano 2010-03-31 11:17 ` Giuseppe Bilotta
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.