[PATH] branch.c:install_branch_config():Simplify code generating verbose message.

[PATH] branch.c:install_branch_config():Simplify code generating verbose message.

[Paweł Wawruch]

>From adfcfa0a334378a6242347efc0d614fa193610db Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pawe=C5=82=20Wawruch?= <pawlo@aleg.pl>
Date: Thu, 6 Mar 2014 00:05:00 +0100
Subject: [PATCH] branch.c:install_branch_config(): Simplify the long chain of
 if statements. Threre was a long chain of if statements. The code can be more
 clear. The chain is replaced with table of strings. New approach is more
 compact.

---
 branch.c | 38 ++++++++++++++++----------------------
 1 file changed, 16 insertions(+), 22 deletions(-)

diff --git a/branch.c b/branch.c
index 723a36b..ebc2172 100644
--- a/branch.c
+++ b/branch.c
@@ -77,29 +77,23 @@ void install_branch_config(int flag, const char *local, 
const char *origin, cons
 	strbuf_release(&key);
 
 	if (flag & BRANCH_CONFIG_VERBOSE) {
-		if (remote_is_branch && origin)
-			printf_ln(rebasing ?
-				  _("Branch %s set up to track remote branch %s from %s by 
rebasing.") :
-				  _("Branch %s set up to track remote branch %s from %s."),
-				  local, shortname, origin);
-		else if (remote_is_branch && !origin)
-			printf_ln(rebasing ?
-				  _("Branch %s set up to track local branch %s by rebasing.") :
-				  _("Branch %s set up to track local branch %s."),
-				  local, shortname);
-		else if (!remote_is_branch && origin)
-			printf_ln(rebasing ?
-				  _("Branch %s set up to track remote ref %s by rebasing.") :
-				  _("Branch %s set up to track remote ref %s."),
-				  local, remote);
-		else if (!remote_is_branch && !origin)
-			printf_ln(rebasing ?
-				  _("Branch %s set up to track local ref %s by rebasing.") :
-				  _("Branch %s set up to track local ref %s."),
-				  local, remote);
+		const char *messages[8] ;
+		messages[0] = _("Branch %s set up to track remote branch %s from %s by 
rebasing.");
+		messages[1] = _("Branch %s set up to track remote branch %s from %s.");
+		messages[2] = _("Branch %s set up to track local branch %s by 
rebasing.");
+		messages[3] = _("Branch %s set up to track local branch %s.");
+		messages[4] = _("Branch %s set up to track remote ref %s by rebasing.");
+		messages[5] = _("Branch %s set up to track remote ref %s.");
+		messages[6] = _("Branch %s set up to track local ref %s by rebasing.");
+		messages[7] = _("Branch %s set up to track local ref %s.");
+
+		const char *name = remote_is_branch ? remote : shortname;
+		int message_number = rebasing + 2 * (origin != NULL) + 4 * 
remote_is_branch;
+
+		if (message_number < 2)
+			printf_ln(messages[message_number], local, name, origin);
 		else
-			die("BUG: impossible combination of %d and %p",
-			    remote_is_branch, origin);
+			printf_ln(messages[message_number], local, name);
 	}
 }
 
-- 
1.8.3.2

Re: [PATH] branch.c:install_branch_config():Simplify code generating verbose message.

[Eric Sunshine]

Thanks for the submission. Some comments below to give you a taste of
the Git review process...

On Thu, Mar 6, 2014 at 2:58 AM, Paweł Wawruch <pawlo@aleg.pl> wrote:
> From adfcfa0a334378a6242347efc0d614fa193610db Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pawe=C5=82=20Wawruch?= <pawlo@aleg.pl>
> Date: Thu, 6 Mar 2014 00:05:00 +0100

Drop these lines. They are extracted automatically by "git am" from
the headers of your mail message.

> Subject: [PATCH] branch.c:install_branch_config(): Simplify the long chain of
>  if statements. Threre was a long chain of if statements. The code can be more
>  clear. The chain is replaced with table of strings. New approach is more
>  compact.

The Subject: also is extracted automatically from the email, so it
should not be pasted here. Instead, when you compose your commit
message, write a very concise summary of the patch as the very first
line, then a blank line, then the meat of the description using as
many lines as needed. Wrap the lines to about 65-70 characters. "git
format-patch" automatically uses that concise first line as the
Subject: of the mail message. This is all explained in section "(2)
Describe your changes well" of Documentation/SubmittingPatches.

Write in imperative tone. Rather than "There was a long...", say
"There is a long...". Rather than "The chain is replaced...", say
"Replace the chain...".

You misspelled "There" as "Threre".

Your patch sign-off is missing. It should be placed right here above
the "---" line. See Documentation/SubmittingPatches.

> ---

The Git GSoC page on which you found this microproject suggests
mentioning that this is a GSoC submission so that reviewers don't
overlook it. Either do so by adding [GSoC] to the subject or by
mentioning it right here as "patch commentary" after the "---" line.

>  branch.c | 38 ++++++++++++++++----------------------
>  1 file changed, 16 insertions(+), 22 deletions(-)
>
> diff --git a/branch.c b/branch.c
> index 723a36b..ebc2172 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -77,29 +77,23 @@ void install_branch_config(int flag, const char *local,
> const char *origin, cons
>         strbuf_release(&key);

Your patch is whitespace damaged, and thus not easily applied. This
probably resulted from pasting it into your email client. Using "git
send-email" avoids this problem.

>         if (flag & BRANCH_CONFIG_VERBOSE) {
> -               if (remote_is_branch && origin)
> -                       printf_ln(rebasing ?
> -                                 _("Branch %s set up to track remote branch %s from %s by
> rebasing.") :
> -                                 _("Branch %s set up to track remote branch %s from %s."),
> -                                 local, shortname, origin);
> -               else if (remote_is_branch && !origin)
> -                       printf_ln(rebasing ?
> -                                 _("Branch %s set up to track local branch %s by rebasing.") :
> -                                 _("Branch %s set up to track local branch %s."),
> -                                 local, shortname);
> -               else if (!remote_is_branch && origin)
> -                       printf_ln(rebasing ?
> -                                 _("Branch %s set up to track remote ref %s by rebasing.") :
> -                                 _("Branch %s set up to track remote ref %s."),
> -                                 local, remote);
> -               else if (!remote_is_branch && !origin)
> -                       printf_ln(rebasing ?
> -                                 _("Branch %s set up to track local ref %s by rebasing.") :
> -                                 _("Branch %s set up to track local ref %s."),
> -                                 local, remote);
> +               const char *messages[8] ;

Drop the space before the semicolon.

On this project, we avoid intermixing declarations with code, so move
the declaration of messages[] to the top of the block.

> +               messages[0] = _("Branch %s set up to track remote branch %s from %s by
> rebasing.");
> +               messages[1] = _("Branch %s set up to track remote branch %s from %s.");
> +               messages[2] = _("Branch %s set up to track local branch %s by
> rebasing.");
> +               messages[3] = _("Branch %s set up to track local branch %s.");
> +               messages[4] = _("Branch %s set up to track remote ref %s by rebasing.");
> +               messages[5] = _("Branch %s set up to track remote ref %s.");
> +               messages[6] = _("Branch %s set up to track local ref %s by rebasing.");
> +               messages[7] = _("Branch %s set up to track local ref %s.");

Rather than hardcoding the array size and then assigning the elements
individually, perhaps you can construct the array with an initializer.
Use N_(...) rather than _(...).

    const char *messages[] = {
        N_("..."),
        ...
    };

> +               const char *name = remote_is_branch ? remote : shortname;
> +               int message_number = rebasing + 2 * (origin != NULL) + 4 *
> remote_is_branch;

Move the declarations of 'name' and 'message_number' to the top of the block.

I don't know how other people feel about it, but it disturbs me a bit
to see message_number computed from these other variables without
ensuring that their values really are 0 or 1. I checked the
implementation of the functions from which these variables are set, so
I know that they are indeed 0 or 1, but doing so was an extra burden
which wouldn't have been required if this code was more explicit about
their values. For instance:

    int message_number = !!rebasing + 2 * !!origin + 4 * !!remote_is_branch;

> +               if (message_number < 2)
> +                       printf_ln(messages[message_number], local, name, origin);
>                 else
> -                       die("BUG: impossible combination of %d and %p",
> -                           remote_is_branch, origin);
> +                       printf_ln(messages[message_number], local, name);

Again, it disturbs me to see messages[] indexed without ensuring that
message_number is within range. At the very least, an

    assert(message_number < ARRAY_SIZE(messages));

after computing message_number's value would help to protect against
programmer error in future changes.

>         }
>  }
>
> --
> 1.8.3.2