{"thread":{"id":"58571","subject":"[PATCH 0/2] Minor Refactors: remove useless else","startedAt":"2022-10-06T11:45:56Z","lastAt":"2022-10-08T16:21:43Z","messageCount":15,"participants":["dsal3389 via GitGitGadget","Victoria Dye","Junio C Hamano","Daniel Sonbolian via GitGitGadget","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"464280","messageId":"pull.1355.git.git.1665056747.gitgitgadget@gmail.com","threadId":"58571","inReplyTo":null,"subject":"[PATCH 0/2] Minor Refactors: remove useless else","fromName":"dsal3389 via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-06T11:45:45Z","receivedAt":"2022-10-06T11:45:56Z","isPatch":true,"sender":{"key":"name:dsal3389","avatar":null},"body":"git.c removed redundant else statement - it is redundant because all we\nactually need is to check if there are no arguments, the program print\nhelpful information to the screen and exit, we don't need the else for that\n\ngit-p4.py L371 - remove redundant else statement - same as git.c (made sure\nnot to repeat what was already done on #1349\n[https://github.com/git/git/pull/1349]) git-p4.py L404 - minor, there is no\nneed to decode the output and iterate over the variable if we may exit\n\ndsal3389 (2):\n python file more pytonic, adjust \"if\" and \"for\"\n removed else statement\n\n git-p4.py | 9 ++++-----\n git.c | 14 ++++++++------\n 2 files changed, 12 insertions(+), 11 deletions(-)\n\n\nbase-commit: bcd6bc478adc4951d57ec597c44b12ee74bc88fb\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1355%2Fdsal3389%2Frm-useless-else-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1355/dsal3389/rm-useless-else-v1\nPull-Request: https://github.com/git/git/pull/1355\n-- \ngitgitgadget\n"},{"id":"464281","messageId":"71da6f53a44cd3390d122ff2c0446824313e5101.1665056747.git.gitgitgadget@gmail.com","threadId":"58571","inReplyTo":"pull.1355.git.git.1665056747.gitgitgadget@gmail.com","subject":"[PATCH 1/2] python file more pytonic, adjust \"if\" and \"for\"","fromName":"dsal3389 via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-06T11:45:46Z","receivedAt":"2022-10-06T11:45:57Z","isPatch":true,"sender":{"key":"name:dsal3389","avatar":null},"body":"From: dsal3389 \n\nL371\nredesign few lines to get rid of the \"else\" statement\n\nL404\nmoved the if statement below another if statement that\nchecks if it should exit the code, only if it doesnt need to,\nthen we can iterate the for loop and decode the text\n\nChanges to be committed:\n\tmodified: git-p4.py\n\nSigned-off-by: Daniel Sonbolian \n---\n git-p4.py | 9 ++++-----\n 1 file changed, 4 insertions(+), 5 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex d26a980e5ac..0ba5115fa2e 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -368,10 +368,9 @@ def read_pipe(c, ignore_error=False, raw=False, *k, **kw):\n \"\"\"\n retcode, out, err = read_pipe_full(c, *k, **kw)\n if retcode != 0:\n- if ignore_error:\n- out = \"\"\n- else:\n+ if not ignore_error:\n die('Command failed: {}\\nError: {}'.format(' '.join(c), err))\n+ out = \"\"\n if not raw:\n out = decode_text_stream(out)\n return out\n@@ -400,10 +399,10 @@ def read_pipe_lines(c, raw=False, *k, **kw):\n p = subprocess.Popen(c, stdout=subprocess.PIPE, *k, **kw)\n pipe = p.stdout\n lines = pipe.readlines()\n- if not raw:\n- lines = [decode_text_stream(line) for line in lines]\n if pipe.close() or p.wait():\n die('Command failed: {}'.format(' '.join(c)))\n+ if not raw:\n+ lines = [decode_text_stream(line) for line in lines]\n return lines\n \n \n-- \ngitgitgadget\n\n"},{"id":"464282","messageId":"c107ad9f6ff2d5e00134eb1348f24737774edbac.1665056747.git.gitgitgadget@gmail.com","threadId":"58571","inReplyTo":"pull.1355.git.git.1665056747.gitgitgadget@gmail.com","subject":"[PATCH 2/2] removed else statement","fromName":"dsal3389 via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-06T11:45:47Z","receivedAt":"2022-10-06T11:46:01Z","isPatch":true,"sender":{"key":"name:dsal3389","avatar":null},"body":"From: dsal3389 \n\nthere is no need for the else statement if we can do it more\nelegantly with a signle if statement we no \"else\"\n\nSigned-off-by: Daniel Sonbolian \n---\n git.c | 14 ++++++++------\n 1 file changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex da411c53822..340ec8bcb31 100644\n--- a/git.c\n+++ b/git.c\n@@ -894,12 +894,8 @@ int cmd_main(int argc, const char **argv)\n \targv++;\n \targc--;\n \thandle_options(&argv, &argc, NULL);\n-\tif (argc > 0) {\n-\t\tif (!strcmp(\"--version\", argv[0]) || !strcmp(\"-v\", argv[0]))\n-\t\t\targv[0] = \"version\";\n-\t\telse if (!strcmp(\"--help\", argv[0]) || !strcmp(\"-h\", argv[0]))\n-\t\t\targv[0] = \"help\";\n-\t} else {\n+\n+\tif (argc <= 0) {\n \t\t/* The user didn't specify a command; give them help */\n \t\tcommit_pager_choice();\n \t\tprintf(_(\"usage: %s\\n\\n\"), git_usage_string);\n@@ -907,6 +903,12 @@ int cmd_main(int argc, const char **argv)\n \t\tprintf(\"\\n%s\\n\", _(git_more_info_string));\n \t\texit(1);\n \t}\n+\n+\tif (!strcmp(\"--version\", argv[0]) || !strcmp(\"-v\", argv[0]))\n+\t\targv[0] = \"version\";\n+\telse if (!strcmp(\"--help\", argv[0]) || !strcmp(\"-h\", argv[0]))\n+\t\targv[0] = \"help\";\n+\n \tcmd = argv[0];\n \n \t/*\n-- \ngitgitgadget\n"},{"id":"464302","messageId":"66d1bcaf-64c8-13c2-ba7a-98715de3617b@github.com","threadId":"58571","inReplyTo":"71da6f53a44cd3390d122ff2c0446824313e5101.1665056747.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] python file more pytonic, adjust \"if\" and \"for\"","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2022-10-06T16:02:51Z","receivedAt":"2022-10-06T16:02:57Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"dsal3389 via GitGitGadget wrote:\n> From: dsal3389 \n\nre: the commit message title\n\ns/pytonic/pythonic\n\nPlease prefix the title with the file/function being updated. Based on\nprecedent [1], that'd probably be:\n\n git-p4: \n\nAlso, I'm not sure the reference to \"for\" is helpful, since you're only\nadjusting if-statements in this patch (although one of those conditionals\ndoes include a for-loop, nothing about the loop itself changes).\n\nFinally, please use the imperative mood in your commit messages [2];\nsomething like:\n\n git-p4: adjust \"if\" statements to be more pythonic \n\n[1] https://lore.kernel.org/git/?q=dfn%3Agit-p4.py\n[2] https://git-scm.com/docs/SubmittingPatches#describe-changes\n\n> \n> L371\n> redesign few lines to get rid of the \"else\" statement\n\nWhat was your reason for making this change? I don't see anything in PEP8\n[3] regarding this kind of conditional organization being preferred over\nwhat's already there.\n\nFor what it's worth, I'm happy with the change itself; I'd just ask that\nyou include an explanation for why (because it's more concise, or makes the\nassignment to 'out' clearer, etc.). The \"Describe your changes well\" section\nof the \"SubmittingPatches\" document [2] should be a helpful guideline.\n\nAlso, as a matter of convention, it's not necessary to include line numbers.\n\n[2] https://git-scm.com/docs/SubmittingPatches#describe-changes\n[3] https://peps.python.org/pep-0008/\n\n> \n> L404\n> moved the if statement below another if statement that\n> checks if it should exit the code, only if it doesnt need to,\n> then we can iterate the for loop and decode the text\n\nWhile the actual change below makes sense, I found this explanation very\ndifficult to parse. Maybe something like:\n\n Reorder if-statements in 'read_pipe_lines()' to raise an error with the \n command before trying to decode its results in order to avoid \n unnecessary computation.\n\n> \n> Changes to be committed:\n> \tmodified: git-p4.py\n\nPlease remove this text from the commit message.\n\n> \n> Signed-off-by: Daniel Sonbolian \n> ---\n> git-p4.py | 9 ++++-----\n> 1 file changed, 4 insertions(+), 5 deletions(-)\n\nThe rest of the implementation looks good, thanks!\n\n> \n> diff --git a/git-p4.py b/git-p4.py\n> index d26a980e5ac..0ba5115fa2e 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -368,10 +368,9 @@ def read_pipe(c, ignore_error=False, raw=False, *k, **kw):\n> \"\"\"\n> retcode, out, err = read_pipe_full(c, *k, **kw)\n> if retcode != 0:\n> - if ignore_error:\n> - out = \"\"\n> - else:\n> + if not ignore_error:\n> die('Command failed: {}\\nError: {}'.format(' '.join(c), err))\n> + out = \"\"\n> if not raw:\n> out = decode_text_stream(out)\n> return out\n> @@ -400,10 +399,10 @@ def read_pipe_lines(c, raw=False, *k, **kw):\n> p = subprocess.Popen(c, stdout=subprocess.PIPE, *k, **kw)\n> pipe = p.stdout\n> lines = pipe.readlines()\n> - if not raw:\n> - lines = [decode_text_stream(line) for line in lines]\n> if pipe.close() or p.wait():\n> die('Command failed: {}'.format(' '.join(c)))\n> + if not raw:\n> + lines = [decode_text_stream(line) for line in lines]\n> return lines\n> \n> \n\n"},{"id":"464305","messageId":"44d2f602-a299-3bd0-1624-cfdddb6dcda7@github.com","threadId":"58571","inReplyTo":"c107ad9f6ff2d5e00134eb1348f24737774edbac.1665056747.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] removed else statement","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2022-10-06T16:14:10Z","receivedAt":"2022-10-06T16:14:16Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"dsal3389 via GitGitGadget wrote:\n> From: dsal3389 \n> \n> there is no need for the else statement if we can do it more\n> elegantly with a signle if statement we no \"else\"\n\nSimilar recommendations on the commit message as in the previous patch [1]:\n\n- title should be prefixed with 'git.c:'\n- title & message should use the imperative mood (e.g. \"remove else\n statement\" instead of \"removed else statement\")\n- please fix typos\n - s/there/There\n - s/signle/single\n - s/we/with(?)\n\n[1] https://lore.kernel.org/git/66d1bcaf-64c8-13c2-ba7a-98715de3617b@github.com/\n\n> \n> Signed-off-by: Daniel Sonbolian \n> ---\n> git.c | 14 ++++++++------\n> 1 file changed, 8 insertions(+), 6 deletions(-)\n> \n> diff --git a/git.c b/git.c\n> index da411c53822..340ec8bcb31 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -894,12 +894,8 @@ int cmd_main(int argc, const char **argv)\n> \targv++;\n> \targc--;\n> \thandle_options(&argv, &argc, NULL);\n> -\tif (argc > 0) {\n> -\t\tif (!strcmp(\"--version\", argv[0]) || !strcmp(\"-v\", argv[0]))\n> -\t\t\targv[0] = \"version\";\n> -\t\telse if (!strcmp(\"--help\", argv[0]) || !strcmp(\"-h\", argv[0]))\n> -\t\t\targv[0] = \"help\";\n> -\t} else {\n> +\n> +\tif (argc <= 0) {\n\nnit: argc is always >= 0 [2], so a more appropriate condition would be:\n\n if (!argc)\n\nThere are lots of examples of that '!argc' conditional in Git, but none of\nthe 'argc <= 0' pattern, so it's probably best to match convention here.\n\nOtherwise, the rest of this diff looks good.\n\n[2] https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf, ยง5.1.2.2.1 #2\n\n> \t\t/* The user didn't specify a command; give them help */\n> \t\tcommit_pager_choice();\n> \t\tprintf(_(\"usage: %s\\n\\n\"), git_usage_string);\n> @@ -907,6 +903,12 @@ int cmd_main(int argc, const char **argv)\n> \t\tprintf(\"\\n%s\\n\", _(git_more_info_string));\n> \t\texit(1);\n> \t}\n> +\n> +\tif (!strcmp(\"--version\", argv[0]) || !strcmp(\"-v\", argv[0]))\n> +\t\targv[0] = \"version\";\n> +\telse if (!strcmp(\"--help\", argv[0]) || !strcmp(\"-h\", argv[0]))\n> +\t\targv[0] = \"help\";\n> +\n> \tcmd = argv[0];\n> \n> \t/*\n\n"},{"id":"464311","messageId":"xmqqmta8lw4c.fsf@gitster.g","threadId":"58571","inReplyTo":"pull.1355.git.git.1665056747.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/2] Minor Refactors: remove useless else","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-06T17:06:43Z","receivedAt":"2022-10-06T17:06:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"dsal3389 via GitGitGadget\" writes:\n\n> git.c removed redundant else statement - it is redundant because all we\n> actually need is to check if there are no arguments, the program print\n> helpful information to the screen and exit, we don't need the else for that\n>\n> git-p4.py L371 - remove redundant else statement - same as git.c (made sure\n> not to repeat what was already done on #1349\n> [https://github.com/git/git/pull/1349]) git-p4.py L404 - minor, there is no\n> need to decode the output and iterate over the variable if we may exit\n>\n> dsal3389 (2):\n> python file more pytonic, adjust \"if\" and \"for\"\n> removed else statement\n>\n> git-p4.py | 9 ++++-----\n> git.c | 14 ++++++++------\n> 2 files changed, 12 insertions(+), 11 deletions(-)\n\nWelcome to the Git development community.\n\nA few general suggestions.\n\n * Familiarlize yourself with Documentation/CodingGuidelines and\n Documentation/SubmittingPatches.\n\n * See how commits in the project are explained in their log\n messages in \"git shortlog --no-merges -n100\" (for how their\n titles are written to help distinguish them among many other\n commits) and \"git log --no-merges -n100\" (for how they explain\n why making their changes is a good idea and helps the project).\n\n * Make sure the name and the e-mail used as the author of your\n commits match the name and the e-mail on the Signed-off-by: line\n in them.\n\n * Spell check before sending things out to the public list.\n\n"},{"id":"464312","messageId":"xmqqh70glvno.fsf@gitster.g","threadId":"58571","inReplyTo":"71da6f53a44cd3390d122ff2c0446824313e5101.1665056747.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] python file more pytonic, adjust \"if\" and \"for\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-06T17:16:43Z","receivedAt":"2022-10-06T17:16:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"dsal3389 via GitGitGadget\" writes:\n\n> From: dsal3389 \n>\n> L371\n> redesign few lines to get rid of the \"else\" statement\n>\n> L404\n> moved the if statement below another if statement that\n> checks if it should exit the code, only if it doesnt need to,\n> then we can iterate the for loop and decode the text\n>\n> Changes to be committed:\n> \tmodified: git-p4.py\n\nCompare this with the commits by others in \"git log --no-merges\"\noutput of this project.\n\n> Signed-off-by: Daniel Sonbolian \n\nPlease have this on the in-body \"From:\" line we see above. I think\nGitGitGadget takes it from the author of the commit object it sends\nout, so you may have to go back to your branch and fix them with\n\"rebase -i\".\n\n> ---\n> git-p4.py | 9 ++++-----\n> 1 file changed, 4 insertions(+), 5 deletions(-)\n>\n> diff --git a/git-p4.py b/git-p4.py\n> index d26a980e5ac..0ba5115fa2e 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -368,10 +368,9 @@ def read_pipe(c, ignore_error=False, raw=False, *k, **kw):\n> \"\"\"\n> retcode, out, err = read_pipe_full(c, *k, **kw)\n> if retcode != 0:\n> - if ignore_error:\n> - out = \"\"\n> - else:\n> + if not ignore_error:\n> die('Command failed: {}\\nError: {}'.format(' '.join(c), err))\n> + out = \"\"\n\nI think the code with or without the patch is about the same\ncomplexity, but people tend to have harder time understanding logic\nthat involves double negation, so I can imagine that some readers\nmay find the code with the patch harder to understand. In any case,\nthe difference falls into the \"it is minor enough that once it is\nwritten in one way, it is not worth the churn to rewrite it in the\nother way\" category.\n\n> @@ -400,10 +399,10 @@ def read_pipe_lines(c, raw=False, *k, **kw):\n> p = subprocess.Popen(c, stdout=subprocess.PIPE, *k, **kw)\n> pipe = p.stdout\n> lines = pipe.readlines()\n> - if not raw:\n> - lines = [decode_text_stream(line) for line in lines]\n> if pipe.close() or p.wait():\n> die('Command failed: {}'.format(' '.join(c)))\n> + if not raw:\n> + lines = [decode_text_stream(line) for line in lines]\n> return lines\n\nThis is in the same \"the difference is minor enough that once it is\nwritten in one way, it is not worth the churn to rewrite it in the\nother way\" category. Your reasoning might be that massaging of the\nlines is only needed when we do not die() and it is more efficient\nto check and die first, but that is optimizing for the wrong case.\nThe code should not die in its normal operation and there is no\npoint optimizing for an error code path.\n\nOne thing that might deserve benchmarking and optimizing here is if\nwe can do better than reading everything in lines array and holding\nonto the original until decoding the whole thing at once at the end\nof input. If converting each line and appending the result as it is\nread from the pipe turns out to be more efficient, it may be an\noptimization worth considering, as it is optimizaing for the normal\ncase.\n\n"},{"id":"464313","messageId":"xmqqbkqolvnn.fsf@gitster.g","threadId":"58571","inReplyTo":"c107ad9f6ff2d5e00134eb1348f24737774edbac.1665056747.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] removed else statement","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-06T17:16:44Z","receivedAt":"2022-10-06T17:16:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"dsal3389 via GitGitGadget\" writes:\n\n> From: dsal3389 \n>\n> there is no need for the else statement if we can do it more\n> elegantly with a signle if statement we no \"else\"\n\nSome people seem to use subjective words like \"elegantly\" but they\nweaken your justification. You are better off avoiding them.\n\nAs to the change in the patch, I do find it easier to read to have a\ncheck for an error condition whose body unconditionally exits first,\nand then the special casing of \"version\" and \"help\" as part of the\npreparation for the \"normal codepath\".\n\n Side note: yes, you can steal from the last paragraph when you\n are redoing the justfication of this patch.\n\nThanks.\n\n> -\tif (argc > 0) {\n> -\t\tif (!strcmp(\"--version\", argv[0]) || !strcmp(\"-v\", argv[0]))\n> -\t\t\targv[0] = \"version\";\n> -\t\telse if (!strcmp(\"--help\", argv[0]) || !strcmp(\"-h\", argv[0]))\n> -\t\t\targv[0] = \"help\";\n> -\t} else {\n> +\n> +\tif (argc <= 0) {\n> \t\t/* The user didn't specify a command; give them help */\n> \t\tcommit_pager_choice();\n> \t\tprintf(_(\"usage: %s\\n\\n\"), git_usage_string);\n> @@ -907,6 +903,12 @@ int cmd_main(int argc, const char **argv)\n> \t\tprintf(\"\\n%s\\n\", _(git_more_info_string));\n> \t\texit(1);\n> \t}\n> +\n> +\tif (!strcmp(\"--version\", argv[0]) || !strcmp(\"-v\", argv[0]))\n> +\t\targv[0] = \"version\";\n> +\telse if (!strcmp(\"--help\", argv[0]) || !strcmp(\"-h\", argv[0]))\n> +\t\targv[0] = \"help\";\n> +\n> \tcmd = argv[0];\n> \n> \t/*\n"},{"id":"464375","messageId":"pull.1355.v2.git.git.1665153486.gitgitgadget@gmail.com","threadId":"58571","inReplyTo":"pull.1355.git.git.1665056747.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] git.c: improve readability | git-p4: minor optimization","fromName":"Daniel Sonbolian via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-07T14:38:04Z","receivedAt":"2022-10-07T14:38:13Z","isPatch":true,"sender":{"key":"name:Daniel Sonbolian","avatar":null},"body":"git.c - made the code more readable in cmd_main by moving the spacial casing\nfor \"version\" and \"help\" as part of the regular code path\n\ngit-p4.py - minor optimization in read_pipe_lines by first checking for\nerrors, then reading data and/or decoding it from the pip stream\n\nDaniel Sonbolian (2):\n git-p4: minor optimization in read_pip_lines\n git.c: improve code readability in cmd_main\n\n git-p4.py | 10 +++++++---\n git.c | 14 ++++++++------\n 2 files changed, 15 insertions(+), 9 deletions(-)\n\n\nbase-commit: bcd6bc478adc4951d57ec597c44b12ee74bc88fb\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1355%2Fdsal3389%2Frm-useless-else-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1355/dsal3389/rm-useless-else-v2\nPull-Request: https://github.com/git/git/pull/1355\n\nRange-diff vs v1:\n\n 1: 71da6f53a44 ! 1: dd81a2cadec python file more pytonic, adjust \"if\" and \"for\"\n @@\n ## Metadata ##\n -Author: dsal3389 \n +Author: Daniel Sonbolian \n \n ## Commit message ##\n - python file more pytonic, adjust \"if\" and \"for\"\n + git-p4: minor optimization in read_pip_lines\n \n - L371\n - redesign few lines to get rid of the \"else\" statement\n -\n - L404\n - moved the if statement below another if statement that\n - checks if it should exit the code, only if it doesnt need to,\n - then we can iterate the for loop and decode the text\n -\n - Changes to be committed:\n - modified: git-p4.py\n + checking for an error condition before reading and/or decoding\n + lines from the pip stream to avoid unnecessary computation\n \n Signed-off-by: Daniel Sonbolian \n \n ## git-p4.py ##\n -@@ git-p4.py: def read_pipe(c, ignore_error=False, raw=False, *k, **kw):\n - \"\"\"\n - retcode, out, err = read_pipe_full(c, *k, **kw)\n - if retcode != 0:\n -- if ignore_error:\n -- out = \"\"\n -- else:\n -+ if not ignore_error:\n - die('Command failed: {}\\nError: {}'.format(' '.join(c), err))\n -+ out = \"\"\n - if not raw:\n - out = decode_text_stream(out)\n - return out\n @@ git-p4.py: def read_pipe_lines(c, raw=False, *k, **kw):\n + \n p = subprocess.Popen(c, stdout=subprocess.PIPE, *k, **kw)\n pipe = p.stdout\n ++\n ++ if p.wait():\n ++ die('Command failed: {}'.format(' '.join(c)))\n ++\n lines = pipe.readlines()\n -- if not raw:\n ++ pipe.close()\n ++\n + if not raw:\n - lines = [decode_text_stream(line) for line in lines]\n - if pipe.close() or p.wait():\n - die('Command failed: {}'.format(' '.join(c)))\n -+ if not raw:\n -+ lines = [decode_text_stream(line) for line in lines]\n +- if pipe.close() or p.wait():\n +- die('Command failed: {}'.format(' '.join(c)))\n ++ return [decode_text_stream(line) for line in lines]\n return lines\n \n \n 2: c107ad9f6ff ! 2: 7fe59688018 removed else statement\n @@\n ## Metadata ##\n -Author: dsal3389 \n +Author: Daniel Sonbolian \n \n ## Commit message ##\n - removed else statement\n + git.c: improve code readability in cmd_main\n \n - there is no need for the else statement if we can do it more\n - elegantly with a signle if statement we no \"else\"\n + checking for an error condition whose body unconditionally exists first,\n + and then the special casing of \"version\" and \"help\" as part of the\n + preparation for the \"normal codepath\", making the code simpler to read\n \n Signed-off-by: Daniel Sonbolian \n \n\n-- \ngitgitgadget\n"},{"id":"464376","messageId":"dd81a2cadec3e3f131d7b573bf110d4b6cc8f40d.1665153486.git.gitgitgadget@gmail.com","threadId":"58571","inReplyTo":"pull.1355.v2.git.git.1665153486.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] git-p4: minor optimization in read_pip_lines","fromName":"Daniel Sonbolian via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-07T14:38:05Z","receivedAt":"2022-10-07T14:38:16Z","isPatch":true,"sender":{"key":"name:Daniel Sonbolian","avatar":null},"body":"From: Daniel Sonbolian \n\nchecking for an error condition before reading and/or decoding\nlines from the pip stream to avoid unnecessary computation\n\nSigned-off-by: Daniel Sonbolian \n---\n git-p4.py | 10 +++++++---\n 1 file changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex d26a980e5ac..097272a5543 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -399,11 +399,15 @@ def read_pipe_lines(c, raw=False, *k, **kw):\n \n p = subprocess.Popen(c, stdout=subprocess.PIPE, *k, **kw)\n pipe = p.stdout\n+\n+ if p.wait():\n+ die('Command failed: {}'.format(' '.join(c)))\n+\n lines = pipe.readlines()\n+ pipe.close()\n+\n if not raw:\n- lines = [decode_text_stream(line) for line in lines]\n- if pipe.close() or p.wait():\n- die('Command failed: {}'.format(' '.join(c)))\n+ return [decode_text_stream(line) for line in lines]\n return lines\n \n \n-- \ngitgitgadget\n\n"},{"id":"464377","messageId":"7fe59688018d195f27456af029d5b4fd8e40206b.1665153486.git.gitgitgadget@gmail.com","threadId":"58571","inReplyTo":"pull.1355.v2.git.git.1665153486.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] git.c: improve code readability in cmd_main","fromName":"Daniel Sonbolian via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-07T14:38:06Z","receivedAt":"2022-10-07T14:38:24Z","isPatch":true,"sender":{"key":"name:Daniel Sonbolian","avatar":null},"body":"From: Daniel Sonbolian \n\nchecking for an error condition whose body unconditionally exists first,\nand then the special casing of \"version\" and \"help\" as part of the\npreparation for the \"normal codepath\", making the code simpler to read\n\nSigned-off-by: Daniel Sonbolian \n---\n git.c | 14 ++++++++------\n 1 file changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex da411c53822..340ec8bcb31 100644\n--- a/git.c\n+++ b/git.c\n@@ -894,12 +894,8 @@ int cmd_main(int argc, const char **argv)\n \targv++;\n \targc--;\n \thandle_options(&argv, &argc, NULL);\n-\tif (argc > 0) {\n-\t\tif (!strcmp(\"--version\", argv[0]) || !strcmp(\"-v\", argv[0]))\n-\t\t\targv[0] = \"version\";\n-\t\telse if (!strcmp(\"--help\", argv[0]) || !strcmp(\"-h\", argv[0]))\n-\t\t\targv[0] = \"help\";\n-\t} else {\n+\n+\tif (argc <= 0) {\n \t\t/* The user didn't specify a command; give them help */\n \t\tcommit_pager_choice();\n \t\tprintf(_(\"usage: %s\\n\\n\"), git_usage_string);\n@@ -907,6 +903,12 @@ int cmd_main(int argc, const char **argv)\n \t\tprintf(\"\\n%s\\n\", _(git_more_info_string));\n \t\texit(1);\n \t}\n+\n+\tif (!strcmp(\"--version\", argv[0]) || !strcmp(\"-v\", argv[0]))\n+\t\targv[0] = \"version\";\n+\telse if (!strcmp(\"--help\", argv[0]) || !strcmp(\"-h\", argv[0]))\n+\t\targv[0] = \"help\";\n+\n \tcmd = argv[0];\n \n \t/*\n-- \ngitgitgadget\n"},{"id":"464380","messageId":"69d57d56-2b74-5ee0-a279-de5eb10df7bf@dunelm.org.uk","threadId":"58571","inReplyTo":"dd81a2cadec3e3f131d7b573bf110d4b6cc8f40d.1665153486.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/2] git-p4: minor optimization in read_pip_lines","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-10-07T15:17:33Z","receivedAt":"2022-10-07T15:17:41Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Daniel\n\nOn 07/10/2022 15:38, Daniel Sonbolian via GitGitGadget wrote:\n> From: Daniel Sonbolian \n> \n> checking for an error condition before reading and/or decoding\n> lines from the pip stream to avoid unnecessary computation\n\nIt would be helpful to say a little more about what the error is you're \ndetecting why it is better to detect it earlier. Having looked at the \nchanges I'm not really sure what these changes are trying to improve\n\n> Signed-off-by: Daniel Sonbolian \n> ---\n> git-p4.py | 10 +++++++---\n> 1 file changed, 7 insertions(+), 3 deletions(-)\n> \n> diff --git a/git-p4.py b/git-p4.py\n> index d26a980e5ac..097272a5543 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -399,11 +399,15 @@ def read_pipe_lines(c, raw=False, *k, **kw):\n> \n> p = subprocess.Popen(c, stdout=subprocess.PIPE, *k, **kw)\n> pipe = p.stdout\n> +\n> + if p.wait():\n> + die('Command failed: {}'.format(' '.join(c)))\n\nI'm not a python programmer, but the documentation for Popen.wait() says \nthat this will wait for the process to finish, so I think calling it \nbefore reading the lines below is an error.\n\n> lines = pipe.readlines()\n> + pipe.close()\n\nYou're now ignoring the return value of pipe.close() - is that safe?\n\n> +\n> if not raw:\n> - lines = [decode_text_stream(line) for line in lines]\n> - if pipe.close() or p.wait():\n> - die('Command failed: {}'.format(' '.join(c)))\n> + return [decode_text_stream(line) for line in lines]\n> return lines\n\nI'm not really sure what you're tying to achieve here - what was wrong \nwith the original code?\n\nBest Wishes\n\nPhillip\n\n"},{"id":"464387","messageId":"xmqqy1trfsqm.fsf@gitster.g","threadId":"58571","inReplyTo":"69d57d56-2b74-5ee0-a279-de5eb10df7bf@dunelm.org.uk","subject":"Re: [PATCH v2 1/2] git-p4: minor optimization in read_pip_lines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-07T17:28:33Z","receivedAt":"2022-10-07T17:28:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> Hi Daniel\n>\n> On 07/10/2022 15:38, Daniel Sonbolian via GitGitGadget wrote:\n>> From: Daniel Sonbolian \n>> checking for an error condition before reading and/or decoding\n>> lines from the pip stream to avoid unnecessary computation\n>\n> It would be helpful to say a little more about what the error is\n> you're detecting why it is better to detect it earlier. Having looked\n> at the changes I'm not really sure what these changes are trying to\n> improve\n\nThanks for the comments.\n\nAlso, even though Documentation/SubmittingPatches does not mention\nin its [[describe-changes]] section, we do use the usual\ncapitalization in the body of the commit log message (after the\n: prefix on the commit title is the only exception).\n\nI agree with everything you said in your review about the code.\nUnless what pipe.readlines() would read is so small that it fits in\nthe OS pipe buffer, waiting for the subprocess to finish without\nreading its output is likely to deadlock.\n\nThanks.\n\n>> Signed-off-by: Daniel Sonbolian \n>> ---\n>> git-p4.py | 10 +++++++---\n>> 1 file changed, 7 insertions(+), 3 deletions(-)\n>> diff --git a/git-p4.py b/git-p4.py\n>> index d26a980e5ac..097272a5543 100755\n>> --- a/git-p4.py\n>> +++ b/git-p4.py\n>> @@ -399,11 +399,15 @@ def read_pipe_lines(c, raw=False, *k, **kw):\n>> p = subprocess.Popen(c, stdout=subprocess.PIPE, *k, **kw)\n>> pipe = p.stdout\n>> +\n>> + if p.wait():\n>> + die('Command failed: {}'.format(' '.join(c)))\n>\n> I'm not a python programmer, but the documentation for Popen.wait()\n> says that this will wait for the process to finish, so I think calling\n> it before reading the lines below is an error.\n>\n>> lines = pipe.readlines()\n>> + pipe.close()\n>\n> You're now ignoring the return value of pipe.close() - is that safe?\n>\n>> +\n>> if not raw:\n>> - lines = [decode_text_stream(line) for line in lines]\n>> - if pipe.close() or p.wait():\n>> - die('Command failed: {}'.format(' '.join(c)))\n>> + return [decode_text_stream(line) for line in lines]\n>> return lines\n>\n> I'm not really sure what you're tying to achieve here - what was wrong\n> with the original code?\n>\n> Best Wishes\n>\n> Phillip\n"},{"id":"464395","messageId":"xmqqpmf3eb26.fsf@gitster.g","threadId":"58571","inReplyTo":"44d2f602-a299-3bd0-1624-cfdddb6dcda7@github.com","subject":"Re: [PATCH 2/2] removed else statement","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-07T18:35:45Z","receivedAt":"2022-10-07T18:36:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Victoria Dye writes:\n\n> dsal3389 via GitGitGadget wrote:\n>> From: dsal3389 \n>> \n>> there is no need for the else statement if we can do it more\n>> elegantly with a signle if statement we no \"else\"\n>\n> Similar recommendations on the commit message as in the previous patch [1]:\n>\n> - title should be prefixed with 'git.c:'\n> - title & message should use the imperative mood (e.g. \"remove else\n> statement\" instead of \"removed else statement\")\n> - please fix typos\n> - s/there/There\n> - s/signle/single\n> - s/we/with(?)\n\nThanks for these.\n\n>> +\tif (argc <= 0) {\n>\n> nit: argc is always >= 0 [2], so a more appropriate condition would be:\n>\n> if (!argc)\n>\n> There are lots of examples of that '!argc' conditional in Git, but none of\n> the 'argc <= 0' pattern, so it's probably best to match convention here.\n\nYes to this, too.\n"},{"id":"464429","messageId":"pull.1355.v3.git.git.1665246097190.gitgitgadget@gmail.com","threadId":"58571","inReplyTo":"pull.1355.v2.git.git.1665153486.gitgitgadget@gmail.com","subject":"[PATCH v3] git.c: improve code readability in cmd_main","fromName":"Daniel Sonbolian via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-08T16:21:37Z","receivedAt":"2022-10-08T16:21:43Z","isPatch":true,"sender":{"key":"name:Daniel Sonbolian","avatar":null},"body":"From: Daniel Sonbolian \n\nChecking for an error condition whose body unconditionally exists first,\nand then the special casing of \"version\" and \"help\" as part of the\npreparation for the \"normal codepath\", making the code simpler to read.\n\nSigned-off-by: Daniel Sonbolian \n---\n git.c: improve readability in cmd_main\n \n made the code more readable in cmd_main by moving the spacial casing for\n \"version\" and \"help\" as part of the regular code path\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1355%2Fdsal3389%2Frm-useless-else-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1355/dsal3389/rm-useless-else-v3\nPull-Request: https://github.com/git/git/pull/1355\n\nRange-diff vs v2:\n\n 1: dd81a2cadec < -: ----------- git-p4: minor optimization in read_pip_lines\n 2: 7fe59688018 ! 1: 664974f71d4 git.c: improve code readability in cmd_main\n @@ Metadata\n ## Commit message ##\n git.c: improve code readability in cmd_main\n \n - checking for an error condition whose body unconditionally exists first,\n + Checking for an error condition whose body unconditionally exists first,\n and then the special casing of \"version\" and \"help\" as part of the\n - preparation for the \"normal codepath\", making the code simpler to read\n + preparation for the \"normal codepath\", making the code simpler to read.\n \n Signed-off-by: Daniel Sonbolian \n \n @@ git.c: int cmd_main(int argc, const char **argv)\n -\t\t\targv[0] = \"help\";\n -\t} else {\n +\n -+\tif (argc <= 0) {\n ++\tif (!argc) {\n \t\t/* The user didn't specify a command; give them help */\n \t\tcommit_pager_choice();\n \t\tprintf(_(\"usage: %s\\n\\n\"), git_usage_string);\n\n\n git.c | 14 ++++++++------\n 1 file changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex da411c53822..ee7758dcb0e 100644\n--- a/git.c\n+++ b/git.c\n@@ -894,12 +894,8 @@ int cmd_main(int argc, const char **argv)\n \targv++;\n \targc--;\n \thandle_options(&argv, &argc, NULL);\n-\tif (argc > 0) {\n-\t\tif (!strcmp(\"--version\", argv[0]) || !strcmp(\"-v\", argv[0]))\n-\t\t\targv[0] = \"version\";\n-\t\telse if (!strcmp(\"--help\", argv[0]) || !strcmp(\"-h\", argv[0]))\n-\t\t\targv[0] = \"help\";\n-\t} else {\n+\n+\tif (!argc) {\n \t\t/* The user didn't specify a command; give them help */\n \t\tcommit_pager_choice();\n \t\tprintf(_(\"usage: %s\\n\\n\"), git_usage_string);\n@@ -907,6 +903,12 @@ int cmd_main(int argc, const char **argv)\n \t\tprintf(\"\\n%s\\n\", _(git_more_info_string));\n \t\texit(1);\n \t}\n+\n+\tif (!strcmp(\"--version\", argv[0]) || !strcmp(\"-v\", argv[0]))\n+\t\targv[0] = \"version\";\n+\telse if (!strcmp(\"--help\", argv[0]) || !strcmp(\"-h\", argv[0]))\n+\t\targv[0] = \"help\";\n+\n \tcmd = argv[0];\n \n \t/*\n\nbase-commit: bcd6bc478adc4951d57ec597c44b12ee74bc88fb\n-- \ngitgitgadget\n"}]}