{"thread":{"id":"57194","subject":"[PATCH] submodule.h: use a named enum for RECURSE_SUBMODULES_*","startedAt":"2022-01-05T19:26:30Z","lastAt":"2022-04-04T21:23:48Z","messageCount":7,"participants":["Philippe Blain via GitGitGadget","Junio C Hamano","Philippe Blain","Glen Choo"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"445555","messageId":"pull.1111.git.1641410782015.gitgitgadget@gmail.com","threadId":"57194","inReplyTo":null,"subject":"[PATCH] submodule.h: use a named enum for RECURSE_SUBMODULES_*","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-01-05T19:26:21Z","receivedAt":"2022-01-05T19:26:30Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain \n\nUsing a named enum allows casting an integer to the enum type in both\nGDB and LLDB:\n\n (gdb) p (enum diff_submodule_format) options->submodule_format\n $1 = DIFF_SUBMODULE_LOG\n\n (lldb) p (diff_submodule_format) options->submodule_format\n (diff_submodule_format) $1 = DIFF_SUBMODULE_LOG\n\nIn LLDB, it's also required to cast in the reversed direction, i.e.\ncast an enum constant into its corresponding integer:\n\n (lldb) p (int) diff_submodule_format::DIFF_SUBMODULE_SHORT\n (int) $0 = 0\n\nName the enum listing the different RECURSE_SUBMODULES_* modes, to make\ndebugging easier.\n\nSigned-off-by: Philippe Blain \n---\n submodule.h: use a named enum for RECURSE_SUBMODULES_*\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1111%2Fphil-blain%2Fsubmodule-recurse-enum-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1111/phil-blain/submodule-recurse-enum-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1111\n\n submodule.h | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/submodule.h b/submodule.h\nindex 6bd2c99fd99..55cf6f01d0c 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -13,7 +13,7 @@ struct repository;\n struct string_list;\n struct strbuf;\n \n-enum {\n+enum submodule_recurse_mode {\n \tRECURSE_SUBMODULES_ONLY = -5,\n \tRECURSE_SUBMODULES_CHECK = -4,\n \tRECURSE_SUBMODULES_ERROR = -3,\n\nbase-commit: 2ae0a9cb8298185a94e5998086f380a355dd8907\n-- \ngitgitgadget\n"},{"id":"445582","messageId":"xmqq1r1lhobf.fsf@gitster.g","threadId":"57194","inReplyTo":"pull.1111.git.1641410782015.gitgitgadget@gmail.com","subject":"Re: [PATCH] submodule.h: use a named enum for RECURSE_SUBMODULES_*","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-05T21:20:36Z","receivedAt":"2022-01-05T21:20:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Philippe Blain via GitGitGadget\" writes:\n\n> From: Philippe Blain \n>\n> Using a named enum allows casting an integer to the enum type in both\n> GDB and LLDB:\n>\n> (gdb) p (enum diff_submodule_format) options->submodule_format\n> $1 = DIFF_SUBMODULE_LOG\n\nHmph, this was somewhat unexpected and quite disappointing.\n\nBecause that's not what those \"Let's move away from #define'd\nconstants and use more enums\" said when they sold their \"enum\" to\nus. In the diff_options struct, the .submodule_format member is of\ntype enum already, so, if we trust what they told us when they sold\ntheir enums, \"p options->submodule_format\" should be enough to give\nus \"DIFF_SUBMODULE_LOG\", not \"1\", as its output. Do you really need\nthe cast in the above example?\n\n> Name the enum listing the different RECURSE_SUBMODULES_* modes, to make\n> debugging easier.\n\nEven though this patch may be a good single step in the right\ndirection, until it is _used_ to declare a variable or a member of a\nstruct of that enum type, it probably is not useful as much as it\ncould be. The user needs to know which enum is stored in that \"int\"\nvariable or member the user is interested in to cast it to.\n\nThat is, many changes like this one.\n\ndiff --git i/builtin/pull.c w/builtin/pull.c\nindex c8457619d8..f2fd4784df 100644\n--- i/builtin/pull.c\n+++ w/builtin/pull.c\n@@ -71,7 +71,7 @@ static const char * const pull_usage[] = {\n /* Shared options */\n static int opt_verbosity;\n static char *opt_progress;\n-static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n+static enum submodule_recurse_mode recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n \n /* Options passed to git-merge or git-rebase */\n static enum rebase_type opt_rebase = -1;\n"},{"id":"445719","messageId":"3160705c-d95c-c389-e929-f1f6bbcd80fc@gmail.com","threadId":"57194","inReplyTo":"xmqq1r1lhobf.fsf@gitster.g","subject":"Re: [PATCH] submodule.h: use a named enum for RECURSE_SUBMODULES_*","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2022-01-07T13:27:00Z","receivedAt":"2022-01-07T13:27:04Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Junio,\n\nLe 2022-01-05 à 16:20, Junio C Hamano a écrit :\n> \"Philippe Blain via GitGitGadget\" writes:\n> \n>> From: Philippe Blain \n>>\n>> Using a named enum allows casting an integer to the enum type in both\n>> GDB and LLDB:\n>>\n>> (gdb) p (enum diff_submodule_format) options->submodule_format\n>> $1 = DIFF_SUBMODULE_LOG\n> \n> Hmph, this was somewhat unexpected and quite disappointing.\n> \n> Because that's not what those \"Let's move away from #define'd\n> constants and use more enums\" said when they sold their \"enum\" to\n> us. In the diff_options struct, the .submodule_format member is of\n> type enum already, so, if we trust what they told us when they sold\n> their enums, \"p options->submodule_format\" should be enough to give\n> us \"DIFF_SUBMODULE_LOG\", not \"1\", as its output. Do you really need\n> the cast in the above example?\n\nYes, you are right that my example does not reflect what I'm saying, since\noptions->submodule_format is not an int. I checked and indeed we do not\nneed any cast to get \"DIFF_SUBMODULE_LOG\". We do need it when dealing with int's,\nwhich is not the case here. I'll try to find a better example.\n\n> \n>> Name the enum listing the different RECURSE_SUBMODULES_* modes, to make\n>> debugging easier.\n> \n> Even though this patch may be a good single step in the right\n> direction, until it is _used_ to declare a variable or a member of a\n> struct of that enum type, it probably is not useful as much as it\n> could be. The user needs to know which enum is stored in that \"int\"\n> variable or member the user is interested in to cast it to.\n\nYes, that's true. But when I came across that, I was in a place of the\ncode where some int was compared with a constant in this enum,\nRECURSE_SUBMODULES_something. So it would have been easy to check where\nthe enum is declared, learn its name and use it to cast the int to the enum\ntype. That's the kind of situation I have in mind.\n\n> \n> That is, many changes like this one.\n> \n> diff --git i/builtin/pull.c w/builtin/pull.c\n> index c8457619d8..f2fd4784df 100644\n> --- i/builtin/pull.c\n> +++ w/builtin/pull.c\n> @@ -71,7 +71,7 @@ static const char * const pull_usage[] = {\n> /* Shared options */\n> static int opt_verbosity;\n> static char *opt_progress;\n> -static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n> +static enum submodule_recurse_mode recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n> \n> /* Options passed to git-merge or git-rebase */\n> static enum rebase_type opt_rebase = -1;\n> \n\nYes, this is a parallel effort that could be done, I agree, but my patch\nwas meant to help in the mean time.\n\nThanks,\n\nPhilippe.\n"},{"id":"445727","messageId":"kl6lczl3786k.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57194","inReplyTo":"3160705c-d95c-c389-e929-f1f6bbcd80fc@gmail.com","subject":"Re: [PATCH] submodule.h: use a named enum for RECURSE_SUBMODULES_*","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-01-07T17:43:47Z","receivedAt":"2022-01-07T17:43:54Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Philippe Blain writes:\n\n>> \n>> That is, many changes like this one.\n>> \n>> diff --git i/builtin/pull.c w/builtin/pull.c\n>> index c8457619d8..f2fd4784df 100644\n>> --- i/builtin/pull.c\n>> +++ w/builtin/pull.c\n>> @@ -71,7 +71,7 @@ static const char * const pull_usage[] = {\n>> /* Shared options */\n>> static int opt_verbosity;\n>> static char *opt_progress;\n>> -static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n>> +static enum submodule_recurse_mode recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n>> \n>> /* Options passed to git-merge or git-rebase */\n>> static enum rebase_type opt_rebase = -1;\n>> \n>\n> Yes, this is a parallel effort that could be done, I agree, but my patch\n> was meant to help in the mean time.\n\nThere are quite a few sites that could use this\ns/int/enum submodule_recurse_mode change. I suppose one _could_ change\nall of them at once, but that seems cumbersome to review and prone to\nconflict.\n\nSo that this isn't debugger-only, I'd be happy with at least one change\n(perhaps the one that inspired you to name the enum in the first place),\nand making the other changes when it makes sense, e.g. I can do this for\nthe fetch machinery while I work on enhancements to `fetch\n--recurse-submodules`.\n"},{"id":"446439","messageId":"kl6l5yqgzz10.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57194","inReplyTo":"xmqq1r1lhobf.fsf@gitster.g","subject":"Re: [PATCH] submodule.h: use a named enum for RECURSE_SUBMODULES_*","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-01-18T18:20:27Z","receivedAt":"2022-01-18T18:20:35Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Junio C Hamano writes:\n\n> \"Philippe Blain via GitGitGadget\" writes:\n>\n>> From: Philippe Blain \n>>\n>> Using a named enum allows casting an integer to the enum type in both\n>> GDB and LLDB:\n>>\n>> (gdb) p (enum diff_submodule_format) options->submodule_format\n>> $1 = DIFF_SUBMODULE_LOG\n>\n> Hmph, this was somewhat unexpected and quite disappointing.\n>\n> Because that's not what those \"Let's move away from #define'd\n> constants and use more enums\" said when they sold their \"enum\" to\n> us. In the diff_options struct, the .submodule_format member is of\n> type enum already, so, if we trust what they told us when they sold\n> their enums, \"p options->submodule_format\" should be enough to give\n> us \"DIFF_SUBMODULE_LOG\", not \"1\", as its output. Do you really need\n> the cast in the above example?\n>\n>> Name the enum listing the different RECURSE_SUBMODULES_* modes, to make\n>> debugging easier.\n>\n> Even though this patch may be a good single step in the right\n> direction, until it is _used_ to declare a variable or a member of a\n> struct of that enum type, it probably is not useful as much as it\n> could be. The user needs to know which enum is stored in that \"int\"\n> variable or member the user is interested in to cast it to.\n>\n> That is, many changes like this one.\n>\n> diff --git i/builtin/pull.c w/builtin/pull.c\n> index c8457619d8..f2fd4784df 100644\n> --- i/builtin/pull.c\n> +++ w/builtin/pull.c\n> @@ -71,7 +71,7 @@ static const char * const pull_usage[] = {\n> /* Shared options */\n> static int opt_verbosity;\n> static char *opt_progress;\n> -static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n> +static enum submodule_recurse_mode recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n> \n> /* Options passed to git-merge or git-rebase */\n> static enum rebase_type opt_rebase = -1;\n\nYour comment on the use of RECURSE_SUBMODULES_DEFAULT elsewhere [1]\nreminded me of how odd this enum actually is.\n\n* submodule_recurse_mode is used almost exclusively by fetch and push \n because they are the only commands that accept anything other than\n true/false.\n* however, it is also used by\n submodule.c:config_update_recurse_submodules to store true/false (it\n don't even use RECURSE_SUBMODULES_DEFAULT!)\n\ni.e. I suspect that the enum is only relevant for fetch/push, but its\nvalues for _ON and _OFF have been co-opted for other things.\n\nThis patch and the suggestion of s/int/enum submodule_recurse_mode makes\nsense, though I think we can take this a bit further in some follow-up\nwork:\n\nIf I am correct in my suspicion earlier, then submodule_recurse_mode can\nbe made even more specific, like \"submodule_transport_mode\", and all\nnon-transport related uses can be replaced with int.\n\nIf I am wrong and there are some legitimate uses for\nsubmodule_recurse_mode that I have missed, it might still be worthwhile\nto separate the different uses of the enum so that it doesn't end up\noverloaded.\n\n[1] https://lore.kernel.org/git/xmqqr19cjuzw.fsf@gitster.g\n"},{"id":"453041","messageId":"kl6lv8voybvh.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57194","inReplyTo":"pull.1111.v2.git.1649092211419.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] submodule.h: use a named enum for RECURSE_SUBMODULES_*","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-04-04T17:52:34Z","receivedAt":"2022-04-04T21:18:56Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"\"Philippe Blain via GitGitGadget\" writes:\n\n> From: Philippe Blain \n>\n> Using a named enum allows casting an integer to the enum type in both\n> GDB and LLDB:\n>\n> $ gdb -q -ex 'b wt-status.c:44' -ex r --args ./git status\n> (gdb) p (enum color_wt_status) slot\n> $1 = WT_STATUS_ONBRANCH\n>\n> $ lldb -o 'b wt-status.c:44' -o r -- ./git status\n> (lldb) p (color_wt_status) slot\n> (color_wt_status) $0 = WT_STATUS_ONBRANCH\n>\n> In LLDB, it's also required to cast in the reversed direction, i.e.\n> cast an enum constant into its corresponding integer:\n>\n> (lldb) p (int) color_wt_status::WT_STATUS_ONBRANCH\n> (int) $1 = 8\n>\n> Name the enum listing the different RECURSE_SUBMODULES_* modes, to make\n> debugging easier. For example, when stepping through a part of the code\n> where an int is compared with a constant in this enum, it allows casting\n> the int to the enum type or vice-versa, after quickly checking where the\n> enum constant is declared and learning the enum name.\n\nMakes sense, and besides just making debugging easier, this would also\nmake code easier to read once we use the enum in more places (instead of\njust int).\n\n> As to not make this patch a debug-only change, convert the\n> 'fetch_recurse' member of 'struct submodule' to use the newly named\n> enum.\n\n[...]\n\n> submodule-config.h | 2 +-\n> submodule.h | 2 +-\n> 2 files changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/submodule-config.h b/submodule-config.h\n> index 65875b94ea5..55a4c3e0bd5 100644\n> --- a/submodule-config.h\n> +++ b/submodule-config.h\n> @@ -37,7 +37,7 @@ struct submodule {\n> \tconst char *path;\n> \tconst char *name;\n> \tconst char *url;\n> -\tint fetch_recurse;\n> +\tenum submodule_recurse_mode fetch_recurse;\n> \tconst char *ignore;\n> \tconst char *branch;\n> \tstruct submodule_update_strategy update_strategy;\n> diff --git a/submodule.h b/submodule.h\n> index 6bd2c99fd99..55cf6f01d0c 100644\n> --- a/submodule.h\n> +++ b/submodule.h\n> @@ -13,7 +13,7 @@ struct repository;\n> struct string_list;\n> struct strbuf;\n> \n> -enum {\n> +enum submodule_recurse_mode {\n> \tRECURSE_SUBMODULES_ONLY = -5,\n> \tRECURSE_SUBMODULES_CHECK = -4,\n> \tRECURSE_SUBMODULES_ERROR = -3,\n\nLike Junio, I think it would be nice to use the enum in more places, but\nfrankly there are so many sites that would need to change that I don't\nthink it's reasonable for one person to change them all.\n\nSo I'm happy to take this first step and do the rest of the refactoring\nincrementally. I still think the enum's values are too disjointed and\nshould eventually be broken up, but that's a refactoring for later.\n\nReviewed-by: Glen Choo \n"},{"id":"453056","messageId":"pull.1111.v2.git.1649092211419.gitgitgadget@gmail.com","threadId":"57194","inReplyTo":"pull.1111.git.1641410782015.gitgitgadget@gmail.com","subject":"[PATCH v2] submodule.h: use a named enum for RECURSE_SUBMODULES_*","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-04-04T17:10:11Z","receivedAt":"2022-04-04T21:23:48Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain \n\nUsing a named enum allows casting an integer to the enum type in both\nGDB and LLDB:\n\n $ gdb -q -ex 'b wt-status.c:44' -ex r --args ./git status\n (gdb) p (enum color_wt_status) slot\n $1 = WT_STATUS_ONBRANCH\n\n $ lldb -o 'b wt-status.c:44' -o r -- ./git status\n (lldb) p (color_wt_status) slot\n (color_wt_status) $0 = WT_STATUS_ONBRANCH\n\nIn LLDB, it's also required to cast in the reversed direction, i.e.\ncast an enum constant into its corresponding integer:\n\n (lldb) p (int) color_wt_status::WT_STATUS_ONBRANCH\n (int) $1 = 8\n\nName the enum listing the different RECURSE_SUBMODULES_* modes, to make\ndebugging easier. For example, when stepping through a part of the code\nwhere an int is compared with a constant in this enum, it allows casting\nthe int to the enum type or vice-versa, after quickly checking where the\nenum constant is declared and learning the enum name.\n\nAs to not make this patch a debug-only change, convert the\n'fetch_recurse' member of 'struct submodule' to use the newly named\nenum.\n\nSigned-off-by: Philippe Blain \n---\n submodule.h: use a named enum for RECURSE_SUBMODULES_*\n \n Changes since v1:\n \n * converted one user of the enum from an 'int' to the new enum type, so\n the patch is not debug-only, as suggested by Glen.\n * updated the commit message to use an 'int' as example of a local\n variable being compared with an enum constant, instead of using a\n struct member which is already of enum type, as pointed out by Junio\n * added a little bit more explanation in the commit message as to how\n this patch can help when debugging.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1111%2Fphil-blain%2Fsubmodule-recurse-enum-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1111/phil-blain/submodule-recurse-enum-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1111\n\nRange-diff vs v1:\n\n 1: 4c787d4054e ! 1: e0b211b88f5 submodule.h: use a named enum for RECURSE_SUBMODULES_*\n @@ Commit message\n Using a named enum allows casting an integer to the enum type in both\n GDB and LLDB:\n \n - (gdb) p (enum diff_submodule_format) options->submodule_format\n - $1 = DIFF_SUBMODULE_LOG\n + $ gdb -q -ex 'b wt-status.c:44' -ex r --args ./git status\n + (gdb) p (enum color_wt_status) slot\n + $1 = WT_STATUS_ONBRANCH\n \n - (lldb) p (diff_submodule_format) options->submodule_format\n - (diff_submodule_format) $1 = DIFF_SUBMODULE_LOG\n + $ lldb -o 'b wt-status.c:44' -o r -- ./git status\n + (lldb) p (color_wt_status) slot\n + (color_wt_status) $0 = WT_STATUS_ONBRANCH\n \n In LLDB, it's also required to cast in the reversed direction, i.e.\n cast an enum constant into its corresponding integer:\n \n - (lldb) p (int) diff_submodule_format::DIFF_SUBMODULE_SHORT\n - (int) $0 = 0\n + (lldb) p (int) color_wt_status::WT_STATUS_ONBRANCH\n + (int) $1 = 8\n \n Name the enum listing the different RECURSE_SUBMODULES_* modes, to make\n - debugging easier.\n + debugging easier. For example, when stepping through a part of the code\n + where an int is compared with a constant in this enum, it allows casting\n + the int to the enum type or vice-versa, after quickly checking where the\n + enum constant is declared and learning the enum name.\n +\n + As to not make this patch a debug-only change, convert the\n + 'fetch_recurse' member of 'struct submodule' to use the newly named\n + enum.\n \n Signed-off-by: Philippe Blain \n \n + ## submodule-config.h ##\n +@@ submodule-config.h: struct submodule {\n + \tconst char *path;\n + \tconst char *name;\n + \tconst char *url;\n +-\tint fetch_recurse;\n ++\tenum submodule_recurse_mode fetch_recurse;\n + \tconst char *ignore;\n + \tconst char *branch;\n + \tstruct submodule_update_strategy update_strategy;\n +\n ## submodule.h ##\n @@ submodule.h: struct repository;\n struct string_list;\n\n\n submodule-config.h | 2 +-\n submodule.h | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/submodule-config.h b/submodule-config.h\nindex 65875b94ea5..55a4c3e0bd5 100644\n--- a/submodule-config.h\n+++ b/submodule-config.h\n@@ -37,7 +37,7 @@ struct submodule {\n \tconst char *path;\n \tconst char *name;\n \tconst char *url;\n-\tint fetch_recurse;\n+\tenum submodule_recurse_mode fetch_recurse;\n \tconst char *ignore;\n \tconst char *branch;\n \tstruct submodule_update_strategy update_strategy;\ndiff --git a/submodule.h b/submodule.h\nindex 6bd2c99fd99..55cf6f01d0c 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -13,7 +13,7 @@ struct repository;\n struct string_list;\n struct strbuf;\n \n-enum {\n+enum submodule_recurse_mode {\n \tRECURSE_SUBMODULES_ONLY = -5,\n \tRECURSE_SUBMODULES_CHECK = -4,\n \tRECURSE_SUBMODULES_ERROR = -3,\n\nbase-commit: 2ae0a9cb8298185a94e5998086f380a355dd8907\n-- \ngitgitgadget\n"}]}