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 <[email protected]> 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 <[email protected]> > --- > 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
