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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 13, 2014, 20:35 UTC
Message-ID: <CAPig+cQU5__EPA2C3QvCPAT4od3Ux8kCgq-2H8nCddGdNqaG0A@mail.gmail.com>
In-Reply-To: <xmqqr466hrru.fsf@gitster.dls.corp.google.com>

On Thu, Mar 13, 2014 at 2:34 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 22 quoted lines

> 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