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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Mar 13, 2014 at 2:34 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Eric Sunshine <sunshine@xxxxxxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]