Re: [PATCH][GSOC] Selection of the verbose message is replaced with generated message in install_branch_config()
On 03/22/2014 04:13 AM, Michael Haggerty wrote: My expectation when I invented that microproject was that converting the code to be table-driven would be judged *not* to be an improvement. I was hoping that a student would say the 'if' statement is OK, but let's delete this ridiculous unreachable else branch. Possibly they would convert the if chain into nested ifs, which I think would allow some code consolidation in one of the branches. But not a single student agreed with me, so I must be in a minority of one (which, unfortunately, is the definition of lunacy). The multidimensional array lookup table is not so terrible, but I personally still prefer the if. Michael That was expectable. But the main goal for me was to participate in git development process, to become familiar with it. It looks hard to participate when not proposing a patch. I thought about make a small change in if chain, but it looked to minor to feel whole development process. I've used git features for formatting and sending a patch to mailing list. I've met the GNU gettext restrictions when proposed a first patch. Proposed another patch and tried to show Pros and Cons. It didn't look like applying a patch to git master branch was the main goal. As for me that was quite interesting and useful. -- 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
Re: [PATCH][GSOC] Selection of the verbose message is replaced with generated message in install_branch_config()
On 03/19/2014 04:21 PM, Eric Sunshine wrote:
Thanks for the resubmission. Comments below...
On Tue, Mar 18, 2014 at 10:33 AM, Aleksey Mokhovikov
moxobu...@gmail.com wrote:
Subject: [PATCH][GSOC] Selection of the verbose message is replaced with
generated message in install_branch_config()
Say [PATCH v2] to indicate your second attempt.
This subject is quite long. Try to keep it short but informative.
Mention the module or function first. Use imperative voice. You might
say:
Subject: install_branch_config: replace if-chain with table lookup
Thanks. This is my first experience with such newsgroups. I don't know
explicitly how this mail-nntp newsgroups work
under the hood, so I was afraid, that if I'll change the subject, gmane will
create a new thread instead of placing a
comment.
Compare with original construction
Pros:
1) Remove if chain.
2) Single table contains all messages with arguments in one construction.
3) Less code duplication.
Cons:
1) Harder to associate the case with message.
2) Harder for compiler to warn the programmer about not
enough arguments for format string.
3) Harder to add non-string argument to messages.
Nice summary. Do you draw any conclusions from it? Is the new code
better? Easier to understand? Would it be better merely to refactor
the 'if' statements a bit instead of using a table?
I think if that code is heavily developed at this time, then if chain is better,
because if construction is more simple and looks more flexible to major changes.
And if there is no plans to make huge but minor changes,
then table driven approach looks better for me. So I would wrote the if chain at
first, and later, If I had to change verbose message or something similar,
I could rewrite it.
If !!(x) is out of the coding guide, then message_id construction
can be rewritten in several other ways.
The guideline tells that !!(x) is not welcome and should not be
unless needed. But looks like this is normal place for it.
Curious. !!x is indeed used regularly. Where did you read that it is
not welcome?
In git/Documentation/CodingGuidelines:
170 - Some clever tricks, like using the !! operator with arithmetic
171 constructs, can be extremely confusing to others. Avoid them,
172 unless there is a compelling reason to use them.
Unnecessary blank line insertion. This adds noise to the patch which
distracts from the real changes.
void install_branch_config(int flag, const char *local, const char *origin,
const char *remote)
{
const char *shortname = remote + 11;
int remote_is_branch = starts_with(remote, refs/heads/);
struct strbuf key = STRBUF_INIT;
int rebasing = should_setup_rebase(origin);
+ int message_id = (!!remote_is_branch 2) | (!!origin 1) |
(!!rebasing);
It works, but it's also a fairly magical incantation, placing greater
cognitive demands on the reader. You could achieve a similar result
with a multi-dimensional table which is indexed directly by
!!remote_is_branch, !!origin, and !!rebasing. (Whether you actually
want to do so is a different question.)
I thought about a multidimensional table and about this approach before
submitting a patch
and it looks easier for me to read without multidimensional table. But I
mentioned that indexing can be rewritten
several ways. It will be even easier if the named function or macro is used:
#define BOOL_TO_BITFLAG(x, shift) (!!(x) (shift))
...
int message_id = BOOL_TO_BITFLAG(remote_is_branch, 2) | BOOL_TO_BITFLAG(origin,
1) | BOOL_TO_BITFLAG(rebasing, 0);
+ const char *message_table[][4] =
+ {{N_(Branch %s set up to track local ref %s.),
+ local, remote},
+{N_(Branch %s set up to track local ref %s by rebasing.),
+ local, remote},
+{N_(Branch %s set up to track remote ref %s.),
+ local, remote},
+{N_(Branch %s set up to track remote ref %s by rebasing.),
+ local, remote},
+{N_(Branch %s set up to track local branch %s.),
+ local, shortname},
+{N_(Branch %s set up to track local branch %s by
rebasing.),
+ local, shortname},
+{N_(Branch %s set up to track remote branch %s from %s.),
+ local, shortname, origin},
+{N_(Branch %s set up to track remote branch %s from %s by
rebasing.),
+ local, shortname, origin}};
Indeed, this is a reasonably decent way to keep the arguments and
messages together and can simplify the code nicely. Unfortunately,
this project is still restricted primarily to C89, so using
non-constant C99 initializers is likely to prevent the patch from
being accepted.
Strange. This is not a static variable. N_(x) is expanded to x - is just a
marker for xgetext.
array dimensions are known on compile time. Thought
Re: [PATCH][GSOC] Selection of the verbose message is replaced with generated message in install_branch_config()
This patch replaces if chain that selects the message with
2 dimensional array of format strings and arguments.
Signed-off-by: Aleksey Mokhovikov moxobu...@gmail.com
---
This patch is unrelated with previous one, but related to GSoC.
So I don't know if I should create new thread for this patch.
Compare with original construction
Pros:
1) Remove if chain.
2) Single table contains all messages with arguments in one contruction.
3) Less code duplication.
Cons:
1) Harder to associate the case with message.
2) Harder for compiler to warn the programmer about not
enough arguments for format string.
3) Harder to add non-string argument to messages.
If !!(x) is out of the coding guide, then message_id construction
can be rewritten in several other ways.
The guideline tells that !!(x) is not welcome and should not be
unless needed. But looks like this is normal place for it.
P.S.
Thanks to comments I got, so
I've read more GNU gettext manual to get info
about translation workflow. When I posted a first patch
I've passed the same message format strings to gettext /*_()*/
as in original, to save the context of the message. And they
will be translated if every possible string combination
will be marked separately for translation. Because of
xgettext can extract string from source automatically,
it ruins the idea to generate message from parts. Even if the
exaclty same message format string is passed to gettext.
branch.c | 53 ++---
1 file changed, 30 insertions(+), 23 deletions(-)
diff --git a/branch.c b/branch.c
index 723a36b..51a5faf 100644
--- a/branch.c
+++ b/branch.c
@@ -47,12 +47,32 @@ static int should_setup_rebase(const char *origin)
return 0;
}
+
void install_branch_config(int flag, const char *local, const char *origin,
const char *remote)
{
const char *shortname = remote + 11;
int remote_is_branch = starts_with(remote, refs/heads/);
struct strbuf key = STRBUF_INIT;
int rebasing = should_setup_rebase(origin);
+ int message_id = (!!remote_is_branch 2) | (!!origin 1) |
(!!rebasing);
+ const char *message_table[][4] =
+ {{N_(Branch %s set up to track local ref %s.),
+ local, remote},
+{N_(Branch %s set up to track local ref %s by rebasing.),
+ local, remote},
+{N_(Branch %s set up to track remote ref %s.),
+ local, remote},
+{N_(Branch %s set up to track remote ref %s by
rebasing.),
+ local, remote},
+{N_(Branch %s set up to track local branch %s.),
+ local, shortname},
+{N_(Branch %s set up to track local branch %s by
rebasing.),
+ local, shortname},
+{N_(Branch %s set up to track remote branch %s from %s.),
+ local, shortname, origin},
+{N_(Branch %s set up to track remote branch %s from %s by
rebasing.),
+ local, shortname, origin}};
+ const char **message = NULL;
if (remote_is_branch
!strcmp(local, shortname)
@@ -68,7 +88,7 @@ void install_branch_config(int flag, const char *local,
const char *origin, cons
strbuf_reset(key);
strbuf_addf(key, branch.%s.merge, local);
git_config_set(key.buf, remote);
-
+
if (rebasing) {
strbuf_reset(key);
strbuf_addf(key, branch.%s.rebase, local);
@@ -77,29 +97,16 @@ 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
Re: [PATCH][GSOC] Selection of the verbose message is replaced with generated message in install_branch_config()
Eric Sunshine sunshine at sunshineco.com writes: 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 Wrap lines to 65-70 characters. 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. Thanks a lot for you language and message formatting style advices. I've make a new patch taking into account the GNU gettext requirements. I don't know if I should create a new thread for another patch, but I'd be glad if you will give me some information about new patch: http://permalink.gmane.org/gmane.comp.version-control.git/244357 -- 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
Re: [PATCH][GSOC] Selection of the verbose message is replaced with generated message in install_branch_config()
This patch replaces if chain with
2 dimensional array of format strings and arguments.
Signed-off-by: Aleksey Mokhovikov moxobu...@gmail.com
---
This patch is unrelated with previous one, but related to GSoC.
So I don't know if I should create new thread for this patch.
Compare with original construction
Pros:
1) Remove if chain.
2) Single table contains all messages with arguments in one construction.
3) Less code duplication.
Cons:
1) Harder to associate the case with message.
2) Harder for compiler to warn the programmer about not
enough arguments for format string.
3) Harder to add non-string argument to messages.
If !!(x) is out of the coding guide, then message_id construction
can be rewritten in several other ways.
The guideline tells that !!(x) is not welcome and should not be
unless needed. But looks like this is normal place for it.
P.S.
Thanks to comments I got, so
I've read more GNU gettext manual to get info
about translation workflow. When I posted a first patch
I've passed the same message format strings to gettext /*_()*/
as in original, to save the context of the message. And they
will be translated if every possible string combination
will be marked separately for translation. Because of
xgettext can extract string from source automatically,
it ruins the idea to generate message from parts. Even if the
exaclty same message format string is passed to gettext.
branch.c | 53 ++---
1 file changed, 30 insertions(+), 23 deletions(-)
diff --git a/branch.c b/branch.c
index 723a36b..51a5faf 100644
--- a/branch.c
+++ b/branch.c
@@ -47,12 +47,32 @@ static int should_setup_rebase(const char *origin)
return 0;
}
+
void install_branch_config(int flag, const char *local, const char *origin,
const char *remote)
{
const char *shortname = remote + 11;
int remote_is_branch = starts_with(remote, refs/heads/);
struct strbuf key = STRBUF_INIT;
int rebasing = should_setup_rebase(origin);
+ int message_id = (!!remote_is_branch 2) | (!!origin 1) |
(!!rebasing);
+ const char *message_table[][4] =
+ {{N_(Branch %s set up to track local ref %s.),
+ local, remote},
+{N_(Branch %s set up to track local ref %s by rebasing.),
+ local, remote},
+{N_(Branch %s set up to track remote ref %s.),
+ local, remote},
+{N_(Branch %s set up to track remote ref %s by rebasing.),
+ local, remote},
+{N_(Branch %s set up to track local branch %s.),
+ local, shortname},
+{N_(Branch %s set up to track local branch %s by rebasing.),
+ local, shortname},
+{N_(Branch %s set up to track remote branch %s from %s.),
+ local, shortname, origin},
+{N_(Branch %s set up to track remote branch %s from %s by
rebasing.),
+ local, shortname, origin}};
+ const char **message = NULL;
if (remote_is_branch
!strcmp(local, shortname)
@@ -68,7 +88,7 @@ void install_branch_config(int flag, const char *local, const
char *origin, cons
strbuf_reset(key);
strbuf_addf(key, branch.%s.merge, local);
git_config_set(key.buf, remote);
-
+
if (rebasing) {
strbuf_reset(key);
strbuf_addf(key, branch.%s.rebase, local);
@@ -77,29 +97,16 @@ 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
Re: [PATCH][GSOC] Selection of the verbose message is replaced with generated message in install_branch_config()
Matthieu Moy Matthieu.Moy at grenoble-inp.fr writes: Hi, Aleksey Mokhovikov moxobukob at gmail.com writes: Please, read the threads for other submissions for this microproject. Most remarks done there also apply for your case. See for example: http://thread.gmane.org/gmane.comp.version-control.git/244210 Thank you for your reply. I've read a bit more GNU gettext manual to get information about translation with GNU gettext. Long story short, the idea to generate message from parts will produce even more problems, despite the message strings passed to the _() are equal before and after the patch. So I decided to make an array with all messages and mark them for translation with N_() to keep them together. Because we now have an array we can improve it to make a table with message format string and arguments. Now we need just to find the proper message index to print the message. Please look at another approach: http://permalink.gmane.org/gmane.comp.version-control.git/244357 -- 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
[PATCH][GSOC] Selection of the verbose message is replaced with generated message in install_branch_config()
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.
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. : .};
+ struct strbuf message_template = STRBUF_INIT;
+ size_t i = 0;
+
+ for (i = 0; i sizeof(message_template_parts)/sizeof(const
char *); ++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
