Thanks for the submission. Comments below to give you a feel for the
Git review process...
On Mon, Mar 10, 2014 at 6:04 PM, Vincenzo di Cicco
<enzodicicco@xxxxxxxxx> wrote:
> From: NaN <enzodicicco@xxxxxxxxx>
Drop this line unless it is intentionally different from your email
From: header, which "git am" will pick up automatically when applying
your patch. On this project, real names are preferred (as you
correctly used in your sign-off).
> Hi there, I've made this patch in according to the rules to participate at GSoC.
> I've seen other patches about this issue very well constructed, so this is only another way to solve this microproject and to test how I can send a patch and discuss about it.
>
> Thanks,
> NaN
These "commentary" lines, which are not intended as part of the
official commit message, belong below the "---" line following your
sign-off. Wrap them to 65-70 characters.
> Signed-off-by: Vincenzo di Cicco <enzodicicco@xxxxxxxxx>
> ---
> Table-driven approach to avoid the long chain of if statements.
This non-commentary information is suitable for the commit message,
thus it belongs above your sign-off.
> branch.c | 37 ++++++++++++++-----------------------
> 1 file changed, 14 insertions(+), 23 deletions(-)
>
> diff --git a/branch.c b/branch.c
> index 723a36b..cb8a544 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -53,6 +53,10 @@ 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);
> + struct strbuf msg = STRBUF_INIT;
> + char *locations[2][2] = {{"locate ref \%s", "local branch \%s"},
> + {"remote ref \%s", "remote branch \%s from \%s"}};
> + char *location;
Use 'const char *'. You can probably drop the hard-coded array dimensions.
These strings ought to be internationalized, as in the original code,
thus they should be wrapped with N_().
> if (remote_is_branch
> && !strcmp(local, shortname)
> @@ -77,30 +81,17 @@ 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
> - die("BUG: impossible combination of %d and %p",
> - remote_is_branch, origin);
> + location = locations[origin != NULL][remote_is_branch];
On this project, it is more idiomatic to say !!origin than 'origin != NULL'.
If we take the time to trace through the code, we can see that
remote_is_branch is indeed either 0 or 1, thus this expression is safe
today, however, if the implementation of starts_with() ever changes so
that it returns a value other than 1 for true, then this code will
break. To avoid such breakage, and to avoid placing burden of tracing
code, you might instead write the expression as:
location = locations[!!origin][!!remote_is_branch];
> + strbuf_addstr(&msg, "Branch \%s set up to track ");
> + strbuf_addstr(&msg, location);
> + if(rebasing)
> + strbuf_addstr(&msg, " by rebasing");
> + strbuf_addch(&msg, '.');
This approach of composing strings is problematic for translation,
which is why the GSoC microproject states:
Don't forget that the strangely-named function _() is used for
internationalization and limits the possibility of gluing
strings together.
For further details about why this approach is undesirable, see this
other email thread [1].
[1]: http://thread.gmane.org/gmane.comp.version-control.git/243793
> + printf_ln(_(msg.buf), local, remote_is_branch ? remote: shortname, origin);
gettext function _() expects constant strings which it can present to
(human) translators, so passing it a buffer containing a string
composed at run-time will not work.
> }
> + strbuf_release(&msg);
> }
>
> /*
> --
> 1.9.0
--
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