config: return positive from git_config_parse_key()

git_config_parse_key() returns #define-d error codes, but negated. This
negation is merely a convenience to other parts of config.c that don't
bother inspecting the return value before passing it along. But:

a) There's no good reason why those callers couldn't negate the value
   themselves.

b) This requires sanitization of the negative value when we want to
   exit(3) with the relevant error code.

c) We want to move that into a separate library, and returning only
   negative values no longer makes as much sense.
   
Change git_config_parse_key() to return positive values instead, and
adjust callers accordingly. Callers that sanitize the negative sign for
exit(3) now pass the return value opaquely. Callers that secretly needed
the negative sign now negate the return value themselves.

In doing so, we also fix a bug where "git config <key with no section or
name>" results in a different exit code depending on whether we are
setting or getting config.

Signed-off-by: Glen Choo <[email protected]>

builtin/config.c

@@ -362,8 +362,7 @@ static int get_value(const char *key_, const char *regex_, unsigned flags)
  goto free_strings;
  }
  } else {
- if (git_config_parse_key(key_, &key, NULL)) {
- ret = CONFIG_INVALID_KEY;
+ if ((ret = git_config_parse_key(key_, &key, NULL))) {
  goto free_strings;
  }
  }

config.c

@@ -534,8 +534,9 @@ static inline int iskeychar(int c)
  * Auxiliary function to sanity-check and split the key into the section
  * identifier and variable name.
  *
- * Returns 0 on success, -1 when there is an invalid character in the key and
- * -2 if there is no section name in the key.
+ * Returns 0 on success, CONFIG_INVALID_KEY when there is an invalid character
+ * in the key and CONFIG_NO_SECTION_OR_NAME if there is no section name in the
+ * key.
  *
  * store_key - pointer to char* which will hold a copy of the key with
  * lowercase section and variable name
@@ -555,12 +556,12 @@ int git_config_parse_key(const char *key, char **store_key, size_t *baselen_)
 
  if (last_dot == NULL || last_dot == key) {
  error(_("key does not contain a section: %s"), key);
- return -CONFIG_NO_SECTION_OR_NAME;
+ return CONFIG_NO_SECTION_OR_NAME;
  }
 
  if (!last_dot[1]) {
  error(_("key does not contain variable name: %s"), key);
- return -CONFIG_NO_SECTION_OR_NAME;
+ return CONFIG_NO_SECTION_OR_NAME;
  }
 
  baselen = last_dot - key;
@@ -596,7 +597,7 @@ int git_config_parse_key(const char *key, char **store_key, size_t *baselen_)
 
 out_free_ret_1:
  FREE_AND_NULL(*store_key);
- return -CONFIG_INVALID_KEY;
+ return CONFIG_INVALID_KEY;
 }
 
 static int config_parse_pair(const char *key, const char *value,
@@ -2346,7 +2347,7 @@ static int configset_find_element(struct config_set *set, const char *key,
  * `key` may come from the user, so normalize it before using it
  * for querying entries from the hashmap.
  */
- ret = git_config_parse_key(key, &normalized_key, NULL);
+ ret = -git_config_parse_key(key, &normalized_key, NULL);
  if (ret)
  return ret;
 
@@ -3334,8 +3335,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
  size_t contents_sz;
  struct config_store_data store = CONFIG_STORE_INIT;
 
- /* parse-key returns negative; flip the sign to feed exit(3) */
- ret = 0 - git_config_parse_key(key, &store.key, &store.baselen);
+ ret = git_config_parse_key(key, &store.key, &store.baselen);
  if (ret)
  goto out_free;
 

config.h

@@ -23,7 +23,7 @@
 
 struct object_id;
 
-/* git_config_parse_key() returns these negated: */
+/* git_config_parse_key() returns these: */
 #define CONFIG_INVALID_KEY 1
 #define CONFIG_NO_SECTION_OR_NAME 2
 /* git_config_set_gently(), git_config_set_multivar_gently() return the above or these: */

submodule-config.c

@@ -824,8 +824,8 @@ int print_config_from_gitmodules(struct repository *repo, const char *key)
  char *store_key;
 
  ret = git_config_parse_key(key, &store_key, NULL);
- if (ret < 0)
- return CONFIG_INVALID_KEY;
+ if (ret)
+ return ret;
 
  config_from_gitmodules(config_print_callback, repo, store_key);
 

t/t1300-config.sh

@@ -2590,4 +2590,20 @@ test_expect_success 'includeIf.hasconfig:remote.*.url forbids remote url in such
  grep "fatal: remote URLs cannot be configured in file directly or indirectly included by includeIf.hasconfig:remote.*.url" err
 '
 
+# Exit codes
+test_expect_success '--get with bad key' '
+ # Also exits with 1 if the value is not found
+ test_expect_code 1 git config --get "bad.name\n" 2>err &&
+ grep "error: invalid key" err &&
+ test_expect_code 2 git config --get "bad." 2>err &&
+ grep "error: key does not contain variable name" err
+'
+
+test_expect_success 'set with bad key' '
+ test_expect_code 1 git config "bad.name\n" var 2>err &&
+ grep "error: invalid key" err &&
+ test_expect_code 2 git config "bad." var 2>err &&
+ grep "error: key does not contain variable name" err
+'
+
 test_done