* Re: [PATH] branch.c:install_branch_config():Simplify code generating verbose message.
2014-03-06 7:58 [PATH] branch.c:install_branch_config():Simplify code generating verbose message Paweł Wawruch
@ 2014-03-07 8:55 ` Eric Sunshine
0 siblings, 0 replies; 2+ messages in thread
From: Eric Sunshine @ 2014-03-07 8:55 UTC (permalink / raw)
To: Paweł Wawruch; +Cc: Git List
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
^ permalink raw reply [flat|nested] 2+ messages in thread