Minor Refactors: remove useless else

git.c removed redundant else statement - it is redundant because all we actually need is to check if there are no arguments, the program print helpful information to the screen and exit, we don't need the else for that

git-p4.py L371 - remove redundant else statement - same as git.c (made sure not to repeat what was already done on #1349
[https://github.com/git/git/pull/1349]) git-p4.py L404 - minor, there is no need to decode the output and iterate over the variable if we may exit

dsal3389 (2):
python file more pytonic, adjust "if" and "for"
removed else statement

 git-p4.py |  9 ++++-----
 git.c     | 14 ++++++++------
 2 files changed, 12 insertions(+), 11 deletions(-)

base-commit: bcd6bc478adc4951d57ec597c44b12ee74bc88fb
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1355%2Fdsal3389%2Frm-useless-else-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1355/dsal3389/rm-useless-else-v1
Pull-Request: https://github.com/git/git/pull/1355

gitgitgadget

From: dsal3389 <[email protected]>

L371
redesign few lines to get rid of the "else" statement

L404
moved the if statement below another if statement that checks if it should exit the code, only if it doesnt need to, then we can iterate the for loop and decode the text

Changes to be committed:
modified: git-p4.py

---
 git-p4.py | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

index d26a980e5ac..0ba5115fa2e 100755
@@ -368,10 +368,9 @@ def read_pipe(c, ignore_error=False, raw=False, *k, **kw):
        """
     retcode, out, err = read_pipe_full(c, *k, **kw)
     if retcode != 0:
-        if ignore_error:
-            out = ""
-        else:
+        if not ignore_error:
             die('Command failed: {}\nError: {}'.format(' '.join(c), err))
+        out = ""
     if not raw:
         out = decode_text_stream(out)
     return out

@@ -400,10 +399,10 @@ def read_pipe_lines(c, raw=False, *k, **kw):
     p = subprocess.Popen(c, stdout=subprocess.PIPE, *k, **kw)
     pipe = p.stdout
     lines = pipe.readlines()
-    if not raw:
-        lines = [decode_text_stream(line) for line in lines]
     if pipe.close() or p.wait():
         die('Command failed: {}'.format(' '.join(c)))
+    if not raw:
+        lines = [decode_text_stream(line) for line in lines]
     return lines
 
 

gitgitgadget

re: the commit message title

s/pytonic/pythonic

Please prefix the title with the file/function being updated. Based on precedent [1], that'd probably be:

git-p4: <your message>

Also, I'm not sure the reference to "for" is helpful, since you're only adjusting if-statements in this patch (although one of those conditionals does include a for-loop, nothing about the loop itself changes).

Finally, please use the imperative mood in your commit messages [2]; something like:

git-p4: adjust "if" statements to be more pythonic

[1] https://lore.kernel.org/git/?q=dfn%3Agit-p4.py
[2] https://git-scm.com/docs/SubmittingPatches#describe-changes

What was your reason for making this change? I don't see anything in PEP8
[3] regarding this kind of conditional organization being preferred over what's already there.

For what it's worth, I'm happy with the change itself; I'd just ask that you include an explanation for why (because it's more concise, or makes the assignment to 'out' clearer, etc.). The "Describe your changes well" section of the "SubmittingPatches" document [2] should be a helpful guideline.

Also, as a matter of convention, it's not necessary to include line numbers.

[2] https://git-scm.com/docs/SubmittingPatches#describe-changes
[3] https://peps.python.org/pep-0008/

While the actual change below makes sense, I found this explanation very difficult to parse. Maybe something like:

Reorder if-statements in 'read_pipe_lines()' to raise an error with the command before trying to decode its results in order to avoid unnecessary computation.

Please remove this text from the commit message.

The rest of the implementation looks good, thanks!

Compare this with the commits by others in "git log --no-merges" output of this project.

Please have this on the in-body "From:" line we see above. I think GitGitGadget takes it from the author of the commit object it sends out, so you may have to go back to your branch and fix them with "rebase -i".

From: dsal3389 <[email protected]>

there is no need for the else statement if we can do it more elegantly with a signle if statement we no "else"

---
 git.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

index da411c53822..340ec8bcb31 100644
@@ -894,12 +894,8 @@ int cmd_main(int argc, const char **argv)
 	argv++;
 	argc--;
 	handle_options(&argv, &argc, NULL);
-	if (argc > 0) {
-		if (!strcmp("--version", argv[0]) || !strcmp("-v", argv[0]))
-			argv[0] = "version";
-		else if (!strcmp("--help", argv[0]) || !strcmp("-h", argv[0]))
-			argv[0] = "help";
-	} else {
+
+	if (argc <= 0) {
 		/* The user didn't specify a command; give them help */
 		commit_pager_choice();
 		printf(_("usage: %s\n\n"), git_usage_string);

@@ -907,6 +903,12 @@ int cmd_main(int argc, const char **argv)
 		printf("\n%s\n", _(git_more_info_string));
 		exit(1);
 	}
+
+	if (!strcmp("--version", argv[0]) || !strcmp("-v", argv[0]))
+		argv[0] = "version";
+	else if (!strcmp("--help", argv[0]) || !strcmp("-h", argv[0]))
+		argv[0] = "help";
+
 	cmd = argv[0];
 
 	/*

gitgitgadget

Similar recommendations on the commit message as in the previous patch [1]:

- title should be prefixed with 'git.c:'
- title & message should use the imperative mood (e.g. "remove else statement" instead of "removed else statement")
- please fix typos
- s/there/There
- s/signle/single
- s/we/with(?)

[1] https://lore.kernel.org/git/[email protected]/

    if (!argc)

Thanks for these.

Yes to this, too.

Some people seem to use subjective words like "elegantly" but they weaken your justification. You are better off avoiding them.

As to the change in the patch, I do find it easier to read to have a check for an error condition whose body unconditionally exits first, and then the special casing of "version" and "help" as part of the preparation for the "normal codepath".

Side note: yes, you can steal from the last paragraph when you are redoing the justfication of this patch.

Thanks.

Welcome to the Git development community.

A few general suggestions.

* Familiarlize yourself with Documentation/CodingGuidelines and Documentation/SubmittingPatches.

* See how commits in the project are explained in their log messages in "git shortlog --no-merges -n100" (for how their titles are written to help distinguish them among many other commits) and "git log --no-merges -n100" (for how they explain why making their changes is a good idea and helps the project).

* Make sure the name and the e-mail used as the author of your commits match the name and the e-mail on the Signed-off-by: line in them.

* Spell check before sending things out to the public list.

git.c - made the code more readable in cmd_main by moving the spacial casing for "version" and "help" as part of the regular code path

git-p4.py - minor optimization in read_pipe_lines by first checking for errors, then reading data and/or decoding it from the pip stream

Daniel Sonbolian (2):
git-p4: minor optimization in read_pip_lines
git.c: improve code readability in cmd_main

 git-p4.py | 10 +++++++---
 git.c     | 14 ++++++++------
 2 files changed, 15 insertions(+), 9 deletions(-)

base-commit: bcd6bc478adc4951d57ec597c44b12ee74bc88fb
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1355%2Fdsal3389%2Frm-useless-else-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1355/dsal3389/rm-useless-else-v2
Pull-Request: https://github.com/git/git/pull/1355

Range-diff vs v1:

1: 71da6f53a44 ! 1: dd81a2cadec python file more pytonic, adjust "if" and "for"
@@
## Metadata ##
-Author: dsal3389 <[email protected]>
+Author: Daniel Sonbolian <[email protected]>

## Commit message ##
- python file more pytonic, adjust "if" and "for"
+ git-p4: minor optimization in read_pip_lines

- L371
- redesign few lines to get rid of the "else" statement
-
- L404
- moved the if statement below another if statement that
- checks if it should exit the code, only if it doesnt need to,
- then we can iterate the for loop and decode the text
-
- Changes to be committed:
- modified: git-p4.py
+ checking for an error condition before reading and/or decoding
+ lines from the pip stream to avoid unnecessary computation

## git-p4.py ##
-@@ git-p4.py: def read_pipe(c, ignore_error=False, raw=False, *k, **kw):
- """
- retcode, out, err = read_pipe_full(c, *k, **kw)
- if retcode != 0:
-- if ignore_error:
-- out = ""
-- else:
-+ if not ignore_error:
- die('Command failed: {}\nError: {}'.format(' '.join(c), err))
-+ out = ""
- if not raw:
- out = decode_text_stream(out)
- return out
@@ git-p4.py: def read_pipe_lines(c, raw=False, *k, **kw):
+
p = subprocess.Popen(c, stdout=subprocess.PIPE, *k, **kw) pipe = p.stdout
++
++ if p.wait():
++ die('Command failed: {}'.format(' '.join(c)))
++
lines = pipe.readlines()
-- if not raw:
++ pipe.close()
++
+ if not raw:
- lines = [decode_text_stream(line) for line in lines]
- if pipe.close() or p.wait():
- die('Command failed: {}'.format(' '.join(c)))
-+ if not raw:
-+ lines = [decode_text_stream(line) for line in lines]
+- if pipe.close() or p.wait():
+- die('Command failed: {}'.format(' '.join(c)))
++ return [decode_text_stream(line) for line in lines]
return lines

2: c107ad9f6ff ! 2: 7fe59688018 removed else statement
@@
## Metadata ##
-Author: dsal3389 <[email protected]> +Author: Daniel Sonbolian <[email protected]>

## Commit message ##
- removed else statement
+ git.c: improve code readability in cmd_main

- there is no need for the else statement if we can do it more
- elegantly with a signle if statement we no "else"
+ checking for an error condition whose body unconditionally exists first,
+ and then the special casing of "version" and "help" as part of the
+ preparation for the "normal codepath", making the code simpler to read

gitgitgadget

From: Daniel Sonbolian <[email protected]>

checking for an error condition before reading and/or decoding lines from the pip stream to avoid unnecessary computation

---
 git-p4.py | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

index d26a980e5ac..097272a5543 100755
@@ -399,11 +399,15 @@ def read_pipe_lines(c, raw=False, *k, **kw):
 
     p = subprocess.Popen(c, stdout=subprocess.PIPE, *k, **kw)
     pipe = p.stdout
+
+    if p.wait():
+        die('Command failed: {}'.format(' '.join(c)))
+
     lines = pipe.readlines()
+    pipe.close()
+
     if not raw:
-        lines = [decode_text_stream(line) for line in lines]
-    if pipe.close() or p.wait():
-        die('Command failed: {}'.format(' '.join(c)))
+        return [decode_text_stream(line) for line in lines]
     return lines
 
 

gitgitgadget

Hi Daniel

It would be helpful to say a little more about what the error is you're detecting why it is better to detect it earlier. Having looked at the changes I'm not really sure what these changes are trying to improve

Thanks for the comments.

Also, even though Documentation/SubmittingPatches does not mention in its [[describe-changes]] section, we do use the usual capitalization in the body of the commit log message (after the <area>: prefix on the commit title is the only exception).

I agree with everything you said in your review about the code. Unless what pipe.readlines() would read is so small that it fits in the OS pipe buffer, waiting for the subprocess to finish without reading its output is likely to deadlock.

Thanks.

From: Daniel Sonbolian <[email protected]>

checking for an error condition whose body unconditionally exists first, and then the special casing of "version" and "help" as part of the preparation for the "normal codepath", making the code simpler to read

---
 git.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

index da411c53822..340ec8bcb31 100644
@@ -894,12 +894,8 @@ int cmd_main(int argc, const char **argv)
 	argv++;
 	argc--;
 	handle_options(&argv, &argc, NULL);
-	if (argc > 0) {
-		if (!strcmp("--version", argv[0]) || !strcmp("-v", argv[0]))
-			argv[0] = "version";
-		else if (!strcmp("--help", argv[0]) || !strcmp("-h", argv[0]))
-			argv[0] = "help";
-	} else {
+
+	if (argc <= 0) {
 		/* The user didn't specify a command; give them help */
 		commit_pager_choice();
 		printf(_("usage: %s\n\n"), git_usage_string);

@@ -907,6 +903,12 @@ int cmd_main(int argc, const char **argv)
 		printf("\n%s\n", _(git_more_info_string));
 		exit(1);
 	}
+
+	if (!strcmp("--version", argv[0]) || !strcmp("-v", argv[0]))
+		argv[0] = "version";
+	else if (!strcmp("--help", argv[0]) || !strcmp("-h", argv[0]))
+		argv[0] = "help";
+
 	cmd = argv[0];
 
 	/*

gitgitgadget

From: Daniel Sonbolian <[email protected]>

Checking for an error condition whose body unconditionally exists first, and then the special casing of "version" and "help" as part of the preparation for the "normal codepath", making the code simpler to read.

---
git.c: improve readability in cmd_main

made the code more readable in cmd_main by moving the spacial casing for "version" and "help" as part of the regular code path

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1355%2Fdsal3389%2Frm-useless-else-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1355/dsal3389/rm-useless-else-v3
Pull-Request: https://github.com/git/git/pull/1355

Range-diff vs v2:

 1:  dd81a2cadec < -:  ----------- git-p4: minor optimization in read_pip_lines
 2:  7fe59688018 ! 1:  664974f71d4 git.c: improve code readability in cmd_main

@@ Metadata
## Commit message ##
git.c: improve code readability in cmd_main

- checking for an error condition whose body unconditionally exists first,
+ Checking for an error condition whose body unconditionally exists first,
and then the special casing of "version" and "help" as part of the
- preparation for the "normal codepath", making the code simpler to read
+ preparation for the "normal codepath", making the code simpler to read.

@@ git.c: int cmd_main(int argc, const char **argv)
- argv[0] = "help";
- } else {
+
-+ if (argc <= 0) {
++ if (!argc) {
/* The user didn't specify a command; give them help */ commit_pager_choice();
printf(_("usage: %s\n\n"), git_usage_string);

 git.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

index da411c53822..ee7758dcb0e 100644
@@ -894,12 +894,8 @@ int cmd_main(int argc, const char **argv)
 	argv++;
 	argc--;
 	handle_options(&argv, &argc, NULL);
-	if (argc > 0) {
-		if (!strcmp("--version", argv[0]) || !strcmp("-v", argv[0]))
-			argv[0] = "version";
-		else if (!strcmp("--help", argv[0]) || !strcmp("-h", argv[0]))
-			argv[0] = "help";
-	} else {
+
+	if (!argc) {
 		/* The user didn't specify a command; give them help */
 		commit_pager_choice();
 		printf(_("usage: %s\n\n"), git_usage_string);

@@ -907,6 +903,12 @@ int cmd_main(int argc, const char **argv)
 		printf("\n%s\n", _(git_more_info_string));
 		exit(1);
 	}
+
+	if (!strcmp("--version", argv[0]) || !strcmp("-v", argv[0]))
+		argv[0] = "version";
+	else if (!strcmp("--help", argv[0]) || !strcmp("-h", argv[0]))
+		argv[0] = "help";
+
 	cmd = argv[0];
 
 	/*

base-commit: bcd6bc478adc4951d57ec597c44b12ee74bc88fb

gitgitgadget