Re: [PATCH][GSOC] Selection of the verbose message is replaced with generated message in install_branch_config()
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Mar 17, 2014, 10:53 UTC
- Message-ID
- <CAPig+cQ_gvDWKkgN543TZtfGOqDkH0EYS39w+eEw8CdJO9Ux3Q@mail.gmail.com>
- In-Reply-To
- <1395050104-19152-1-git-send-email-moxobukob@gmail.com>
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 <moxobukob@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
Show 42 quoted lines
> Signed-off-by: Aleksey Mokhovikov <moxobukob@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.
Show 11 quoted lines
> + 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