Re: [PATH] branch.c:install_branch_config():Simplify code generating verbose message.
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Mar 7, 2014, 08:55 UTC
- Message-ID
- <CAPig+cQTSMZq1X7131cbnAB7py_3GXf60ricX4hq6DNB7uHsbQ@mail.gmail.com>
- In-Reply-To
- <8889967.JbMR8CXVSx@endymion>
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.
Show 10 quoted lines
> 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.
Show 23 quoted lines
> 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.
Show 10 quoted lines
> + 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;
Show 6 quoted lines
> + 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.
Show 5 quoted lines
> } > } > > -- > 1.8.3.2