{"thread":{"id":"51437","subject":"[PATCH v1] merge - rename a shadowed variable in cmd_merge","startedAt":"2019-07-05T20:32:41Z","lastAt":"2019-07-08T20:02:26Z","messageCount":2,"participants":["Edmundo Carmona Antoranz","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"378654","messageId":"20190705203227.23451-1-eantoranz@gmail.com","threadId":"51437","inReplyTo":null,"subject":"[PATCH v1] merge - rename a shadowed variable in cmd_merge","fromName":"Edmundo Carmona Antoranz","fromEmail":"eantoranz@gmail.com","sentAt":"2019-07-05T20:32:27Z","receivedAt":"2019-07-05T20:32:41Z","isPatch":true,"sender":{"key":"eantoranz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1491018?v=4"},"body":"variable ret used in cmd_merge introduced in d5a35c114ab was already\na local variable used inside a for loop inside the function.\n\nfor-local variable is being renamed to ret_try_merge to avoid shadow.\n---\n builtin/merge.c | 16 ++++++++--------\n 1 file changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 6e99aead46..972b6c376a 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1587,7 +1587,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\toidclr(&stash);\n \n \tfor (i = 0; i < use_strategies_nr; i++) {\n-\t\tint ret;\n+\t\tint ret_try_merge;\n \t\tif (i) {\n \t\t\tprintf(_(\"Rewinding the tree to pristine...\\n\"));\n \t\t\trestore_state(&head_commit->object.oid, &stash);\n@@ -1601,26 +1601,26 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t */\n \t\twt_strategy = use_strategies[i]->name;\n \n-\t\tret = try_merge_strategy(use_strategies[i]->name,\n-\t\t\t\t\t common, remoteheads,\n-\t\t\t\t\t head_commit);\n-\t\tif (!option_commit && !ret) {\n+\t\tret_try_merge = try_merge_strategy(use_strategies[i]->name,\n+\t\t\t\t\t\tcommon, remoteheads,\n+\t\t\t\t\t\thead_commit);\n+\t\tif (!option_commit && !ret_try_merge) {\n \t\t\tmerge_was_ok = 1;\n \t\t\t/*\n \t\t\t * This is necessary here just to avoid writing\n \t\t\t * the tree, but later we will *not* exit with\n \t\t\t * status code 1 because merge_was_ok is set.\n \t\t\t */\n-\t\t\tret = 1;\n+\t\t\tret_try_merge = 1;\n \t\t}\n \n-\t\tif (ret) {\n+\t\tif (ret_try_merge) {\n \t\t\t/*\n \t\t\t * The backend exits with 1 when conflicts are\n \t\t\t * left to be resolved, with 2 when it does not\n \t\t\t * handle the given merge at all.\n \t\t\t */\n-\t\t\tif (ret == 1) {\n+\t\t\tif (ret_try_merge == 1) {\n \t\t\t\tint cnt = evaluate_result();\n \n \t\t\t\tif (best_cnt <= 0 || cnt <= best_cnt) {\n-- \n2.22.0.214.g8dca754b1e\n\n"},{"id":"378703","messageId":"xmqqlfx8xpko.fsf@gitster-ct.c.googlers.com","threadId":"51437","inReplyTo":"20190705203227.23451-1-eantoranz@gmail.com","subject":"Re: [PATCH v1] merge - rename a shadowed variable in cmd_merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-08T20:02:15Z","receivedAt":"2019-07-08T20:02:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Edmundo Carmona Antoranz <eantoranz@gmail.com> writes:\n\n> variable ret used in cmd_merge introduced in d5a35c114ab was already\n> a local variable used inside a for loop inside the function.\n\nStrictly speaking, there was a local variable 'ret' inside for loop,\nwhich is unrelated to the variable introduced by the said commit.\nThe only resemblance was that they happen to share the same name.\nSo \"was already a local variable\" is not quite right, and made my\nreading hiccup.\n\n> for-local variable is being renamed to ret_try_merge to avoid shadow.\n\nIs this really a problem that needs to be changed?  What compiler\nis having trouble with the code?\n\nI am reasonably negative on this change.  But as you seem to be a\nnew contributor, let me grab this opportunity to comment on other\naspects of the patch.\n\n> ---\n\nMissing sign-off.\n\nIn the proposed commit log message body, write full sentences just\nlike normal English, e.g. a sentence begins with a capital letter,\netc.\n\nThe usual pattern used in our log messages is first to give an\nobservation of the current state and state what the problem is,\nand then write orders you give to the codebase to be like so to fix\nthe problem, e.g.\n\n\tThe commit d5a35c11 (\"Copy resolve_ref() return value for\n\tlonger use\", 2011-11-13) introduced a variable 'ret' to\n\tcmd_merge() to keep the final return value from the\n\tfunction.  There however was an unrelated variable that is\n\tlocal to a for loop that shared the same name.  Because the\n\tstatements inside of the loop do not have enough information\n\tto decide the final outcome of the function, there is no\n\tneed for the outer 'ret' to be visible to them, which is\n\ta perfectly good reason to use the \"shadowing\" technique.\n\n\tRename the local variable used inside the for loop to avoid\n\twarnings when compiled with -Wshadow; this will expose the\n\touter 'ret' to the statements in the loop, allowing them to\n\tmistakenly making an assignment to it, though.\n\nis how I would describe this change.  As you can see, this trades\n\"make -Wshadow less noisy\" with \"make it easier to make mistakes\"\nand I am not sure if it is a good trade-off.\n\n> diff --git a/builtin/merge.c b/builtin/merge.c\n> index 6e99aead46..972b6c376a 100644\n> --- a/builtin/merge.c\n> +++ b/builtin/merge.c\n> @@ -1587,7 +1587,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n>  \t\toidclr(&stash);\n\nInteresting.\n\nAll assignments to ret up to this point are all followed by \"goto\ndone\" to jump over the \"for\" loop we are looking at this patch.\nSo we know that when the control reaches at this point, ret has its\ninitial value 0.\n\nSo an alternative approach would be to just ...\n\n>  \tfor (i = 0; i < use_strategies_nr; i++) {\n> -\t\tint ret;\n> +\t\tint ret_try_merge;\n\n... drop this local variable declaration and let it contaminate the\nouter 'ret', and then after the loop is done, assign 0 to ret.  That\nwould squelch \"-Wshallow\" and at the same time makes sure that the\nloop won't corrupt the \"proposed final outcome\" stored in 'ret'.\n\nQuite honestly, I think the easiest \"solution\" would be not to use\n\"-Wshadow\" in your compilation.  Thsi file has a handful other\ninstances of variable shadowing, and most of them do not look\nconfusing or problematic.\n"}]}