Re: [GSoC] cache.h: change READ_GITFILE_ERR constants group to enum

[Junio C Hamano <gitster@pobox.com>]

sheikh hamza <sheikhhamza012@gmail.com> writes:

> From:   sheikh hamza <sheikhhamza012@gmail.com>

Please make sure that the "your name <address>" on your From: header
matches what you write on the Signed-off-by: trailer.

> this patch is my microproject for the GSoC i converted
> the group of constants in cache.h on line 573 to an enum
> named read_gitfile_err to follow newer coding convension
> i intend to contribute as much to git as i can and this is
> my initial contribution i hope to get guidance for future
> contribution. i would be working on the GSoC proposal any
> help would be appreciated, Thank you

Almost nothing in the above paragraph explains what this patch is
about and why we should consider updating our code with it.

It would be a good background story that can be told below the
three-dash line, though.

The body of the log message is where you explain what the change is
about and justify why the change is a good idea.  Both preprocessor
macros (that is what these things are called; "constants" could mean
"const int READ_GIFILE_ERR_STAT_FAILED = 1;" so avoid using the word
to refer to them) and enums can be used to make the code more
descriptive, so the code that processes the error code, e.g.

	int err;
	const char *path = read_gitfile_gently(git, &err);
        if (err == 7)
		die("not a repository: '%s'", git);

can become more readable by using READ_GITFILE_ERR_NOT_A_REPO
instead of a magic constant "7".  But they help readability the same
way, so that is not a justification to change preprocessor macro to
enum.

One reason why folks prefer enum over preprocessor macro is because
some debuggers can show enum values symbolically, while macros are
not even seen by the debuggers.  For example:

	enum read_gitfile_err err;
	const char *path = read_gitfile_gently(git, &err);

	printf("the first letter is '%c'\n", path[0]);

would segfault while attempting to call the printf(), when
read_gitfile_gently() finds an error and returns NULL.  In such a
case, a debugger that knows about the type of variable 'err' can
show you READ_GITFILE_ERR_NOT_A_REPO instead of 7 when you say
"print err".

I suspect however that this benefit is only possible when the type
of err is known to be a particular enum.  Note that I updated the
type of 'err' in the second example, but you could have left the
variable as "int" and it would have been perfectly valid C, but I do
not think a debugger can infer that what is in err is one of the
values taken from "enum read_gitfile_err".

So, I suspect that this patch alone does not give us the potential
"it helps the debugger" benefit, either.  The callsites of this
function that currently pass a pointer to a variable of type "int"
need to be updated if we want to use it as the justification.

So does the type of the latter parameter of read_gitfile_gently()
function need to change, I think.

> Signed-off-by: Muhammad Hamza <sheikhhamza012@gmail.com>
> ---
>  cache.h | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/cache.h b/cache.h
> index 37c899b..6aae035 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -570,14 +570,17 @@ static inline enum object_type object_type(unsigned int mode)
>   */
>  int is_nonbare_repository_dir(struct strbuf *path);
>  
> -#define READ_GITFILE_ERR_STAT_FAILED 1
> -#define READ_GITFILE_ERR_NOT_A_FILE 2
> -#define READ_GITFILE_ERR_OPEN_FAILED 3
> -#define READ_GITFILE_ERR_READ_FAILED 4
> -#define READ_GITFILE_ERR_INVALID_FORMAT 5
> -#define READ_GITFILE_ERR_NO_PATH 6
> -#define READ_GITFILE_ERR_NOT_A_REPO 7
> -#define READ_GITFILE_ERR_TOO_LARGE 8
> +enum read_gitfile_err {
> +	READ_GITFILE_ERR_STAT_FAILED = 1,
> +	READ_GITFILE_ERR_NOT_A_FILE = 2,
> +	READ_GITFILE_ERR_OPEN_FAILED = 3,
> +	READ_GITFILE_ERR_READ_FAILED = 4,
> +	READ_GITFILE_ERR_INVALID_FORMAT = 5,
> +	READ_GITFILE_ERR_NO_PATH = 6,
> +	READ_GITFILE_ERR_NOT_A_REPO = 7,
> +	READ_GITFILE_ERR_TOO_LARGE = 8,
> +};
> +
>  void read_gitfile_error_die(int error_code, const char *path, const char *dir);
>  const char *read_gitfile_gently(const char *path, int *return_error_code);
>  #define read_gitfile(path) read_gitfile_gently((path), NULL)