Re: [PATHv2] branch.c:install_branch_config():Simplify code generating verbose message.
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Mar 11, 2014, 10:16 UTC
- Message-ID
- <CAPig+cQdbXz_s2-i0eTOq+rmacNWUvvCbeSZ4Z=JY39oNKFQ_Q@mail.gmail.com>
- In-Reply-To
- <1394527207-23065-1-git-send-email-pawlo@aleg.pl>
Thanks for taking review comments from your previous attempt into account. This is a more well-crafted submission. Additional comments below.
On Tue, Mar 11, 2014 at 4:40 AM, Paweł Wawruch <pawlo@aleg.pl> wrote:
> Subject: [PATHv2] branch.c:install_branch_config():Simplify code generating verbose message.
PATCH is misspelled as PATH. Adding the v2 is indeed good etiquette. It's typical to have a space between PATCH and vN.
Insert a space before "Simplify".
SubmittingPatches suggests omitting the final period in the subject. Also downcase "simplify".
> Simplify the long if chain in install_branch_config().
A bit misleading. You're not actually simplifying the 'if' chain, but rather replacing it.
> There is a long chain of if statements. The code can be more clear. > Replace the chain with table of strings. New approach is more > compact.
The first sentence merely repeats what was said earlier. It can be dropped. The second sentence is self-evident since you already talk about "simplification" and also can be dropped, as can the fourth sentence. The third sentence is a good explanation of how the code is being simplified and should be kept. You could, therefore, collapse the commit message to something along these lines:
Subject: install_branch_config: simplify verbose diagnostic logic
Replace the 'if' chain with a table of strings.
> Signed-off-by: Paweł Wawruch <pawlo@aleg.pl> > ---
It is considerate to reviewers to provide a link to the previous version of the patch, like this [1], and to explain what changed in this version relative to the last. This area just below the "---" line after your sign off is where you would write such commentary.
[1]: http://thread.gmane.org/gmane.comp.version-control.git/243502
Show 21 quoted lines
> branch.c | 40 ++++++++++++++++++----------------------
> 1 file changed, 18 insertions(+), 22 deletions(-)
>
> diff --git a/branch.c b/branch.c
> index 723a36b..8d3b219 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -53,6 +53,18 @@ void install_branch_config(int flag, const char *local, const char *origin, cons
> int remote_is_branch = starts_with(remote, "refs/heads/");
> struct strbuf key = STRBUF_INIT;
> int rebasing = should_setup_rebase(origin);
> + const char *messages[] = {
> + N_("Branch %s set up to track remote branch %s from %s by rebasing."),
> + N_("Branch %s set up to track remote branch %s from %s."),
> + N_("Branch %s set up to track local branch %s by rebasing."),
> + N_("Branch %s set up to track local branch %s."),
> + N_("Branch %s set up to track remote ref %s by rebasing."),
> + N_("Branch %s set up to track remote ref %s."),
> + N_("Branch %s set up to track local ref %s by rebasing."),
> + N_("Branch %s set up to track local ref %s.")
> + };On this project, arrays are usually (though not consistently) named in singular form (for instance message[]) so that a reference to a single item, such as message[42], reads more grammatically correct.
Show 30 quoted lines
> + const char *name = remote_is_branch ? remote : shortname;
> + int message_number;
>
> if (remote_is_branch
> && !strcmp(local, shortname)
> @@ -77,29 +89,13 @@ 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);
> + message_number = (!!rebasing) + 2 * (!!origin) + 4 * (!!remote_is_branch);Unnecessary parentheses make the code more cumbersome to read and should be dropped.
> + assert(message_number < ARRAY_SIZE(messages)); > + > + if (message_number < 2) > + printf_ln(messages[message_number], local, name, origin);
Even though the strings are marked for translation via N_() in the initializer, they still need to be passed through _() when they are actually used, so the invocation should be:
printf_ln(_(messages[message_number]), local, name, origin);
Ditto for the printf_ln() below.
See section 4.7 of the GNU gettext manual [2] for an explanation (and note that N_() is shorthand for gettext_noop() mentioned in the manual).
[2]: http://www.gnu.org/software/gettext/manual/gettext.html
Show 5 quoted lines
> else
> - die("BUG: impossible combination of %d and %p",
> - remote_is_branch, origin);
> + printf_ln(messages[message_number], local, name);
> }Although this approach of making message retrieval table-driven does collapse the code a bit, it's not obvious that it is indeed more clear. Magical incantations, such as '!!rebasing + 2 * !!origin + 4 * !!remote_is_branch' and 'if (message_number < 2)', convey little immediate meaning, demand a greater cognitive load, and may make the code less straightforward to modify in the future. Have you considered other ways of making the code table-driven which exhibit fewer shortcomings?
> } > > -- > 1.8.3.2