Re: [PATCH v5 19/44] builtin-am: implement --3way, am.threeWay

[Paul Tan <pyokagan@gmail.com>]

On Wed, Jul 8, 2015 at 4:14 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Paul Tan <pyokagan@gmail.com> writes:
>
>> @@ -82,6 +84,8 @@ struct am_state {
>>       /* number of digits in patch filename */
>>       int prec;
>>
>> +     int threeway;
>> +
>>       int quiet;
>>
>>       int append_signoff;
>
> These "one line surrounded by blank on both sides" starts to get
> irritating, and the structure looksq horrifying after you apply all
> these patches.  Perhaps have a clean-up patch at the end to make
> them look more like this?
>
> struct am_state {
>         /* state directory path */
>         char *dir;
>
>         /* current and last patch numbers, 1-indexed */
>         int cur;
>         int last;
>
>         /* commit metadata and message */
>         char *author_name;
>         char *author_email;
>         char *author_date;
>         char *msg;
>         size_t msg_len;
>
>         /* when --rebasing, records the original commit the patch came from */
>         unsigned char orig_commit[GIT_SHA1_RAWSZ];
>
>         /* number of digits in patch filename */
>         int prec;
>
>         /* various operating modes and command line options */
>         int interactive;
>         int threeway;
>         int quiet;
>         int append_signoff;
>         int utf8;
>         int committer_date_is_author_date;
>         int ignore_date;
>         int allow_rerere_autoupdate;
>         const char *sign_commit;
>         int rebasing;
>
>         /* one of the enum keep_type values */
>         int keep;
>
>         /* pass -m flag to git-mailinfo */
>         int message_id;
>
>         /* one of the enum scissors_type values */
>         int scissors;
>
>         /* used when spawning "git apply" via run_command() */
>         struct argv_array git_apply_opts;
>
>         /* override error message when patch failure occurs */
>         const char *resolvemsg;

Okay. I would prefer to keep the above fields all in the same block
though, as they are all "various operating modes and command line
options". And in retrospect, the comments don't really add much value,
with the exception of the "enum" comments to make it explicit that we
expect an enum value in that field. So, something like this, perhaps?

struct am_state {
    /* state directory path */
    char *dir;

    /* current and last patch numbers, 1-indexed */
    int cur;
    int last;

    /* commit metadata and message */
    char *author_name;
    char *author_email;
    char *author_date;
    char *msg;
    size_t msg_len;

    /* when --rebasing, records the original commit the patch came from */
    unsigned char orig_commit[GIT_SHA1_RAWSZ];

    /* number of digits in patch filename */
    int prec;

    /* various operating modes and command line options */
    int interactive;
    int threeway;
    int quiet;
    int append_signoff;
    int utf8;
    int keep; /* enum keep_type */
    int message_id;
    int scissors; /* enum scissors_type */
    struct argv_array git_apply_opts;
    const char *resolvemsg;
    int committer_date_is_author_date;
    int ignore_date;
    int allow_rerere_autoupdate;
    const char *sign_commit;
    int rebasing;
};

Thanks,
Paul