{"thread":{"id":"53059","subject":"[GSoC] cache.h: change READ_GITFILE_ERR constants group to enum","startedAt":"2020-03-22T17:36:22Z","lastAt":"2020-03-22T23:38:15Z","messageCount":2,"participants":["sheikh hamza","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"393727","messageId":"1584898493-27180-1-git-send-email-sheikhhamza012@gmail.com","threadId":"53059","inReplyTo":"sheikhhamza012@gmail.com","subject":"[GSoC] cache.h: change READ_GITFILE_ERR constants group to enum","fromName":"sheikh hamza","fromEmail":"sheikhhamza012@gmail.com","sentAt":"2020-03-22T17:34:53Z","receivedAt":"2020-03-22T17:36:22Z","isPatch":false,"sender":{"key":"sheikhhamza012@gmail.com","avatar":"https://gravatar.com/avatar/67e765e1aa9c192c25a00a31a51b83deb2afde34d497f0bf90867e71cd36c87b?d=mp&s=160"},"body":"this patch is my microproject for the GSoC i converted\nthe group of constants in cache.h on line 573 to an enum\nnamed read_gitfile_err to follow newer coding convension\ni intend to contribute as much to git as i can and this is\nmy initial contribution i hope to get guidance for future\ncontribution. i would be working on the GSoC proposal any\nhelp would be appreciated, Thank you\n\nSigned-off-by: Muhammad Hamza \n---\n cache.h | 19 +++++++++++--------\n 1 file changed, 11 insertions(+), 8 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 37c899b..6aae035 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -570,14 +570,17 @@ static inline enum object_type object_type(unsigned int mode)\n */\n int is_nonbare_repository_dir(struct strbuf *path);\n \n-#define READ_GITFILE_ERR_STAT_FAILED 1\n-#define READ_GITFILE_ERR_NOT_A_FILE 2\n-#define READ_GITFILE_ERR_OPEN_FAILED 3\n-#define READ_GITFILE_ERR_READ_FAILED 4\n-#define READ_GITFILE_ERR_INVALID_FORMAT 5\n-#define READ_GITFILE_ERR_NO_PATH 6\n-#define READ_GITFILE_ERR_NOT_A_REPO 7\n-#define READ_GITFILE_ERR_TOO_LARGE 8\n+enum read_gitfile_err {\n+\tREAD_GITFILE_ERR_STAT_FAILED = 1,\n+\tREAD_GITFILE_ERR_NOT_A_FILE = 2,\n+\tREAD_GITFILE_ERR_OPEN_FAILED = 3,\n+\tREAD_GITFILE_ERR_READ_FAILED = 4,\n+\tREAD_GITFILE_ERR_INVALID_FORMAT = 5,\n+\tREAD_GITFILE_ERR_NO_PATH = 6,\n+\tREAD_GITFILE_ERR_NOT_A_REPO = 7,\n+\tREAD_GITFILE_ERR_TOO_LARGE = 8,\n+};\n+\n void read_gitfile_error_die(int error_code, const char *path, const char *dir);\n const char *read_gitfile_gently(const char *path, int *return_error_code);\n #define read_gitfile(path) read_gitfile_gently((path), NULL)\n-- \n1.9.1\n\n"},{"id":"393748","messageId":"xmqqv9mw6jvj.fsf@gitster.c.googlers.com","threadId":"53059","inReplyTo":"1584898493-27180-1-git-send-email-sheikhhamza012@gmail.com","subject":"Re: [GSoC] cache.h: change READ_GITFILE_ERR constants group to enum","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-22T23:38:08Z","receivedAt":"2020-03-22T23:38:15Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"sheikh hamza writes:\n\n> From: sheikh hamza \n\nPlease make sure that the \"your name
\" on your From: header\nmatches what you write on the Signed-off-by: trailer.\n\n> this patch is my microproject for the GSoC i converted\n> the group of constants in cache.h on line 573 to an enum\n> named read_gitfile_err to follow newer coding convension\n> i intend to contribute as much to git as i can and this is\n> my initial contribution i hope to get guidance for future\n> contribution. i would be working on the GSoC proposal any\n> help would be appreciated, Thank you\n\nAlmost nothing in the above paragraph explains what this patch is\nabout and why we should consider updating our code with it.\n\nIt would be a good background story that can be told below the\nthree-dash line, though.\n\nThe body of the log message is where you explain what the change is\nabout and justify why the change is a good idea. Both preprocessor\nmacros (that is what these things are called; \"constants\" could mean\n\"const int READ_GIFILE_ERR_STAT_FAILED = 1;\" so avoid using the word\nto refer to them) and enums can be used to make the code more\ndescriptive, so the code that processes the error code, e.g.\n\n\tint err;\n\tconst char *path = read_gitfile_gently(git, &err);\n if (err == 7)\n\t\tdie(\"not a repository: '%s'\", git);\n\ncan become more readable by using READ_GITFILE_ERR_NOT_A_REPO\ninstead of a magic constant \"7\". But they help readability the same\nway, so that is not a justification to change preprocessor macro to\nenum.\n\nOne reason why folks prefer enum over preprocessor macro is because\nsome debuggers can show enum values symbolically, while macros are\nnot even seen by the debuggers. For example:\n\n\tenum read_gitfile_err err;\n\tconst char *path = read_gitfile_gently(git, &err);\n\n\tprintf(\"the first letter is '%c'\\n\", path[0]);\n\nwould segfault while attempting to call the printf(), when\nread_gitfile_gently() finds an error and returns NULL. In such a\ncase, a debugger that knows about the type of variable 'err' can\nshow you READ_GITFILE_ERR_NOT_A_REPO instead of 7 when you say\n\"print err\".\n\nI suspect however that this benefit is only possible when the type\nof err is known to be a particular enum. Note that I updated the\ntype of 'err' in the second example, but you could have left the\nvariable as \"int\" and it would have been perfectly valid C, but I do\nnot think a debugger can infer that what is in err is one of the\nvalues taken from \"enum read_gitfile_err\".\n\nSo, I suspect that this patch alone does not give us the potential\n\"it helps the debugger\" benefit, either. The callsites of this\nfunction that currently pass a pointer to a variable of type \"int\"\nneed to be updated if we want to use it as the justification.\n\nSo does the type of the latter parameter of read_gitfile_gently()\nfunction need to change, I think.\n\n> Signed-off-by: Muhammad Hamza \n> ---\n> cache.h | 19 +++++++++++--------\n> 1 file changed, 11 insertions(+), 8 deletions(-)\n>\n> diff --git a/cache.h b/cache.h\n> index 37c899b..6aae035 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -570,14 +570,17 @@ static inline enum object_type object_type(unsigned int mode)\n> */\n> int is_nonbare_repository_dir(struct strbuf *path);\n> \n> -#define READ_GITFILE_ERR_STAT_FAILED 1\n> -#define READ_GITFILE_ERR_NOT_A_FILE 2\n> -#define READ_GITFILE_ERR_OPEN_FAILED 3\n> -#define READ_GITFILE_ERR_READ_FAILED 4\n> -#define READ_GITFILE_ERR_INVALID_FORMAT 5\n> -#define READ_GITFILE_ERR_NO_PATH 6\n> -#define READ_GITFILE_ERR_NOT_A_REPO 7\n> -#define READ_GITFILE_ERR_TOO_LARGE 8\n> +enum read_gitfile_err {\n> +\tREAD_GITFILE_ERR_STAT_FAILED = 1,\n> +\tREAD_GITFILE_ERR_NOT_A_FILE = 2,\n> +\tREAD_GITFILE_ERR_OPEN_FAILED = 3,\n> +\tREAD_GITFILE_ERR_READ_FAILED = 4,\n> +\tREAD_GITFILE_ERR_INVALID_FORMAT = 5,\n> +\tREAD_GITFILE_ERR_NO_PATH = 6,\n> +\tREAD_GITFILE_ERR_NOT_A_REPO = 7,\n> +\tREAD_GITFILE_ERR_TOO_LARGE = 8,\n> +};\n> +\n> void read_gitfile_error_die(int error_code, const char *path, const char *dir);\n> const char *read_gitfile_gently(const char *path, int *return_error_code);\n> #define read_gitfile(path) read_gitfile_gently((path), NULL)\n"}]}