[PATCH 0/2] Minor Refactors: remove useless else

[PATCH 0/2] Minor Refactors: remove useless else

[dsal3389 via GitGitGadget]

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

[PATCH 1/2] python file more pytonic, adjust "if" and "for"

[dsal3389 via GitGitGadget]

From: dsal3389 <dsal3389@gmail.com>

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

Signed-off-by: Daniel Sonbolian <dsal3389@gmail.com>
---
 git-p4.py | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index d26a980e5ac..0ba5115fa2e 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -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

[PATCH 2/2] removed else statement

[dsal3389 via GitGitGadget]

From: dsal3389 <dsal3389@gmail.com>

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

Signed-off-by: Daniel Sonbolian <dsal3389@gmail.com>
---
 git.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/git.c b/git.c
index da411c53822..340ec8bcb31 100644
--- a/git.c
+++ b/git.c
@@ -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

Re: [PATCH 1/2] python file more pytonic, adjust "if" and "for"

[Victoria Dye]

dsal3389 via GitGitGadget wrote:
> From: dsal3389 <dsal3389@gmail.com>

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

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

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/

> 
> 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

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.

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

Please remove this text from the commit message.

> 
> Signed-off-by: Daniel Sonbolian <dsal3389@gmail.com>
> ---
>  git-p4.py | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)

The rest of the implementation looks good, thanks!

> 
> diff --git a/git-p4.py b/git-p4.py
> index d26a980e5ac..0ba5115fa2e 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -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
>  
>  

Re: [PATCH 2/2] removed else statement

[Victoria Dye]

dsal3389 via GitGitGadget wrote:
> From: dsal3389 <dsal3389@gmail.com>
> 
> there is no need for the else statement if we can do it more
> elegantly with a signle if statement we no "else"

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/66d1bcaf-64c8-13c2-ba7a-98715de3617b@github.com/

> 
> Signed-off-by: Daniel Sonbolian <dsal3389@gmail.com>
> ---
>  git.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/git.c b/git.c
> index da411c53822..340ec8bcb31 100644
> --- a/git.c
> +++ b/git.c
> @@ -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) {

nit: argc is always >= 0 [2], so a more appropriate condition would be:

    if (!argc)

There are lots of examples of that '!argc' conditional in Git, but none of
the 'argc <= 0' pattern, so it's probably best to match convention here.

Otherwise, the rest of this diff looks good.

[2] https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf, §5.1.2.2.1 #2

>  		/* 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];
>  
>  	/*

Re: [PATCH 0/2] Minor Refactors: remove useless else

[Junio C Hamano]

"dsal3389 via GitGitGadget" <gitgitgadget@gmail.com> writes:

> 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(-)

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.

Re: [PATCH 1/2] python file more pytonic, adjust "if" and "for"

[Junio C Hamano]

"dsal3389 via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: dsal3389 <dsal3389@gmail.com>
>
> 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

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

> Signed-off-by: Daniel Sonbolian <dsal3389@gmail.com>

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".

> ---
>  git-p4.py | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index d26a980e5ac..0ba5115fa2e 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -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 = ""

I think the code with or without the patch is about the same
complexity, but people tend to have harder time understanding logic
that involves double negation, so I can imagine that some readers
may find the code with the patch harder to understand.  In any case,
the difference falls into the "it is minor enough that once it is
written in one way, it is not worth the churn to rewrite it in the
other way" category.

> @@ -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

This is in the same "the difference is minor enough that once it is
written in one way, it is not worth the churn to rewrite it in the
other way" category.  Your reasoning might be that massaging of the
lines is only needed when we do not die() and it is more efficient
to check and die first, but that is optimizing for the wrong case.
The code should not die in its normal operation and there is no
point optimizing for an error code path.

One thing that might deserve benchmarking and optimizing here is if
we can do better than reading everything in lines array and holding
onto the original until decoding the whole thing at once at the end
of input.  If converting each line and appending the result as it is
read from the pipe turns out to be more efficient, it may be an
optimization worth considering, as it is optimizaing for the normal
case.

Re: [PATCH 2/2] removed else statement

[Junio C Hamano]

"dsal3389 via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: dsal3389 <dsal3389@gmail.com>
>
> there is no need for the else statement if we can do it more
> elegantly with a signle if statement we no "else"

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.

> -	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];
>  
>  	/*

[PATCH v2 0/2] git.c: improve readability | git-p4: minor optimization

[Daniel Sonbolian via GitGitGadget]

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 <dsal3389@gmail.com>
     +Author: Daniel Sonbolian <dsal3389@gmail.com>
      
       ## 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
      
          Signed-off-by: Daniel Sonbolian <dsal3389@gmail.com>
      
       ## 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 <dsal3389@gmail.com>
     +Author: Daniel Sonbolian <dsal3389@gmail.com>
      
       ## 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
      
          Signed-off-by: Daniel Sonbolian <dsal3389@gmail.com>
      

-- 
gitgitgadget

[PATCH v2 1/2] git-p4: minor optimization in read_pip_lines

[Daniel Sonbolian via GitGitGadget]

From: Daniel Sonbolian <dsal3389@gmail.com>

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

Signed-off-by: Daniel Sonbolian <dsal3389@gmail.com>
---
 git-p4.py | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index d26a980e5ac..097272a5543 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -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

[PATCH v2 2/2] git.c: improve code readability in cmd_main

[Daniel Sonbolian via GitGitGadget]

From: Daniel Sonbolian <dsal3389@gmail.com>

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

Signed-off-by: Daniel Sonbolian <dsal3389@gmail.com>
---
 git.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/git.c b/git.c
index da411c53822..340ec8bcb31 100644
--- a/git.c
+++ b/git.c
@@ -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

Re: [PATCH v2 1/2] git-p4: minor optimization in read_pip_lines

[Phillip Wood]

Hi Daniel

On 07/10/2022 15:38, Daniel Sonbolian via GitGitGadget wrote:
> From: Daniel Sonbolian <dsal3389@gmail.com>
> 
> checking for an error condition before reading and/or decoding
> lines from the pip stream to avoid unnecessary computation

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

> Signed-off-by: Daniel Sonbolian <dsal3389@gmail.com>
> ---
>   git-p4.py | 10 +++++++---
>   1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/git-p4.py b/git-p4.py
> index d26a980e5ac..097272a5543 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -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)))

I'm not a python programmer, but the documentation for Popen.wait() says 
that this will wait for the process to finish, so I think calling it 
before reading the lines below is an error.

>       lines = pipe.readlines()
> +    pipe.close()

You're now ignoring the return value of pipe.close() - is that safe?

> +
>       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

I'm not really sure what you're tying to achieve here - what was wrong 
with the original code?

Best Wishes

Phillip

Re: [PATCH v2 1/2] git-p4: minor optimization in read_pip_lines

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> Hi Daniel
>
> On 07/10/2022 15:38, Daniel Sonbolian via GitGitGadget wrote:
>> From: Daniel Sonbolian <dsal3389@gmail.com>
>> checking for an error condition before reading and/or decoding
>> lines from the pip stream to avoid unnecessary computation
>
> 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.

>> Signed-off-by: Daniel Sonbolian <dsal3389@gmail.com>
>> ---
>>   git-p4.py | 10 +++++++---
>>   1 file changed, 7 insertions(+), 3 deletions(-)
>> diff --git a/git-p4.py b/git-p4.py
>> index d26a980e5ac..097272a5543 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -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)))
>
> I'm not a python programmer, but the documentation for Popen.wait()
> says that this will wait for the process to finish, so I think calling
> it before reading the lines below is an error.
>
>>       lines = pipe.readlines()
>> +    pipe.close()
>
> You're now ignoring the return value of pipe.close() - is that safe?
>
>> +
>>       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
>
> I'm not really sure what you're tying to achieve here - what was wrong
> with the original code?
>
> Best Wishes
>
> Phillip

Re: [PATCH 2/2] removed else statement

[Junio C Hamano]

Victoria Dye <vdye@github.com> writes:

> dsal3389 via GitGitGadget wrote:
>> From: dsal3389 <dsal3389@gmail.com>
>> 
>> there is no need for the else statement if we can do it more
>> elegantly with a signle if statement we no "else"
>
> 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(?)

Thanks for these.

>> +	if (argc <= 0) {
>
> nit: argc is always >= 0 [2], so a more appropriate condition would be:
>
>     if (!argc)
>
> There are lots of examples of that '!argc' conditional in Git, but none of
> the 'argc <= 0' pattern, so it's probably best to match convention here.

Yes to this, too.

[PATCH v3] git.c: improve code readability in cmd_main

[Daniel Sonbolian via GitGitGadget]

From: Daniel Sonbolian <dsal3389@gmail.com>

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.

Signed-off-by: Daniel Sonbolian <dsal3389@gmail.com>
---
    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.
      
          Signed-off-by: Daniel Sonbolian <dsal3389@gmail.com>
      
     @@ 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(-)

diff --git a/git.c b/git.c
index da411c53822..ee7758dcb0e 100644
--- a/git.c
+++ b/git.c
@@ -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