Thanks for the resubmission. Comments below...
On Mon, Mar 17, 2014 at 11:51 AM, Dragos Foianu <dragos.foianu@xxxxxxxxx> wrote:
> This patch uses a table to store the different messages that can
> be emitted by the verbose install_branch_config function. It
> computes an index based on the three flags and prints the message
> located at the specific index in the table of messages. If the
> index somehow is not within the table, we have a bug.
Most of this text can be dropped due to redundancy.
Saying "This patch..." is unnecessary.
The remaining text primarily says in prose what the patch itself
conveys more concisely and precisely. It's easier to read and
understand the actual code than it is to wade through a lengthy
description of the code change.
Speak in imperative voice: "Use a table to store..."
You might, for instance, say instead something like this:
install_branch_config() uses a long, somewhat complex if-chain to
select a message to display in verbose mode. Simplify the logic
by moving the messages to a table from which they can be
easily retrieved without complex logic.
> Signed-off-by: Dragos Foianu <dragos.foianu@xxxxxxxxx>
> ---
> branch.c | 44 +++++++++++++++++++++++++-------------------
> 1 file changed, 25 insertions(+), 19 deletions(-)
>
> diff --git a/branch.c b/branch.c
> index 723a36b..95645d5 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -54,6 +54,18 @@ void install_branch_config(int flag, const char *local, const char *origin, cons
> struct strbuf key = STRBUF_INIT;
> int rebasing = should_setup_rebase(origin);
>
> + const char *messages[] = {
> + N_("Branch %s set up to track local ref %s."),
> + N_("Branch %s set up to track remote ref %s."),
> + N_("Branch %s set up to track local branch %s."),
> + N_("Branch %s set up to track remote branch %s from %s."),
> + N_("Branch %s set up to track local ref %s by rebasing."),
> + N_("Branch %s set up to track remote ref %s by rebasing."),
> + N_("Branch %s set up to track local branch %s by rebasing."),
> + N_("Branch %s set up to track remote branch %s from %s by rebasing.")
> + };
> + int index = 0;
> +
> if (remote_is_branch
> && !strcmp(local, shortname)
> && !origin) {
> @@ -76,28 +88,22 @@ void install_branch_config(int flag, const char *local, const char *origin, cons
> }
> strbuf_release(&key);
>
> + if (origin)
> + index += 1;
> + if (remote_is_branch)
> + index += 2;
> + if (rebasing)
> + index += 4;
Other submissions have computed this value mathematically without need
for conditionals. For instance, we've seen:
index = (!!origin << 0) + (!!remote_is_branch << 1) + (!!rebasing << 2)
as, well as the equivalent:
index = !!origin + !!remote_is_branch * 2 + !!rebasing * 4
Although this works, it does place greater cognitive demands on the
reader by requiring more effort to figure out what is going on and how
it relates to table position. The original (ungainly) chain of 'if'
statements in the original code does not suffer this problem. It
likewise is harder to understand than merely indexing into a
multi-dimension table where each variable is a 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);
> + printf_ln(_(messages[index]),
> + local, shortname, origin);
> else
> + printf_ln(_(messages[index]),
> + local, (!remote_is_branch) ? remote : shortname);
It's possible to simplify this logic and have only a single
printf_ln() invocation. Hint: It's safe to pass in more arguments than
there are %s directives in the format string.
> +
> + if (index < 0 || index > sizeof(messages) / sizeof(*messages))
> die("BUG: impossible combination of %d and %p",
> remote_is_branch, origin);
You can use ARRAY_SIZE() in place of sizeof(...)/sizeof(...).
Since an out-of-bound index would be a programmer bug, it would
probably be more appropriate to use an assert(), just after 'index' is
computed, rather than if+die(). The original code used die() because
it couldn't detect the error until the end of the if-chain.
> }
> --
> 1.8.3.2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html