Re: [PATCH 0/1] revert/cherry-pick: add --show-current-patch option

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Michael

On 18/12/2023 12:10, Michael Lohmann wrote:
> Hi,
> I am a lead developer of a small team and quite often I have to
> cherry-pick commits (and sometimes also revert them). When
> cherry-picking multiple commits at once and there is a merge conflict it
> sometimes can be hard to understand what the current patch is trying to
> do in order to resolve the conflict properly. With `rebase` there is
> `--show-current-patch` and since that is quite helpful I would suggest
> to also add this flag also to `cherry-pick` and `revert`.

Thanks for bringing this up I agree it can be very helpful to look at 
the original commit when resolving cherry-pick and revert conflicts. I'm 
in two minds about this change though - I wonder if it'd be better to 
improve the documentation for CHERRY_PICK_HEAD and REVERT_HEAD and tell 
users to run "git show CHERRY_PICK_HEAD" instead. I think the main 
reason we have a "--show-current-patch" option for "rebase" is that 
there are two different implementations of that command and the 
patched-based one of them does not support REBASE_HEAD. That reasoning 
does not apply to "cherry-pick" and "revert" and "--show-current-patch" 
suggests a patch-based implementation which is also not the case for 
these commands.

Best Wishes

Phillip

> Since this is my first contribution to git I am not exactly sure where
> the best place for this functionality is. From my initial understanding
> there are two places where to put the actual invocation of the `show`:
> - Duplicate the code (with the needed adaptations) of builtin/rebase.c
>    in builtin/revert.c
> - Create a central function that shows the respective `*_HEAD` depending
>    on the current `action`.
> 
> In this first draft I went with the second option, since I felt that it
> reduces code duplication and the sequencer already has the action enum
> with exactly those three cases. On the other hand I don’t really have a
> good understanding of the role that this `sequencer` should play and if
> this adds additional coupling that is unwanted. My current impression
> is, that this would be the right place, since this looks to be the core
> of the commands where a user can apply a sequence of commits and in my
> opinion even if additional actions would be added, they could also fail
> and so it would be good to add the `--show-current-patch` option to that
> one as well.
> 
> Side note: my only C(++) experience was ~10 years ago and only for a
> single university course, so my perspective is much more from a general
> architecture point of view than based on any C experience, let alone in
> this code base and so I would be very grateful for criticism!
> 
> 
> Side note: The check for the `REBASE_HEAD` would not be necessary, since
> that is already taken care of in the builtin/rebase.c before.
> Nevertheless I opted for this check, because I would much rather require
> the same preconditions no matter from where I call this function. The
> whole argument parsing / option struct are very different between rebase
> and revert. Maybe it would make sense to align them a bit further?
> Initial observations: `rebase_options->type` is functionally similar to
> `replay_opts->action` (as in "what general action am I performing? -
> interactive rebase / cherry-pick / revert / ...") whereas
> `rebase_options->action` is not part of the `replay_opts` struct at all.
> Instead the role is taken over in builtin/revert.c by `int cmd = 0;`.
> I am preparing a patch converting this to an enum, so that there are
> no random chars that have to be kept in sync manually in different
> places, or is that a design decision?
> 
> I looked through the mailing list archive and did not find anything
> related on this topic. The only slightly related thread I could find was
> in [1] by Elijah Newren and that one was talking about a separate
> possible feature and how to get certain information if CHERRY_PICK_HEAD
> and REVERT_HEAD were to be replaced by a different construct. I hope I
> did not miss something...
> 
> Cheers
> Michael
> 
> [1]:
> https://lore.kernel.org/git/CABPp-BGd-W8T7EsvKYyjdi3=mfSTJ8zM-uzVsFnh1AWyV2wEzQ@mail.gmail.com
> 
> Michael Lohmann (1):
>    revert/cherry-pick: add --show-current-patch option
> 
>   Documentation/git-cherry-pick.txt      |  2 +-
>   Documentation/git-revert.txt           |  2 +-
>   Documentation/sequencer.txt            |  5 +++++
>   builtin/rebase.c                       |  7 ++----
>   builtin/revert.c                       |  9 ++++++--
>   contrib/completion/git-completion.bash |  2 +-
>   sequencer.c                            | 24 +++++++++++++++++++++
>   sequencer.h                            |  2 ++
>   t/t3507-cherry-pick-conflict.sh        | 30 ++++++++++++++++++++++++++
>   9 files changed, 73 insertions(+), 10 deletions(-)
>