Re: [PATCH 2/6] sequencer: learn about the special "fake root commit" handling

From: Stefan Beller <sbeller@google.com>
Date: Apr 29, 2018, 21:44 UTC
Message-ID: <CAGZ79kbfOd9r9JGE4qP_=CVnYDbn2A2aY2i+UrttC_0x2xOFuQ@mail.gmail.com>
In-Reply-To: <nycvar.QRO.7.76.6.1804291255010.79@tvgsbejvaqbjf.bet>

Hi Johannes,

thanks for taking your time to explain things. It shows I am not familiar with the rebase code, yet.

Show 5 quoted lines

>
> Having said that, *this* time round, what we need to do is actually very
> similar to what builtin/am.c's read_author_script() does (even if we
> cannot use it as-is: it populates part of a `struct am_state`). I'll have
> to look into this more closely.

Heh, so my rambling was worth it. Thanks for looking into that.

Show 8 quoted lines

>> This part could be prefixed with
>>     /* un-escape text: turn \\ into ' and remove single quotes. */
>
> If could be prefixed that way, but it would be incorrect. We never turn \\
> into '. What we do here (and I do not want to repeat in a comment what the
> code does): we dequote what we previously quoted using single quotes. So
> we use the fact that we know that the value is of the form 'abc', or if it
> contains single quotes: 'this has '\''single'\'' quotes'.

Is there a helper 'dequote' somewhere?

Show 7 quoted lines

>> > +/* Does this command create a (non-merge) commit? */
>> > +static int is_pick_or_similar(enum todo_command command)
>> > +{
>> > +       return command <= TODO_SQUASH;
>> > +}
>>
>> This code looks scary.

[...]

> The code already does things like that, by testing `command <
> TODO_COMMENT`.

ok great. So that is a widely used pattern for enum todo_command, such that rearranging their order would break a lot. (Hence people will find out quickly when doing so).

> But I guess your concerns would go away if I made this a big honking
> switch() statement that lists *explicitly* what should be considered "pick
> or similar"?

I did not spell that out as producing lots of lines of code is not the primary goal, but understandability is.

And it looked like it would just pick the first section, so I thought about some different approaches, either by making the enum todo_command a lot more complex than an enum (an array of structs?) or adding a new parallel array, that contains additional information for each value at the given index, e.g.

static int is_pick_or_similar(enum todo_command command)
{
    return todo_command_flags[value] & TODO_CMDS_IS_PICKING;
}

but all approaches I had were more complicated, such that the additional verbosity would not be enough of a trade off.

I think this function was only scary as I was not familiar with the rebase code as that employs similar patterns already.

Show 15 quoted lines

>> > +                       if (is_fixup(command))
>> > +                               return error(_("cannot fixup root commit"));
>>
>> I would expect you also cannot squash into root commit?
>
> In sequencer.c, `is_fixup()` really means "is it a fixup or a squash?". So
> yes, you are correct that we also cannot squash into a root commit.
>
> However, a squash is the same as a fixup with the only difference being
> that the squash lets you edit the final commit message (and does not
> comment out the squash commit's message, in contrast to fixup).
>
> Is it really worth adding an ugly line break in the middle of the error()
> statement just to say "cannot fixup/squash into root commit"? I'd rather
> not.

Makes sense,

Thanks for your patience, Stefan