Re: [PATCH v2 23/23] rebase -i: enable options --signoff, --reset-author for pick, reword

From: Fabian Ruch <bafain@gmail.com>
Date: Sep 21, 2014, 16:59 UTC
Message-ID: <541F03FC.8040506@gmail.com>
In-Reply-To: <53EB5E77.8010005@alum.mit.edu>

Hi Michael,

On 08/13/2014 02:47 PM, Michael Haggerty wrote:

Show 17 quoted lines

> On 08/07/2014 01:59 AM, Fabian Ruch wrote:
>> pick and reword are atomic to-do list commands in the sense that they
>> open a new task which is closed after the respective command is
>> completed. squash and fixup are not atomic. They create a new task
>> which is not completed until the last squash or fixup is processed.
> 
> I don't understand the distinction that you are attempting to draw
> between "atomic" and "non-atomic" commands.  For example, in the
> following command list:
> 
>     pick 1111111
>     squash 2222222
>     fixup 3333333
> 
> the "pick" command doesn't seem very atomic, because the *end* result of
> the three commands is a single commit that is affected by all three
> commands.

Right, when I wrote the commit message I was thinking in abstract terms so I implicitly thought of your example as a (single) squash/fixup command. Now it has become obvious that I wasn't very thorough with the implementation part. The git-rebase implementation is oblivious to the context when it processes 'pick' lines and your example shows how 'pick' lines can be part of squash/fixup command context. In conclusion, I intended to keep options disabled for squash/fixup commands but failed to do so because I neglected that a 'pick' line can initiate a squash/fixup command.

Show 8 quoted lines

> Furthermore, if we change the example to
> 
>     pick 1111111
>     squash --reset-author 2222222
>     fixup --signoff 3333333
> 
> then isn't it clear that the user's intention was to apply both options,
> "--reset-author" and "--signoff", to the resulting commit?

This seems to suggest an interpretation of todo lists similar to what I was thinking of when writing the commit message, that is one in which pick is not oblivious to the neighbouring commands. It might be a problem that it forbids the (admittedly improbable) use case where --reset-author is used to rewrite the authorship to something recent and fixup to have an even more recent committership.

To reconcile this kind of vertical interpretation with the horizontal specification of options one could introduce a todo list command taking the list of commits to be squashed as an argument. However, that seems to make it difficult to obtain the squash behaviour for some commits and the fixup behaviour for others that are part of the same chain.

The alternative interpretation of todo lists as simplified batch scripts for git commands would allow the intended behaviour (--reset-author and --signoff applied to the resulting commit), not restrict the user relatively to what she can already do on the command line and give actually different meanings to the syntactically different todo lists

    pick 1111111
    squash --reset-author 2222222
    fixup --signoff 3333333

and

    pick 1111111
    squash --signoff 2222222
    fixup --reset-author 3333333

, which would be treated identically by an implementation that collects the options. The current meaning of squash/fixup seems to be valid in the batch interpretation.

> In other
> words, it seems to me that any options on such a chain of lines should
> be collected and applied to the final commit as a whole.

To summarise, I think line options might be confusing if we interpret

    pick 1111111
    squash --reset-author 2222222
    fixup --signoff 3333333

as

    combine the changes of 1111111, 2222222, 3333333
    concatenate the messages of 1111111 and 2222222
    edit the message
    reset the authorship to the committership
    add a Signed-off-by: line

and not as

    pick 1111111
    pick -n 2222222
    commit amend --reset-author -m $squash_msg
    pick -n 3333333
    commit amend --signoff --edit

Thanks for pointing me at these issues. "Atomic" and "non-atomic" are really poorly-chosen terminology and the squash-initiating 'pick' might not be implemented correctly.

   Fabian