[PATCH][GSOC] Selection of the verbose message is replaced with generated message in install_branch_config()

[PATCH][GSOC] Selection of the verbose message is replaced with generated message in install_branch_config()

[Aleksey Mokhovikov]

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 <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." : "."};
+		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

Re: [PATCH][GSOC] Selection of the verbose message is replaced with generated message in install_branch_config()

[Matthieu Moy]

Hi,

Aleksey Mokhovikov <moxobukob@gmail.com> writes:

> 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.

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

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH][GSOC] Selection of the verbose message is replaced with generated message in install_branch_config()

[Eric Sunshine]

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

> 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.

> +                       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

Re: [PATCH][GSOC] Selection of the verbose message is replaced with generated message in install_branch_config()

[Aleksey Mokhovikov]

This patch replaces if chain that selects the message with
2 dimensional array of format strings and arguments.


Signed-off-by: Aleksey Mokhovikov <moxobukob@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, remote);
+		if ((0 <= message_id) && (message_id < 
ARRAY_SIZE(message_table)))
+			message = message_table[message_id];
 		else
-			die("BUG: impossible combination of %d and %p",
-			    remote_is_branch, origin);
+			die("BUG: id is out of range:id=%d (is_branch=%d, 
origin=%p, rebasing=%d)",
+			    message_id, remote_is_branch, origin, rebasing);
+
+		if (!message || !message[0])
+			die("BUG: message is NULL. Fix verbose message 
table.");
+
+		printf_ln(_(message[0]), message[1], message[2], 
message[3]);
 	}
 }
 
-- 
1.8.3.2

Re: [PATCH][GSOC] Selection of the verbose message is replaced with generated message in install_branch_config()

[Aleksey Mokhovikov]

This patch replaces if chain with
2 dimensional array of format strings and arguments.


Signed-off-by: Aleksey Mokhovikov <moxobukob@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);
+		if ((0 <= message_id) && (message_id < ARRAY_SIZE(message_table)))
+			message = message_table[message_id];
 		else
-			die("BUG: impossible combination of %d and %p",
-			    remote_is_branch, origin);
+			die("BUG: id is out of range:id=%d (is_branch=%d, origin=%p, rebasing=%d)",
+			    message_id, remote_is_branch, origin, rebasing);
+
+		if (!message || !message[0])
+			die("BUG: message is NULL. Fix verbose message table.");
+
+		printf_ln(_(message[0]), message[1], message[2], message[3]);
 	}
 }

-- 
1.8.3.2

Re: [PATCH][GSOC] Selection of the verbose message is replaced with generated message in install_branch_config()

[Aleksey Mokhovikov]

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

Re: [PATCH][GSOC] Selection of the verbose message is replaced with generated message in install_branch_config()

[Aleksey Mokhovikov]

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

Re: [PATCH][GSOC] Selection of the verbose message is replaced with generated message in install_branch_config()

[Eric Sunshine]

Thanks for the resubmission. Comments below...

On Tue, Mar 18, 2014 at 10:33 AM, Aleksey Mokhovikov
<moxobukob@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

> This patch replaces if chain with
> 2 dimensional array of format strings and arguments.

This _is_ a patch, so no need to say so explicitly. Use imperative
voice. You might say:

    Replace if-chain, which selects message in verbose mode, with
    table-driven approach.

> Signed-off-by: Aleksey Mokhovikov <moxobukob@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.

Keeping it in the same thread provides continuity which can help
reviewers. Providing a link to the previous attempt, like this [1], is
also courteous to reviewers.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/244237

> 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?

> 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?

> 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.

Indeed, it's a common and understandable misconception.

>  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;
>  }
>
> +

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.)

> +       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.

> +       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);
> -
> +

Unnecessary whitespace change.

>         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);
> +               if ((0 <= message_id) && (message_id < ARRAY_SIZE(message_table)))

Unnecessary parentheses can make the code a bit more difficult to
read, thus should be dropped.

> +                       message = message_table[message_id];
>                 else
> -                       die("BUG: impossible combination of %d and %p",
> -                           remote_is_branch, origin);
> +                       die("BUG: id is out of range:id=%d (is_branch=%d, origin=%p, rebasing=%d)",
> +                           message_id, remote_is_branch, origin, rebasing);

As this is indicates a programmer error, assert() may be more
appropriate, and placed as near the computation of message_id as
possible. The original code used die() because it didn't detect the
error until the end of the if-chain, but you can detect it as soon as
you compute message_id.

> +               if (!message || !message[0])
> +                       die("BUG: message is NULL. Fix verbose message table.");

Meh. May be overkill. If either of these is NULL, you'll get a crash
anyhow, which is a good indication of a bug.

On the other hand, asserting that message_id is in range is a good
idea since an out-of-bounds array access won't necessarily result in a
crash, thus such a bug could go undetected for some time.

> +               printf_ln(_(message[0]), message[1], message[2], message[3]);
>         }
>  }
>
> --
> 1.8.3.2

Re: [PATCH][GSOC] Selection of the verbose message is replaced with generated message in install_branch_config()

[Aleksey Mokhovikov]

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
> <moxobukob@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 here will be no problems with standard.

>> +                       message = message_table[message_id];
>>                 else
>> -                       die("BUG: impossible combination of %d and %p",
>> -                           remote_is_branch, origin);
>> +                       die("BUG: id is out of range:id=%d (is_branch=%d, origin=%p, rebasing=%d)",
>> +                           message_id, remote_is_branch, origin, rebasing);
> 
> As this is indicates a programmer error, assert() may be more
> appropriate, and placed as near the computation of message_id as
> possible. The original code used die() because it didn't detect the
> error until the end of the if-chain, but you can detect it as soon as
> you compute message_id.

Thanks. At first I wrote assert, but decided to leave die because if was there before.


>> +               if (!message || !message[0])
>> +                       die("BUG: message is NULL. Fix verbose message table.");
> 
> Meh. May be overkill. If either of these is NULL, you'll get a crash
> anyhow, which is a good indication of a bug.

If either of them is NULL it can be UB. And if message[0] is NULL, it's quite likely. that
printf will just skip it, so I think it's better to assert message[0].

Re: [PATCH][GSOC] Selection of the verbose message is replaced with generated message in install_branch_config()

[Eric Sunshine]

On Thu, Mar 20, 2014 at 7:56 AM, Aleksey Mokhovikov <moxobukob@gmail.com> wrote:
> 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
>> <moxobukob@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.

The --in-reply-to option of "git send-email" may be useful. It's
important to satisfy the requirements of patch correctness; and
probably not worth worrying about satisfying Gmane, or any of the
other services archiving the mailing list, which are beyond our
control.

>>> 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.

Interesting. A quick grep shows that most !! are used on pointer
values but some are used with arithmetic constructs (mentioned in the
excerpt). In the context of this type of patch where you need to
restrict those values to 0 or 1 before multiplying or shifting, !!
might qualify for the "unless there is a compelling reason" exception.

>>>  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);

Verbosity doesn't necessarily equal clarity. (1 << n) is a standard
idiom for setting the n'th bit, and should be well understood. The
macro usage also doesn't reduce the cognitive load since the reader
still has to puzzle out how 'bool' and 'bitflag' relate to table
positions.

(But that's only my opinion, and as part of the review process, you
are welcome to present yours. And, as it's Junio's tree into which
patches will be accepted -- or not -- he has final say-so.)

>>> +       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 here will be no problems with standard.

I was referring to the variables 'local', 'remote', 'shortname',
'origin', all of which are, um, variable (non-constant).

>>> +               if (!message || !message[0])
>>> +                       die("BUG: message is NULL. Fix verbose message table.");
>>
>> Meh. May be overkill. If either of these is NULL, you'll get a crash
>> anyhow, which is a good indication of a bug.
>
> If either of them is NULL it can be UB. And if message[0] is NULL, it's quite likely. that
> printf will just skip it, so I think it's better to assert message[0].

Sorry, you're right about message[0] case not being a crasher (though
the assert() still seems overkill).

Re: [PATCH][GSOC] Selection of the verbose message is replaced with generated message in install_branch_config()

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> Sorry, you're right about message[0] case not being a crasher (though
> the assert() still seems overkill).

Assert() often becomes no-op in production build.  I think this may
be an indication that "table-driven" may not be as good an approach
as many candidates thought.  The microproject suggestion asks them
to think _if_ that makes sense, and it is perfectly fine for them if
they answer "no, it introduces more problems than it solves".

Re: [PATCH][GSOC] Selection of the verbose message is replaced with generated message in install_branch_config()

[Michael Haggerty]

On 03/21/2014 06:09 PM, Junio C Hamano wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
>> Sorry, you're right about message[0] case not being a crasher (though
>> the assert() still seems overkill).
> 
> Assert() often becomes no-op in production build.  I think this may
> be an indication that "table-driven" may not be as good an approach
> as many candidates thought.  The microproject suggestion asks them
> to think _if_ that makes sense, and it is perfectly fine for them if
> they answer "no, it introduces more problems than it solves".

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 "if"s, 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

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH][GSOC] Selection of the verbose message is replaced with generated message in install_branch_config()

[Eric Sunshine]

On Fri, Mar 21, 2014 at 5:13 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 03/21/2014 06:09 PM, Junio C Hamano wrote:
>> Eric Sunshine <sunshine@sunshineco.com> writes:
>>
>>> Sorry, you're right about message[0] case not being a crasher (though
>>> the assert() still seems overkill).
>>
>> Assert() often becomes no-op in production build.  I think this may
>> be an indication that "table-driven" may not be as good an approach
>> as many candidates thought.  The microproject suggestion asks them
>> to think _if_ that makes sense, and it is perfectly fine for them if
>> they answer "no, it introduces more problems than it solves".
>
> 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 "if"s, 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).

Adam NoLastName did stick with the 'if' statements and removed the
unreachable branch  [1], although he didn't say if he had considered
the table-driven approach and discarded it.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/243704

> The multidimensional array lookup table is not so terrible, but I
> personally still prefer the "if".
>
> Michael
>
> --
> Michael Haggerty
> mhagger@alum.mit.edu
> http://softwareswirl.blogspot.com/

Re: [PATCH][GSOC] Selection of the verbose message is replaced with generated message in install_branch_config()

[Michael Haggerty]

On 03/21/2014 10:33 PM, Eric Sunshine wrote:
> On Fri, Mar 21, 2014 at 5:13 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> On 03/21/2014 06:09 PM, Junio C Hamano wrote:
>>> Assert() often becomes no-op in production build.  I think this may
>>> be an indication that "table-driven" may not be as good an approach
>>> as many candidates thought.  The microproject suggestion asks them
>>> to think _if_ that makes sense, and it is perfectly fine for them if
>>> they answer "no, it introduces more problems than it solves".
>>
>> 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 "if"s, 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).
> 
> Adam NoLastName did stick with the 'if' statements and removed the
> unreachable branch  [1], although he didn't say if he had considered
> the table-driven approach and discarded it.
> 
> [1]: http://thread.gmane.org/gmane.comp.version-control.git/243704

Thanks for correcting my oversight.

So...there's still hope for my sanity after all :-)  Adam must have
tasted from the tree of wisdom :-)

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH][GSOC] Selection of the verbose message is replaced with generated message in install_branch_config()

[Aleksey Mokhovikov]

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 "if"s, 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.

Re: [PATCH][GSOC] Selection of the verbose message is replaced with generated message in install_branch_config()

[Michael Haggerty]

On 03/24/2014 08:28 AM, Aleksey Mokhovikov wrote:
> 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 "if"s, 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".
> 
> 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.

Sorry if there was a misunderstanding.  I didn't mean to criticize you
and certainly not to single you out among the many students who went for
a table-driven solution.  I was rather replying to Junio's general
comment, that maybe changing the code to be table-driven wasn't such a
good idea.  These things are always a matter a taste, and sometimes hard
to predict before one has tried writing the code a couple of different ways.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/