Thanks for the submission. Comments below to give you a taste of the
Git review process...
On Mon, Mar 17, 2014 at 5:55 AM, Aleksey Mokhovikov <moxobu...@gmail.com> wrote:
> Subject: [GSOC] Selection of the verbose message is replaced with generated
> message in install_branch_config()
Mentioning [GSoC] in the subject is indeed a good idea.
The subject should be concise. Try to keep it at 65-70 characters or
less. More detailed information can be written following the subject
(separated from the subject by a blank line).
Write in imperative tone: say "replace X with Y" rather than "X is
replaced with Y".
Mention the module or function you're touching.
You might say something like this:
Subject: install_branch_config: replace if-chain with string composition
(But read below since that's not what you really want to do...)
> This is a milliproject from git google summer of code page. The current code
> that selects the output message is quite easy to understand. So I tried to
> improve it by removing nested conditions and code duplication. The output
> string is generated by selecting the proper parts of the message and
> concatenating them the into one template string.
Wrap lines to 65-70 characters.
I suspect you meant "not quite easy" rather than "quite easy".
This prose is almost pure email commentary. It doesn't really convey
useful information to a person reading the patch months or years from
now. Place commentary below the "---" line under your sign-off.
Matthieu already pointed you at [1] which explains why this approach
of composing the strings is not GNU gettext-friendly, so I'll review
other aspects of the patch.
[1]: http://thread.gmane.org/gmane.comp.version-control.git/244210
> Signed-off-by: Aleksey Mokhovikov <moxobu...@gmail.com>
> ---
> branch.c | 39 ++++++++++++++++-----------------------
> 1 file changed, 16 insertions(+), 23 deletions(-)
>
> diff --git a/branch.c b/branch.c
> index 723a36b..2ee353f 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -77,29 +77,22 @@ 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);
> + const char *message_template_parts[] = {
> + "Branch %s set up to track",
> + origin ? " remote" : " local",
> + remote_is_branch ? " branch %s" : " ref %s",
> + (remote_is_branch && origin) ? " from %s" : "",
> + rebasing ? " by rebasing." : "."};
For portability, this project is still mostly restricted to C89, so
these non-constant C99 initializer expressions are probably a no-go.
> + struct strbuf message_template = STRBUF_INIT;
> + size_t i = 0;
> +
> + for (i = 0; i < sizeof(message_template_parts)/sizeof(const
> char *); ++i) {
You can use the ARRAY_SIZE() macro instead of sizeof(...)/sizeof(...).
On this project, i++ is preferred when the context does not
specifically demand ++i.
> + strbuf_addstr(&message_template,
> message_template_parts[i]);
> + }
> +
> + printf_ln(_(message_template.buf), local, remote_is_branch ?
> shortname : remote, origin);
> +
> + strbuf_release(&message_template);
> }
> }
>
> --
> 1.8.3.2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
