Re: [PATHv2] branch.c:install_branch_config():Simplify code generating verbose message.
From: Eric Sunshine <hidden>
Date: 2016-06-15 23:00:16
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 [off-list ref] 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 <redacted> ---
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
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
+ 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
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