Re: [PATCH][GSOC2014] install_branch_config: change logical chain to lookup table
On Fri, Mar 14, 2014 at 5:30 PM, TamerTas wrote:
> Signed-off-by: TamerTas
> ---
> Thanks again for the feedback it's been a great learning experience. Comments
> Below :)
>
> I have refactored the commit [1] to * suggested changes [2].
> format-patch was placing 2 hyphens instead of 3 but it's fixed now.
> I've turned the table into a multidimensional one and I didn't put
> the inner braces since this was used method in other multidimensional arrays
> throughout the project.
Thanks, this is looking better. A few minor comments below, but it's
probably not worth a re-roll. What is important is that you have had a
taste of the review process on this project, and the GSoC mentors have
had a chance to observe your abilities and reviewer interaction.
> I've also changed shortname to short_name since that seems to be how
> variables are named
> in this project.
This is a conceptually distinct change which should be done as a
separate "cleanup" patch since it otherwise adds noise which obscures
the "real" change (rewriting the if-chain). One of the goals of a
patch submitter is to make the review process as streamlined as
possible, so noise should be avoided.
Apart from that, unless the variable is really improperly named and
misleading, this sort of change (renaming "shortname" to "short_name")
is likely to be considered unnecessary code churn which will probably
be rejected. At any given time, there are many patch series in-flight
which Junio has to juggle, and code churn increases possibility of
conflict between them, which makes his job more difficult.
> It appears that table-driven code might be more readable after all.
>
> [1]http://git.661346.n2.nabble.com/PATCH-GSOC2014-install-branch-config-change-logical-chain-to-lookup-table-tp7605550.html
> [2]http://git.661346.n2.nabble.com/PATCH-GSOC2014-install-branch-config-change-logical-chain-to-lookup-table-tp7605550p7605605.html
> ---
> branch.c | 42 +-
> 1 file changed, 17 insertions(+), 25 deletions(-)
>
> diff --git a/branch.c b/branch.c
> index 723a36b..eab6fa4 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -49,13 +49,27 @@ static int should_setup_rebase(const char *origin)
>
> void install_branch_config(int flag, const char *local, const char *origin,
> const char *remote)
> {
> - const char *shortname = remote + 11;
> + const char *short_name = remote + 11;
> + const char *setup_message[][2][2] = {
> + N_("Branch %s set up to track local ref %s."),
> + N_("Branch %s set up to track local branch %s."),
> + N_("Branch %s set up to track remote ref %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 local branch %s by rebasing."),
> + N_("Branch %s set up to track remote ref %s by rebasing."),
> + N_("Branch %s set up to track remote branch %s from %s by
> rebasing.")
> + };
> +
> int remote_is_branch = starts_with(remote, "refs/heads/");
> struct strbuf key = STRBUF_INIT;
> int rebasing = should_setup_rebase(origin);
>
> + const char *remote_name = remote_is_branch? short_name : remote;
> + const char *message =
> setup_message[!!rebasing][!!origin][!!remote_is_branch];
In the previous review, it was suggested to pull this value out into
its own variable to make the code less noisy since it was being
accessed frequently in just a few lines of code. However, since you've
collapsed the code to a single printf_ln() invocation in this patch,
the separate variable may be not helping clarity (especially as the
assignment is divorced by some distance from the code which references
it). The same is probably true of 'remote_name' which is used in just
the one printf_ln() call.
> if (remote_is_branch
> - && !strcmp(local, shortname)
> + && !strcmp(local, short_name)
> && !origin) {
> warning(_("Not setting branch %s as its own upstream."),
> local);
> @@ -77,29 +91,7 @@ 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 t
Re: [PATCH][GSOC2014] install_branch_config: change logical chain to lookup table
On Wed, Mar 12, 2014 at 5:24 PM, TamerTas wrote:
>
> Signed-off-by: TamerTas
> --
There should be three hyphens "---" here but somehow you have only two
"--". Since "---" is detected automatically when the patch is applied,
this deviation can be problematic.
> Thanks for the feedback. Comments below.
Thanks for the resubmission. Comments below. :-)
> I've made the suggested changes [1] to patch [2]
Better. This is a more well-crafted submission.
> but, since there are different number of format
> specifiers, an if-else clause is necessary.
> Removing the if-else chain completely doesn't seem to be possible.
> So making the format table-driven seems to be like an optional change.
Explaining why you chose one approach over another is indeed good
etiquette, and can forestall questions which reviewers may otherwise
ask.
Note, however, that it is possible to move this logic into a table.
Clue: it is not an error to supply more arguments than there are %s's
in the format string. This is not saying that you must make it
table-driven, but perhaps it may alter the reasons you gave above for
rejecting it (assuming you still do).
> [1]:
> http://git.661346.n2.nabble.com/PATCH-GSOC2014-changed-logical-chain-in-branch-c-to-lookup-tables-tp7605343p7605444.html
> [2]:
> http://git.661346.n2.nabble.com/PATCH-GSOC2014-changed-logical-chain-in-branch-c-to-lookup-tables-tp7605343p7605407.html
> --
> branch.c | 44 +---
> 1 file changed, 21 insertions(+), 23 deletions(-)
>
> diff --git a/branch.c b/branch.c
> index 723a36b..1ccf30f 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -50,10 +50,25 @@ static int should_setup_rebase(const char *origin)
> void install_branch_config(int flag, const char *local, const char *origin,
> const char *remote)
> {
> const char *shortname = remote + 11;
> + const char *setup_message[] = {
> + N_("Branch %s set up to track local ref %s."),
> + N_("Branch %s set up to track local branch %s."),
> + N_("Branch %s set up to track remote ref %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 local branch %s by rebasing."),
> + N_("Branch %s set up to track remote ref %s by rebasing."),
> + N_("Branch %s set up to track remote branch %s from %s by
> rebasing."),
Some compilers will warn about the trailing comma, so you might want to drop it.
> + };
> +
> int remote_is_branch = starts_with(remote, "refs/heads/");
> struct strbuf key = STRBUF_INIT;
> int rebasing = should_setup_rebase(origin);
>
> + int msg_index = (!!remote_is_branch << 0) +
> + (!!origin << 1) +
> + (!!rebasing << 2);
Better than the last (buggy) attempt. Nevertheless, it's a fairly
magical incantation requiring more thought than some other approaches.
Have you considered instead using a multi-dimensional array for the
messages and then indexing into it with these variables as direct keys
(after using ! or !! to constrain them to 0 or 1)? Would that be
better or worse?
> if (remote_is_branch
> && !strcmp(local, shortname)
> && !origin) {
> @@ -77,29 +92,12 @@ 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);
> - else
> -
