Re: [PATCH 1/2] config: define and document exit codes

[Thiago Farina <tfransosi@gmail.com>]

On Tue, May 17, 2011 at 12:38 PM, Michael J Gruber
<git@drmicha.warpmail.net> wrote:
> The return codes of git_config_set() and friends are magic numbers right
> in the source. #define them in cache.h where the functions are declared,

Why defining them on cache.h? Just because the functions are declared
there? Is this a good reason? I think this pollute even more the
cache.h and these constants are not used outside of config.c. So I'd
move them back onto config.c. Maybe it's fine as is though.

> and use the constants in the source.
>
> Also, mention the resulting exit codes of "git config" in its man page
> (and complete the list).
>
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> ---
>  Documentation/git-config.txt |   18 ++++++++++--------
>  cache.h                      |   10 ++++++++++
>  config.c                     |   20 ++++++++++----------
>  3 files changed, 30 insertions(+), 18 deletions(-)
>
> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> index 8804de3..e7ecf5d 100644
> --- a/Documentation/git-config.txt
> +++ b/Documentation/git-config.txt
> @@ -50,16 +50,18 @@ The default is to assume the config file of the current repository,
>  .git/config unless defined otherwise with GIT_DIR and GIT_CONFIG
>  (see <<FILES>>).
>
> -This command will fail if:
> -
> -. The config file is invalid,
> -. Can not write to the config file,
> -. no section was provided,
> -. the section or key is invalid,
> -. you try to unset an option which does not exist,
> -. you try to unset/set an option for which multiple lines match, or
> -. you use '--global' option without $HOME being properly set.
> -
> +This command will fail (with exit code ret) if:
> +
> +. The config file is invalid (ret=3),
> +. can not write to the config file (ret=4),
> +. no section or name was provided (ret=2),
> +. the section or key is invalid (ret=1),
> +. you try to unset an option which does not exist (ret=5),
> +. you try to unset/set an option for which multiple lines match (ret=5),
> +. you try to use an invalid regexp (ret=6), or
> +. you use '--global' option without $HOME being properly set (ret=128).
> +
> +On success, the command returns the exit code 0.
>
>  OPTIONS
>  -------
> diff --git a/cache.h b/cache.h
> index 2225c3f..57a98d2 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1018,6 +1018,16 @@ extern const char *packed_object_info_detail(struct packed_git *, off_t, unsigne
>  /* Dumb servers support */
>  extern int update_server_info(int);
>
> +/* git_config_parse_key() returns these negated: */
> +#define CONFIG_INVALID_KEY 1
> +#define CONFIG_NO_SECTION_OR_NAME 2
> +/* git_config_set(), git_config_set_multivar() return the above or these: */
> +#define CONFIG_NO_LOCK -1
> +#define CONFIG_INVALID_FILE 3
> +#define CONFIG_NO_WRITE 4
> +#define CONFIG_NOTHING_SET 5
> +#define CONFIG_INVALID_PATTERN 6
> +
>  typedef int (*config_fn_t)(const char *, const char *, void *);
>  extern int git_default_config(const char *, const char *, void *);
>  extern int git_config_from_file(config_fn_t fn, const char *, void *);
> diff --git a/config.c b/config.c
> index 671c8df..9d36848 100644
> --- a/config.c
> +++ b/config.c
> @@ -1123,12 +1123,12 @@ int git_config_parse_key(const char *key, char **store_key, int *baselen_)
>
>        if (last_dot == NULL || last_dot == key) {
>                error("key does not contain a section: %s", key);
> -               return -2;
> +               return -CONFIG_NO_SECTION_OR_NAME;
>        }
>
>        if (!last_dot[1]) {
>                error("key does not contain variable name: %s", key);
> -               return -2;
> +               return -CONFIG_NO_SECTION_OR_NAME;
>        }
>
>        baselen = last_dot - key;
> @@ -1165,7 +1165,7 @@ int git_config_parse_key(const char *key, char **store_key, int *baselen_)
>
>  out_free_ret_1:
>        free(*store_key);
> -       return -1;
> +       return -CONFIG_INVALID_KEY;
>  }
>
>  /*
> @@ -1221,7 +1221,7 @@ int git_config_set_multivar(const char *key, const char *value,
>        if (fd < 0) {
>                error("could not lock config file %s: %s", config_filename, strerror(errno));
>                free(store.key);
> -               ret = -1;
> +               ret = CONFIG_NO_LOCK;
>                goto out_free;
>        }
>
> @@ -1235,12 +1235,12 @@ int git_config_set_multivar(const char *key, const char *value,
>                if ( ENOENT != errno ) {
>                        error("opening %s: %s", config_filename,
>                              strerror(errno));
> -                       ret = 3; /* same as "invalid config file" */
> +                       ret = CONFIG_INVALID_FILE; /* same as "invalid config file" */
>                        goto out_free;
>                }
>                /* if nothing to unset, error out */
>                if (value == NULL) {
> -                       ret = 5;
> +                       ret = CONFIG_NOTHING_SET;
>                        goto out_free;
>                }
>
> @@ -1268,7 +1268,7 @@ int git_config_set_multivar(const char *key, const char *value,
>                                        REG_EXTENDED)) {
>                                error("invalid pattern: %s", value_regex);
>                                free(store.value_regex);
> -                               ret = 6;
> +                               ret = CONFIG_INVALID_PATTERN;
>                                goto out_free;
>                        }
>                }
> @@ -1290,7 +1290,7 @@ int git_config_set_multivar(const char *key, const char *value,
>                                regfree(store.value_regex);
>                                free(store.value_regex);
>                        }
> -                       ret = 3;
> +                       ret = CONFIG_INVALID_FILE;
>                        goto out_free;
>                }
>
> @@ -1303,7 +1303,7 @@ int git_config_set_multivar(const char *key, const char *value,
>                /* if nothing to unset, or too many matches, error out */
>                if ((store.seen == 0 && value == NULL) ||
>                                (store.seen > 1 && multi_replace == 0)) {
> -                       ret = 5;
> +                       ret = CONFIG_NOTHING_SET;
>                        goto out_free;
>                }
>
> @@ -1364,7 +1364,7 @@ int git_config_set_multivar(const char *key, const char *value,
>
>        if (commit_lock_file(lock) < 0) {
>                error("could not commit config file %s", config_filename);
> -               ret = 4;
> +               ret = CONFIG_NO_WRITE;
>                goto out_free;
>        }
>
> --
> 1.7.5.1.514.gd181fb
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>