[PATCH v4] install_branch_config: simplify verbose messages logic

[PATCH v4] install_branch_config: simplify verbose messages logic

[Paweł Wawruch]

Replace the chain of if statements with table of strings.

Signed-off-by: Paweł Wawruch <pawlo@aleg.pl>
---
The changes proposed by Junio C Hamano:
Improvement of indentations. Removed an unused variable.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/243502
[2]: http://thread.gmane.org/gmane.comp.version-control.git/243849
[3]: http://thread.gmane.org/gmane.comp.version-control.git/243865

 branch.c | 41 +++++++++++++++++++----------------------
 1 file changed, 19 insertions(+), 22 deletions(-)

diff --git a/branch.c b/branch.c
index 723a36b..2a4b911 100644
--- a/branch.c
+++ b/branch.c
@@ -53,6 +53,20 @@ void install_branch_config(int flag, const char *local, const char *origin, cons
 	int remote_is_branch = starts_with(remote, "refs/heads/");
 	struct strbuf key = STRBUF_INIT;
 	int rebasing = should_setup_rebase(origin);
+	const char *message[][2][2] = {{{
+		N_("Branch %s set up to track remote branch %s from %s by rebasing."),
+		N_("Branch %s set up to track remote branch %s from %s."),
+		},{
+		N_("Branch %s set up to track local branch %s by rebasing."),
+		N_("Branch %s set up to track local branch %s."),
+		}},{{
+		N_("Branch %s set up to track remote ref %s by rebasing."),
+		N_("Branch %s set up to track remote ref %s."),
+		},{
+		N_("Branch %s set up to track local ref %s by rebasing."),
+		N_("Branch %s set up to track local ref %s.")
+	}}};
+	const char *name = remote_is_branch ? remote : shortname;
 
 	if (remote_is_branch
 	    && !strcmp(local, shortname)
@@ -77,29 +91,12 @@ 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 (origin && remote_is_branch)
+			printf_ln(_(message[!remote_is_branch][!origin][!rebasing]),
+				local, name, origin);
 		else
-			die("BUG: impossible combination of %d and %p",
-			    remote_is_branch, origin);
+			printf_ln(_(message[!remote_is_branch][!origin][!rebasing]),
+				local, name);
 	}
 }
 
-- 
1.8.3.2

Re: [PATCH v4] install_branch_config: simplify verbose messages logic

[Eric Sunshine]

On Tue, Mar 11, 2014 at 8:33 PM, Paweł Wawruch <pawlo@aleg.pl> wrote:
> Replace the chain of if statements with table of strings.
>
> Signed-off-by: Paweł Wawruch <pawlo@aleg.pl>
> ---
> The changes proposed by Junio C Hamano:
> Improvement of indentations. Removed an unused variable.

Better, thanks. More below.

> [1]: http://thread.gmane.org/gmane.comp.version-control.git/243502
> [2]: http://thread.gmane.org/gmane.comp.version-control.git/243849
> [3]: http://thread.gmane.org/gmane.comp.version-control.git/243865

The [n]: notation typically is used for footnotes which you reference
in the running text as [n] (or sometimes [*n*], depending upon
preference). If you're simply listing links to previous attempts, it
would be less confusing to say:

v3: http://...
v2: http://...
v1: http://...

>  branch.c | 41 +++++++++++++++++++----------------------
>  1 file changed, 19 insertions(+), 22 deletions(-)
>
> diff --git a/branch.c b/branch.c
> index 723a36b..2a4b911 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -53,6 +53,20 @@ void install_branch_config(int flag, const char *local, const char *origin, cons
>         int remote_is_branch = starts_with(remote, "refs/heads/");
>         struct strbuf key = STRBUF_INIT;
>         int rebasing = should_setup_rebase(origin);
> +       const char *message[][2][2] = {{{
> +               N_("Branch %s set up to track remote branch %s from %s by rebasing."),
> +               N_("Branch %s set up to track remote branch %s from %s."),
> +               },{
> +               N_("Branch %s set up to track local branch %s by rebasing."),
> +               N_("Branch %s set up to track local branch %s."),
> +               }},{{
> +               N_("Branch %s set up to track remote ref %s by rebasing."),
> +               N_("Branch %s set up to track remote ref %s."),
> +               },{
> +               N_("Branch %s set up to track local ref %s by rebasing."),
> +               N_("Branch %s set up to track local ref %s.")
> +       }}};
> +       const char *name = remote_is_branch ? remote : shortname;
>
>         if (remote_is_branch
>             && !strcmp(local, shortname)
> @@ -77,29 +91,12 @@ 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 (origin && remote_is_branch)
> +                       printf_ln(_(message[!remote_is_branch][!origin][!rebasing]),
> +                               local, name, origin);
>                 else
> -                       die("BUG: impossible combination of %d and %p",
> -                           remote_is_branch, origin);
> +                       printf_ln(_(message[!remote_is_branch][!origin][!rebasing]),
> +                               local, name);

Shouldn't this logic also be encoded in the table? After all, the
point of making the code table-driven is so that such hard-coded logic
can be avoided. It shouldn't be difficult to do.

The same argument also applies to computation of the 'name' variable
above. It too can be pushed into the the table.

>         }
>  }
>
> --
> 1.8.3.2

Re: [PATCH v4] install_branch_config: simplify verbose messages logic

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> +               if (origin && remote_is_branch)
>> +                       printf_ln(_(message[!remote_is_branch][!origin][!rebasing]),
>> +                               local, name, origin);
>>                 else
>> -                       die("BUG: impossible combination of %d and %p",
>> -                           remote_is_branch, origin);
>> +                       printf_ln(_(message[!remote_is_branch][!origin][!rebasing]),
>> +                               local, name);
>
> Shouldn't this logic also be encoded in the table? After all, the
> point of making the code table-driven is so that such hard-coded logic
> can be avoided. It shouldn't be difficult to do.

Hmph.  Is it even necessary in the first place?  Does it hurt if you
give more parameters than the number of placeholders in the format
string?

Re: [PATCH v4] install_branch_config: simplify verbose messages logic

[Eric Sunshine]

On Wed, Mar 12, 2014 at 6:02 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>>> +               if (origin && remote_is_branch)
>>> +                       printf_ln(_(message[!remote_is_branch][!origin][!rebasing]),
>>> +                               local, name, origin);
>>>                 else
>>> -                       die("BUG: impossible combination of %d and %p",
>>> -                           remote_is_branch, origin);
>>> +                       printf_ln(_(message[!remote_is_branch][!origin][!rebasing]),
>>> +                               local, name);
>>
>> Shouldn't this logic also be encoded in the table? After all, the
>> point of making the code table-driven is so that such hard-coded logic
>> can be avoided. It shouldn't be difficult to do.
>
> Hmph.  Is it even necessary in the first place?  Does it hurt if you
> give more parameters than the number of placeholders in the format
> string?

It's not necessary, which is why my question was posed: as a clue. By
asking GSoC applicants to think about how the logic can be
table-driven, it is hoped that they will arrive at the realization
that it is okay to pass in more parameters than placeholders. With
that idea in mind, the code can be simplified whether table-driven or
not.

(I suspect Michael had this in mind when he composed this GSoC
microproject and asked students to consider if it would make sense to
make the code table-driven.)

Re: [PATCH v4] install_branch_config: simplify verbose messages logic

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> Shouldn't this logic [to decide what the printf arguments should
> be] also be encoded in the table?
> ...
> The same argument also applies to computation of the 'name' variable
> above. It too can be pushed into the the table.

Because the "printf argument" logic does not have to be in the
table, the same argument does not apply to the 'name' thing.

After looking at the v5 patch, I do not think an extra two-element
array to switch between remote vs shortname is making it any easier
to read.  I would have to say that personally I find that

	const char *name[] = {remote, shortname};
	... long swath of code ...
	printf_ln(... name[!remote_is_branch] ...);

is a lot harder to read than:

	printf_ln(... remote_is_branch ? shortname : branch ...);

HTH, and thanks.

Re: [PATCH v4] install_branch_config: simplify verbose messages logic

[Eric Sunshine]

On Thu, Mar 13, 2014 at 2:34 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> Shouldn't this logic [to decide what the printf arguments should
>> be] also be encoded in the table?
>> ...
>> The same argument also applies to computation of the 'name' variable
>> above. It too can be pushed into the the table.
>
> Because the "printf argument" logic does not have to be in the
> table, the same argument does not apply to the 'name' thing.
>
> After looking at the v5 patch, I do not think an extra two-element
> array to switch between remote vs shortname is making it any easier
> to read.  I would have to say that personally I find that
>
>         const char *name[] = {remote, shortname};
>         ... long swath of code ...
>         printf_ln(... name[!remote_is_branch] ...);
>
> is a lot harder to read than:
>
>         printf_ln(... remote_is_branch ? shortname : branch ...);

Indeed, that's a step backward, and is not what was asked. Merely
pushing data into tables does not make the logic table-driven
(emphasis on *driven*). The GSoC microproject did not demand a
table-driven approach, but instead asked students if such an approach
would make sense. A more table-driven approach might look something
like this:

    struct M { const char *s; const char **a1; const char **a2; }
    message[][2][2] = {{{
        { "Branch %s set ... %s ... %s", &shortname, &origin },
        ...
    }},{{
        { "Branch %s set ... %s", &remote, NULL },
        ...
    }}};

    const struct M *m = message[!remote_is_branch][!origin][!rebasing];
    printf_ln(m->s, local, *m->a1, *m->a2);

Whether this approach is more clear than the original code is a matter
for debate [1], however, it's obvious from the table which arguments
belong with each message, and the printf_ln() invocation does not
require any logic. When moving only messages into a table, they become
disconnected from their arguments which makes reasoning about them a
bit more difficult. The original code does not have this problem, nor
does a table-driven approach.

[1]: While ungainly, the original code may not be sufficiently bad to
warrant the extra complications of a table. A simple refactoring, such
as [2], can make the code a bit easier to read without adding
complexity.

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

Re: [PATCH v4] install_branch_config: simplify verbose messages logic

[Eric Sunshine]

On Thu, Mar 13, 2014 at 4:35 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> A more table-driven approach might look something
> like this:
>
>     struct M { const char *s; const char **a1; const char **a2; }
>     message[][2][2] = {{{
>         { "Branch %s set ... %s ... %s", &shortname, &origin },
>         ...
>     }},{{
>         { "Branch %s set ... %s", &remote, NULL },
>         ...
>     }}};
>
>     const struct M *m = message[!remote_is_branch][!origin][!rebasing];
>     printf_ln(m->s, local, *m->a1, *m->a2);

Of course, using NULL in the table like that would crash when
dereferenced: *m->a2. It was just an quick example typed on-the-fly.
Real code would want to be more careful.