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