Hi Philippe
On 18/08/2021 23:45, Philippe Blain wrote:
Hi Phillip,
[...]
For interactive rebase, an alternate implementation, that I suggested
in [1] last summer, would be to keep
the cherry-picks in the todo list, but mark them as 'drop' and add a
comment at the
end of their line, like '# already applied' or something like this,
similar
to how empty commits have '# empty' appended. I think that for
interactive rebase, I
would prefer this, since it is easier for the user to notice it and
change the 'drop'
to 'pick' right away if they realise they do not want to drop those
commits (easier
than seeing the warning, realising they did not want to drop them,
aborting the rebase
and redoing it with '--reapply-cherry-picks').
That would be nice, but we could always add it in the future if Josh
does not want to implement it now. At the moment the function that
creates the todo list does not know if it is going to be edited, I'm
not sure how easy it would be to pass that information down.
Well, I'm not sure we need to ? If we change the 'pick' to 'drop',
instead of
not writing these lines to the todo list, the cherry-picked commits will
get dropped
either way, no? Unless I'm missing something - I think you are way more
well-versed in
the sequencer code than me :)
I think I read your suggestion as meaning we would not print the warning
if the user was going to edit the list - hence the need for the
distinction. I've just had a closer look at the code and I think it
would be fairly easy to tell sequencer_make_script() if the todo list is
going to be edited, there is only one caller in builtin/rebase.c which
could easily pass a flag for this. Thinking about the 'print warning' vs
'add drop command' issue if the user is using a terminal based editor
such as vim or nano then they will barely have time to see the warnings
being printed let alone read them before the editor is opened and hides
them so having some indication in the todo list would probably be a good
idea.
Best Wishes
Phillip
Like Junio remarked, it is a little unfortunate that some logic is
duplicated between
'sequencer_make_script' and 'make_script_with_merges', such that your
patch has to do
the same thing at two different code locations. Maybe a preparatory
cleanup could add
a new function that takes care of the duplicated logic and call if
from both ? I'm
just thinking out loud here, I did not analyze in details if this
would be easy/feasible...
I think feasible but not easy (or required for this change), it would
also complicate the code in a different way as I think we'd have to
add some conditionals for whether we are recreating merges or not.
Yes, I agree it's not required for this change.
Cheers,
Philippe.
