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

Hi Michael,

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

On 08/07/2014 01:59 AM, Fabian Ruch wrote:

quoted

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.

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